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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Michal,

On Thu, Nov 28, 2024 at 11:34 AM Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 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?

Right.

> Also, the second bank won't work with CONFIG_ARM_PA_BITS_32 which limits PA 
> to 32bit.

How is this being addressed in the current allocate_memory?
Also, LLC_COLORING depends on ARM_64. ARM_PA_BITS_32 requires ARM_32, so the
two configurations should be incompatible as of now.

> > +    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?

Yes.

> > +
> > +    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.

I can definetely change that.

> 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.

For the current cases (including llc coloring) it works. If there are no
objections, a TODO plus comments and description changes you talked above is
probably sufficient to cover the (current) use-cases and "ensure" this is not
forgotten for the future extension you mention.

> > +            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];

Ok.

> This patch requires also opinion of other maintainers.
>
> ~Michal

Thanks.

- Carlo