Re: [edk2-devel] AARCH64 Cacheability Attributes

["Oliver Smith-Denny via groups.io" <osde=linux.microsoft.com@groups.io>]

On 6/5/2025 10:27 AM, Ard Biesheuvel via groups.io wrote:
> Hi,
> 
> Thanks for elaborating.
> 
> First of all, I would assume that the device is actually non-coherent,
> or it wouldn't work at all using the non-coherent routines, given that
> these perform cache invalidation on buffers used for inbound DMA,
> which would nuke any data the device would have put there via its
> cacheable mapping (line 1453 in the same file).

Well, that's the odd thing, the device works when we have it use the
coherent and the non-coherent routines :). We aren't the platform team
though, so I am following up with them and the silicon vendor to get a
real answer here other than "well it worked."

> 
> I think it was a mistake for ARM platforms to ever use UC|WC||WT|WB by
> default for describing conventional memory. On ARM, only WC and WB
> make sense for RAM, and UC should be used for MMIO (i.e., memory
> ranges that are not backed any kind of RAM but by device registers
> where both reads and writes may have side effects). Note that not only
> misaligned accesses are problematic, also DC ZVA instructions will
> fail, and these are heavily used to accelerate SetMem()

I agree that it was a mistake for ARM platforms to use all the x86
cache types, definitely creates a confusing mess, but that leads me
to the belief that the spec should be updated to be able to fit the
architecture (which I agree, may not be another caching type, I need
to think on this more).

> 
> Simply dropping UC from the code that declares the DRAM in the
> platform startup code should fix the issue entirely, afaict. (Note
> that ArmVirtQemu et al declare their system memory as only WB capable
> and nothing else - this is needed because KVM is fundamentally
> coherent, as the VMM uses cacheable mappings to access guest memory)

That's a good point and we'll test with that. This would work for this
case because NonDiscoverablePciDxe toggles between WC and UC and chooses
WC if UC isn't supported, but there is nothing that specifies that
behavior globally. It would be valid for a driver to say I need to do
non-coherent DMA (or some other use case) and to say so I will set the
attributes to UC. On ARM, you would have to set that to WC, but now the
driver needs to know architecture level details, which is not ideal. For
x86, you would want to set UC.

> 
> Adding more memory types to the spec is not going to solve anything,
> I'm afraid. We already have some dubious ISA mask types that were
> added without proper motivation, and I'd rather have fewer types
> (e.g., on ARM, WT is also just normal non-cacheable under the hood in
> most cases and I don't have a clue how it is intended to be used).
> Instead, we should stop using UC for conventional memory.
> 

Yeah, I agree that already the invalid cacheability types are troubling
and adding more may just make the situation worse. We'll think on this
some more, I still believe this is a spec problem, that the spec does
not envision the ARM64 architecture and very explicitly envisions the
x86 architecture, which leads to issues like this (of course in many
areas).

I worry about relying on code workarounds for spec issues creates an
untenable position where a driver thinks it is doing the right thing,
per spec, but then the juggling under the hood to figure out how to
translate it to an aarch64 concept breaks down somewhere.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121400): https://edk2.groups.io/g/devel/message/121400
Mute This Topic: https://groups.io/mt/113471239/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-