Re: [PATCH v10 03/12] xen/arm: permit non direct-mapped Dom0 construction

[Michal Orzel <michal.orzel@xxxxxxx>]

On 19/11/2024 15:13, Carlo Nonato wrote:
> 
> 
> Cache coloring requires Dom0 not to be direct-mapped because of its non
> contiguous mapping nature, so allocate_memory() is needed in this case.
> 8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
> moved allocate_memory() in dom0less_build.c. In order to use it
> in Dom0 construction bring it back to domain_build.c and declare it in
> domain_build.h.
> 
> Take the opportunity to adapt the implementation of allocate_memory() so
> that it uses the host layout when called on the hwdom, via
> find_unallocated_memory().
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> ---
> v10:
> - fixed a compilation bug that happened when dom0less support was disabled
> v9:
> - no changes
> v8:
> - patch adapted to new changes to allocate_memory()
> v7:
> - allocate_memory() now uses the host layout when called on the hwdom
> v6:
> - new patch
> ---
>  xen/arch/arm/dom0less-build.c           | 44 ------------
>  xen/arch/arm/domain_build.c             | 96 +++++++++++++++++++++++--
>  xen/arch/arm/include/asm/domain_build.h |  1 +
>  3 files changed, 93 insertions(+), 48 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index d93a85434e..67b1503647 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
>      return ( !dom0found && domUfound );
>  }
> 
> -static void __init allocate_memory(struct domain *d, struct kernel_info 
> *kinfo)
> -{
> -    struct membanks *mem = kernel_info_get_mem(kinfo);
> -    unsigned int i;
> -    paddr_t bank_size;
> -
> -    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> -           /* Don't want format this as PRIpaddr (16 digit hex) */
> -           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> -
> -    mem->nr_banks = 0;
> -    bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> -                               bank_size) )
> -        goto fail;
> -
> -    bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> -                               bank_size) )
> -        goto fail;
> -
> -    if ( kinfo->unassigned_mem )
> -        goto fail;
> -
> -    for( i = 0; i < mem->nr_banks; i++ )
> -    {
> -        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" 
> (%ldMB)\n",
> -               d,
> -               i,
> -               mem->bank[i].start,
> -               mem->bank[i].start + mem->bank[i].size,
> -               /* Don't want format this as PRIpaddr (16 digit hex) */
> -               (unsigned long)(mem->bank[i].size >> 20));
> -    }
> -
> -    return;
> -
> -fail:
> -    panic("Failed to allocate requested domain memory."
> -          /* Don't want format this as PRIpaddr (16 digit hex) */
> -          " %ldKB unallocated. Fix the VMs configurations.\n",
> -          (unsigned long)kinfo->unassigned_mem >> 10);
> -}
> -
>  #ifdef CONFIG_VGICV2
>  static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>  {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2c30792de8..a95376dcdc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -416,7 +416,6 @@ static void __init allocate_memory_11(struct domain *d,
>      }
>  }
> 
> -#ifdef CONFIG_DOM0LESS_BOOT
>  bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
>                                      alloc_domheap_mem_cb cb, void *extra)
>  {
> @@ -508,7 +507,6 @@ bool __init allocate_bank_memory(struct kernel_info 
> *kinfo, gfn_t sgfn,
> 
>      return true;
>  }
> -#endif
> 
>  /*
>   * When PCI passthrough is available we want to keep the
> @@ -1003,6 +1001,93 @@ out:
>      return res;
>  }
> 
> +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> +{
> +    struct membanks *mem = kernel_info_get_mem(kinfo);
> +    unsigned int i, nr_banks = 2;
Instead of hardcoding, shouldn't it be GUEST_RAM_BANKS?
Also, the second bank won't work with CONFIG_ARM_PA_BITS_32 which limits PA to 
32bit.


> +    paddr_t bank_start, bank_size;
> +    struct membanks *hwdom_ext_regions = NULL;
AFAICT you search for free memory. Naming it as extended regions is not a good 
choice.
Instead, hwdom_free_mem?

> +
> +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> +           /* Don't want format this as PRIpaddr (16 digit hex) */
> +           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> +
> +    mem->nr_banks = 0;
> +    /*
> +     * Use host memory layout for hwdom. Only case for this is when LLC 
> coloring
> +     * is enabled.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        ASSERT(llc_coloring_enabled);
> +
> +        hwdom_ext_regions = xzalloc_flex_struct(struct membanks, bank,
> +                                                NR_MEM_BANKS);
> +        if ( !hwdom_ext_regions )
> +            goto fail;
empty line here please

> +        hwdom_ext_regions->max_banks = NR_MEM_BANKS;
> +
> +        if ( find_unallocated_memory(kinfo, hwdom_ext_regions) )
If you reuse find_unallocated_memory for a purpose other than extended regions, 
I think
the description of this function should change as well as comments inside.

Today, the function gets all RAM and exclude dom0 RAM (in your case is 0 at 
this point, reserved memory,
static shmem and gnttab (in your case is 0 at this point). I think we cannot 
get away without
making this function more generic. Maybe it should take as a parameter struct 
membanks * array?
Also, the callback add_ext_regions() may not be suited for all purposes (i.e. 
it takes only banks
> 64MB into account). I know that there will be more use cases for a function 
> that will return the
free memory for domains. As an example, today, for direct mapped domains we 
hardcode the gnttab region
(only dom0 is special cased). This should not be like that. We would need to 
find a free memory region
to expose as gnttab.

> +            goto fail;
> +
> +        nr_banks = hwdom_ext_regions->nr_banks;
> +    }
> +
> +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
> +    {
> +        if ( is_hardware_domain(d) )
> +        {
> +            bank_start = hwdom_ext_regions->bank[i].start;
> +            bank_size = hwdom_ext_regions->bank[i].size;
> +
> +            if ( bank_size < min_t(paddr_t, kinfo->unassigned_mem, MB(128)) )
I would expect some explanation.

> +                continue;
> +        }
> +        else
> +        {
> +            if ( i == 0 )
> +            {
> +                bank_start = GUEST_RAM0_BASE;
> +                bank_size = GUEST_RAM0_SIZE;
> +            }
> +            else if ( i == 1 )
> +            {
> +                bank_start = GUEST_RAM1_BASE;
> +                bank_size = GUEST_RAM1_SIZE;
> +            }
> +            else
> +                goto fail;
This could be simplified:
const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;

if ( i >= GUEST_RAM_BANKS )
    goto fail;

bank_start = bankbase[i];
bank_size = banksize[i];

This patch requires also opinion of other maintainers.

~Michal