Re: [PATCH v4 2/9] xen/arm: implement domU extended regions

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/04/2022 01:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>
> Implement extended regions for dom0less domUs. The implementation is
> based on the libxl implementation.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de..b6189b935d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1324,6 +1324,35 @@ out:
>       return res;
>   }
>   
> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
I think this is the same as ROUNDUP(x, SZ_2M).

> +
> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> +                                  struct meminfo *ext_regions)
> +{
> +    unsigned int i;
> +    uint64_t bankend[GUEST_RAM_BANKS];
Looking, you only seem to use one bankend at the time. So why do you 
need to store all the bankend?
This should also be s/uint64_t/paddr_t/. Same for two other instances below.

> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +
> +    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> +    {
> +        ext_regions->bank[ext_regions->nr_banks].start =
The code would be easier to read if you define a local variable ext_bank 
that points to &(ext_regions->bank[ext_regions->nr_banks]).
> +            ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
> +
> +        bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1); > +        if 
> (bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)
Coding style:

if ( ... )

> +            ext_regions->bank[ext_regions->nr_banks].size =
> +                bankend[i] - ext_regions->bank[ext_regions->nr_banks].start + 
> 1;
This is one of the line that could greatly benefits from the local 
variable I suggested above. It would look like:
ext_bank->size = bankend[i] - ext_bank->start + 1;

> +
> +        /* 64MB is the minimum size of an extended region */
> +        if ( ext_regions->bank[ext_regions->nr_banks].size < MB(64) )
> +            continue;
> +        ext_regions->nr_banks++;
> +    }
NIT: We tend to add a newline before the last return.

> +    return 0;
find_memory_holes() and find_unallocated_memory() will return an error 
if there are no banks allocated. I think we should do the same here at 
least for consistency.
In which case, the check should be moved in make_hypervisor_node().

> +}
> +
>   static int __init make_hypervisor_node(struct domain *d,
>                                          const struct kernel_info *kinfo,
>                                          int addrcells, int sizecells)
> @@ -1374,10 +1403,17 @@ static int __init make_hypervisor_node(struct domain *d,
>           if ( !ext_regions )
>               return -ENOMEM;
>   
> -        if ( !is_iommu_enabled(d) )
> -            res = find_unallocated_memory(kinfo, ext_regions);
> +        if ( is_domain_direct_mapped(d) )
I believe the code in the 'if' part would work properly for a dom0 that 
is not direct mapped (e.g. in the cache coloring case).
If it doesn't, I think we need...

> +        {
> +            if ( !is_iommu_enabled(d) )
> +                res = find_unallocated_memory(kinfo, ext_regions);
> +            else
> +                res = find_memory_holes(kinfo, ext_regions);
> +        }
>           else
> -            res = find_memory_holes(kinfo, ext_regions);
> +        {
... and ASSERT() here so we the person that will introduce non direct 
mapped dom0 can easily notice before the domain get corrupted.
> +            res = find_domU_holes(kinfo, ext_regions);
> +        }
>   
>           if ( res )
>               printk(XENLOG_WARNING "Failed to allocate extended regions\n");
This printk() and the others in the function should print the domain.

Cheers,

--
Julien Grall