Re: [PATCH] xen/arm: Fix under-mapping in map_range_to_domain()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 10-Oct-26 12:38, Julien Grall wrote:
> Hi Michal,
> 
> On 08/10/2026 11:50, Orzel, Michal wrote:
>>
>>
>> On 07-Oct-26 22:20, Grall, Julien wrote:
>>> Hi Michal,
>>>
>>> On 07/10/2026 15:58, Michal Orzel wrote:
>>>> If the memory region to be mapped does not start at the page boundary
>>>> and its end spans over the page boundary, map_range_to_domain() passes
>>>> a number of pages that is short by one to map_regions_p2mt(). Domain's
>>>> attempt to access memory within that page will fail. Fix it by calculating
>>>> the number of pages to be mapped taking into account the start offset.
>>>
>>> Can you provide an example where the caller doesn't suitably align?
>> Sure. Example is handle_device() being called on the hwdom DTB creation time 
>> for
>> a node whose address is not page aligned i.e. /reserved-memory node or 
>> basically
>> any node, given that DT spec does not mention anything about memory region
>> alignment and there are existing DTs like that. With reg = <0x0 0x800 0x0
>> 0x1000>, today we only map 0x0-0xfff and an access to 0x1000-0x17ff faults.
> 
> Thanks!
>>
>>>
>>>>
>>>> Fixes: 88c10b0d6eee ("arch/arm: let map_mmio_regions() use start and 
>>>> count")
>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>
>>> The fix LGTM in the current context. However given map_range_to_domain()
>>> is now used in more places (e.g. overlay), I wonder whether this is the
>>> right decision to allow map_range_to_domain() with non-aligned region.
>>>
>>> Imagine a user say "I want to map 0x400 - 0x1000 to domain A and map
>>> domain B "0x0 - 0x400". This could result to interesting behavior if the
>>> user is not aware of the alignment (this would become even more
>>> problematic if we decide to ever support different page granularity on Arm).
>> I agree this is not great, but this is not something introduced here:
>> iomem_permit_access() and the iomem_ranges rangeset already cover whole pages
>> and we already map the full first page.This patch only makes the p2m mapping
>> match what we already allow.
> 
> I am possibly misinterpreting what you wrote, but I disagree with such 
> statement. The fact existing code may be wrong doesn't mean we should 
> continue with the wrongness.
It was just to clarify that this patch does not change who can access the page.

> 
>>
>> I don't think we can reject unaligned regions in map_range_to_domain()
>> though, as the host DT is not under our control and hwdom would fail to
>> boot on platforms with sub-page devices. I asked AI to look at the Linux 
>> boards
>> for Arm and Arm64 (140 boards in total with unaligned regions) and even the
>> boards we claim to support have nodes that would fail:
>> - RPI4: hdmi@7ef00700, <0x7ef01f00 0x400>
>> - IMX8QXP: clock-controller@5b290004, <0x5b290004 0x10000>
> 
> I am not suggesting to reject unaligned regions when mapped into the 
> hardware domain. I am suggesting to reject it when we want to 
> passthrough to another domain. So we don't end up in a situation two 
> different devices on the same page are passthrough to two different 
> domains (or between Xen and a domain).
> 
> Codewise, it would mean map_range_to_domain() would enforce the 
> alignment and the caller will be left to decide what to do. We could 
> also decide about the alignment base don whether this is the HW domain 
> or not within the function.
For passthrough this is already the case. map_range_to_domain() does
not map anything for nodes with xen,passthrough and the domU mapping is done by
handle_passthrough_prop(), which rejects mstart, gstart and size that are not
page aligned.

The remaining case is the overlay which can be attached to any 1:1 domain and
this goes through handle_device() -> map_range_to_domain() without any alignment
check. All the other callers operate on hwdom, so I think we could reject a
region that is not page aligned if d is not the hardware domain, before granting
the iomem permission in map_range_to_domain().

Shouldn't we accept this patch as is so that we can backport (overlay is not
supported) it as a fix and then add a second one with a rejection mentioned
above that will help the overlay (and any future) case?

~Michal