Re: [edk2-devel] AARCH64 Cacheability Attributes

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

On 6/5/2025 11:19 AM, Ard Biesheuvel via groups.io wrote:
> On Thu, 5 Jun 2025 at 19:55, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>> 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.
>>
> 
> No. Whether DMA is coherent or not is /not/ a property of the device,
> it is a property of how that device was integrated into the system.

I agree with that, I was thinking more of the PciNonDiscoverable case,
but more on that below.

> 
> So a device driver should never have to reason about the difference
> between coherent or non-coherent DMA, unless it is tightly coupled
> with the platform, in which case this would not be an issue.
> 
> Note that the NonCoherentDmaLib in EmbeddedPkg already prefers WC over
> UC. I don't remember why I didn't do the same when I wrote
> NonDiscoverablePciDxe, because it is the obvious correct thing to do
> on ARM (and x86 doesn't use this driver at all afaik)
> 

I think you are right here, that we should move to prefer WC over UC in
NonDiscoverablePciDxe since these are host buffers and may need to be
normal uncacheable but should never be device memory (so to that end,
should it only set WC and not UC at all?).

> 
>>>
>>> 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.
>>
> 
> The driver should not care about the difference. If there are
> currently drivers that need to, we should fix the abstractions
> instead.
> 

I will go through edk2 and see if anything is breaking this assumption,
I agree that the platform is the only one who knows the cacheability.

I still think it is unfortunate that we have to do some mapping between
x86 attributes and aarch64 attributes, but if we can ensure edk2 is
clean w.r.t. class drivers not setting cacheability attributes, I think
we can get away with it.

If you are amenable to it, I'll put up a PR for NonDiscoverablePciDxe to
prefer WC over UC (or someone on my team will), though want your input
on whether we allow UC at all there.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121402): https://edk2.groups.io/g/devel/message/121402
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]
-=-=-=-=-=-=-=-=-=-=-=-