[BUG] ARM64 KVM: Data abort executing post-indexed LDR on MMIO address

[Ahmad Fatoum]

Hello Marc,

On 05.10.24 23:35, Marc Zyngier wrote:
> On Sat, 05 Oct 2024 19:38:23 +0100,
> Ahmad Fatoum <a.fatoum at pengutronix.de> wrote:
>>> IIRC, the QEMU flash is implemented as a read-only memslot. A data
>>> cache invalidation is a *write*, as it can be (and is) upgraded to a
>>> clean+invalidate by the HW.
>>
>> So it's a write, even if there are no dirty cache lines?
> 
> Yes.
> 
> At the point where this is handled, the CPU has no clue about the
> dirty state of an arbitrary cache line, at an arbitrary cache level.
> The CPU simply forward the CMO downstream, and the permission check
> happens way before that.

I see. When I added that invalidation, it was suggested[1] that instead
of invalidation after the remapping, I would clean the range before
remapping. Cleaning the zero page triggered a data abort, which I didn't
understand as there should be no dirty lines, but now I get it.

One more question: This upgrading of DC IVAC to DC CIVAC is because
the code is run under virtualization, right?

[1]: https://lore.barebox.org/barebox/9809c04c-58c5-6888-2b14-cf17fa7c4b1a@pengutronix.de/

>> The routine[1] that does this remapping invalidates the virtual address range,
>> because the attributes may change[2]. This invalidate also happens for cfi-flash,
>> but we should never encounter dirty cache lines there as the remap is done
>> before driver probe.
>>
>> Can you advise what should be done differently?
> 
> If you always map the flash as Device memory, there is no need for
> CMOs. Same thing if you map it as NC.

Everything, except for RAM, is mapped Device-nGnRnE.

> And even if you did map it as
> Cacheable, it wouldn't matter. KVM already handles coherency when the
> flash is switching between memory-mapped and programming mode, as the
> physical address space changes (the flash literally drops from the
> memory map).
> 
> In general, issuing CMOs to a device is a bizarre concept, because it
> is pretty rare that a device can handle a full cache-line as
> write-back. Devices tend to handle smaller, register-sized accesses,
> not a full 64-byte eviction.

The same remap_range function is used to:

  - remap normal memory:
    - Mapping memory regions uncached for memory test
    - Mapping memory buffers coherent or write-combine
  - remap ROMs: BootROMs at address zero may be hidden behind the NULL
    page and need to be mapped differently when calling/peeking into it.
  - remap device memory, e.g. in the case of the cfi-flash here

The invalidation is done unconditionally for all of them, although it
makes only the sense in the first case.

> Now, I'm still wondering whether we should simply forward the CMO to
> userspace as we do for other writes, and let the VMM deal with it. The
> main issue is that there is no current way to describe this.
> 
> The alternative would be to silently handle the trap and pretend it
> never occurred, as we do for other bizarre behaviours. But that'd be
> something only new kernels would support, and I guess you'd like your
> guest to work today, not tomorrow.

I think following fix on the barebox side may work:

  - Walk all pages about to be remapped
  - Execute the AT instruction on the page's base address
  - Only if the page was previously mapped cacheable, clean + invalidate
    the cache
  - Remove the current cache invalidation after remap

Does that sound sensible?

Thanks,
Ahmad

> 
> 	M.
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |