Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 30/01/2023 04:05, Henry Wang wrote:
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index a926f30a2b..f0592370ea 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
>   size_t boot_fdt_info(const void *fdt, paddr_t paddr);
>   const char *boot_fdt_cmdline(const void *fdt);
>   
> +bool check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size);
> +
>   struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                      paddr_t start, paddr_t size, bool domU);
>   struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..636604aafa 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -261,6 +261,32 @@ static void __init dt_unreserved_regions(paddr_t s, 
> paddr_t e,
>       cb(s, e);
>   }
>   
> +static bool __init meminfo_overlap_check(struct meminfo *meminfo,
> +                                         paddr_t region_start,
> +                                         paddr_t region_size)
So the interface looks nicer but now...

> +{
> +    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> +    paddr_t region_end = region_start + region_size;
> +    unsigned int i, bank_num = meminfo->nr_banks;
> +
> +    for ( i = 0; i < bank_num; i++ )
> +    {
> +        bank_start = meminfo->bank[i].start;
> +        bank_end = bank_start + meminfo->bank[i].size;
> +
> +        if ( region_end <= bank_start || region_start >= bank_end )
... it clearly shows how this check would be wrong when either the bank 
or the region is at the end of the address space. You may say it doesn't 
overlap when it could (e.g. when region_end < region_start).
That said, unless we rework 'bank', we would not properly solve the 
problem. But that's likely a bigger piece of work and not something I 
would request.
So for now, I would suggest to add a comment. Stefano, what do you think?

> +            continue;
> +        else
> +        {
> +            printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with bank[%u]: 
> [%#"PRIpaddr", %#"PRIpaddr"]\n",
']' usually mean inclusive. But here, 'end' is exclusive. So you want '['.

This could be fixed on commit.


BTW, the same comments applies for the second patch.

> +                   region_start, region_end, i, bank_start, bank_end);
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>   void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>                                     void (*cb)(paddr_t, paddr_t),
>                                     unsigned int first)
> @@ -271,7 +297,22 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>           cb(s, e);
>   }
>   
> +/*
> + * Given an input physical address range, check if this range is overlapping
> + * with the existing reserved memory regions defined in bootinfo.
> + * Return true if the input physical address range is overlapping with any
> + * existing reserved memory regions, otherwise false.
> + */
> +bool __init check_reserved_regions_overlap(paddr_t region_start,
> +                                           paddr_t region_size)
> +{
> +    /* Check if input region is overlapping with bootinfo.reserved_mem banks */
> +    if ( meminfo_overlap_check(&bootinfo.reserved_mem,
> +                               region_start, region_size) )
> +        return true;
>   
> +    return false;
> +}
>   
>   struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                                             paddr_t start, paddr_t size,
Cheers,

--
Julien Grall