Re: [PATCH] xen/arm: fix iomem_ranges cfg in map_range_to_domain()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/02/2025 12:55, Grygorii Strashko wrote:
> Now the following code in map_range_to_domain()
>
> res = iomem_permit_access(d, paddr_to_pfn(addr),
>                  paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>
> and
>
> res = rangeset_add_range(mr_data->iomem_ranges,
>                           paddr_to_pfn(addr),
>                           paddr_to_pfn_aligned(addr + len - 1));
> > will incorrectly calculate end address of the iomem_range by 
rounding it up
> to the next Xen page, which in turn will give Control domain access to
> manage incorrect MMIO range.
I think the key point that needs to be spelled out is that both 
functions are expecting "end" to be inclusive. Whereas the code is 
thinking that the end should be exclusive. This is a very common error 
in Xen and why I have been advocating several times to switch to use
"start, nr" rather than "start, end".

> For example, requested range:
>    00e6140000 - 00e6141004 should set e6140:e6141 nr=2, but will configure
> e6140 e6142 nr=3 instead.
I am not sure what 'nr' is referring to here.

Also, with the range you provide above, it means that you will still 
give access to more than necessary. That's because you give access to 
the full page but only the first four bytes are valid. Is this intended?
> Note. paddr_to_pfn_aligned() has PAGE_ALIGN() inside.
>
> Fix it, by using paddr_to_pfn(addr + len - 1) in both places to get correct
> end address of the iomem_range.
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>

I think this wants to have a fixes tag and possibly split in two because 
I suspect we may need to backport the changes to different versions.
> ---
>
> Hi All,
>
> I have a question - the paddr_to_pfn_aligned() is not used any more,
> should I remove it as part of this patch?
I would keep it. It might be used in the future.

>   xen/arch/arm/device.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 5610cddcba8e..5e1c1cc326ac 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
>                        strlen("/reserved-memory/")) != 0 )
>       {
>           res = iomem_permit_access(d, paddr_to_pfn(addr),
> -                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> +                                  paddr_to_pfn(addr + len - 1));
>           if ( res )
>           {
>               printk(XENLOG_ERR "Unable to permit to dom%d access to"
> @@ -107,7 +107,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
>       {
>           res = rangeset_add_range(mr_data->iomem_ranges,
>                                    paddr_to_pfn(addr),
> -                                 paddr_to_pfn_aligned(addr + len - 1));
> +                                 paddr_to_pfn(addr + len - 1));
>           if ( res )
>               return res;
>       }
Cheers,

--
Julien Grall