Re: [PATCH 2/2] make ioremap_wc() x86 only (for the time being)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2026 05:31, Julien Grall wrote:
> Hi Oleksii,
> 
> On 30/03/2026 15:24, Oleksii Kurochko wrote:
>>
>>
>> On 3/30/26 4:22 PM, Oleksii Kurochko wrote:
>>>
>>>
>>> On 2/19/26 4:53 PM, Jan Beulich wrote:
>>>> Its use in domain building is questionable: Already at the point both 
>>>> uses
>>>> were introduced, ioremap_cache() existed. I can't see why kernel and
>>>> initrd would need mapping WC, when at the same time other similar 
>>>> mappings
>>>> (in common/device-tree/) are done WB.
>>>>
>>>> With those uses replaced, neither Arm nor RISC-V have a need for the
>>>> function anymore.
>>>>
>>>
>>> It is okay for RISC-V, but for Arm, IIRC, likely it was done because 
>>> guest might start with cache disabled and PAGE_HYPERVISOR_WC on Arm 
>>> uses "Normal Non-cacheable" what could lead to that guest won't see 
>>> some part of kernel and/or initrd as it could be in a cache and won't 
>>> be flushed to RAM.
>>>
>>> But probably I misremembered something and 
>>> clean_and_invalidate_dcache_va_range() or something similar should be 
>>> called before guest is laucned.
>>
>> okay, so for this case copy_to_guest_phys_flush_dcache() is called after 
>> initrd and kernel are loaded.
> 
> Looking at the code, we are using ioremap_wc() on the source side rather 
> than the destination. So the use of copy_to_guest_phys_flush_cache() 
> would not matter as this only applies to the destination.
> 
> Regarding the source side, the cache only guarantees coherency when either:
>    * the memory attributes are similar between the component that loaded 
> it (most likely the firmware) and Xen
>    * The component that loaded it issued a cache flush
> 
> I saw Jan mentioned that other part of the device-tree code are using 
> ioremap_cache(). They were adding afterwards and I can't remember why we 
> consisder it was ok.

This sounds like you think it isn't okay.

> For older mapping we would be switching from a 
> non-cacheable mapping (HYPERVISOR_WC) to a cacheable one (HYPERVISOR).

Whereas this sounds like you think switching everything to WB (or whatever
the correct term on Arm) would be okay. IOW - I'm confused, and hence I'm
...

> While I agree we should have some consistency in the mappings, I am a 
> bit unease to change the attributes without any written details on why 
> there will not be any impact.

... also unclear as to whether this actually is an objection to the Arm
part of the change (incl patch 1). Please clarify.

As to written details: Mappings of ordinary RAM shouldn't need any extra
documentation when done WB. Any non-standard mappings (like WC) should
instead have comments. The absence of such comments indicates to me that
at the time the "best thing thought to be available" was used, i.e. WC
preferred over UC (which plain ioremap() would have used). Additionally,
the use of ioremap_wc() from initrd_load() (not Arm-specific) pretty
clearly isn't appropriate for RISC-V, aiui.

That said, using ioremap*() for RAM feels bogus in the first place. At
the time vmap() didn't exist yet, so it may have been appropriate. If
there are reasons not to use it here, perhaps something like Linux'es
memremap() should long have been introduced?

Jan