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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi,

On Mon, Dec 9, 2024 at 8:17 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Michal,
>
> On 07/12/2024 15:04, Michal Orzel wrote:
> >
> >
> > On 06/12/2024 19:37, Julien Grall wrote:
> >>
> >>
> >> Hi,
> >>
> >> Sorry for the late answer.
> >>
> >> On 05/12/2024 09:40, Michal Orzel wrote:
> >>>
> >>>
> >>> On 02/12/2024 17:59, 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>
> >>>> ---
> >>>> v11:
> >>>> - GUEST_RAM_BANKS instead of hardcoding the number of banks in 
> >>>> allocate_memory()
> >>>> - hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
> >>>> - added a comment in allocate_memory() when skipping small banks
> >>>> 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             | 97 ++++++++++++++++++++++++-
> >>>>    xen/arch/arm/include/asm/domain_build.h |  1 +
> >>>>    3 files changed, 94 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..2b8cba9b2f 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,94 @@ 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 = GUEST_RAM_BANKS;
> >>>> +    paddr_t bank_start, bank_size;
> >>> Limit the scope
> >>>
> >>>> +    struct membanks *hwdom_free_mem = NULL;
> >>>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> >>>> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> >>> Limit the scope
> >>>
> >>>> +
> >>>> +    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);
> >>> This patch does not build because of declaration not being visible. You 
> >>> must include <xen/llc-coloring.h>.
> >>
> >> Piggying back on this comment. AFAICT, the code below would work also in
> >> the non cache coloring case. So what's the assert is for?
> >>
> >>>
> >>>> +
> >>>> +        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
> >>>> +                                             NR_MEM_BANKS);
> >>>> +        if ( !hwdom_free_mem )
> >>>> +            goto fail;
> >>>> +
> >>>> +        hwdom_free_mem->max_banks = NR_MEM_BANKS;
> >>>> +
> >>>> +        if ( find_unallocated_memory(kinfo, hwdom_free_mem) )
> >>> My remarks for the use of find_unallocated_memory() 1:1 have not been 
> >>> addressed. You did not even
> >>> change the comments inside the function. The problem is that the function 
> >>> is specifically designed
> >>> for finding extended regions and assumes being called at certain point 
> >>> i.e. dom0 RAM allocated, gnttab
> >>> region allocated, etc.
> >>
> >> So I agree that the function should be updated if we plan to use it for
> >> other purpose.
> >>
> >> My opinion is that we should attempt to make the function generic so
> >> that in your
> >>> case you can choose which regions to exclude, define even your own 
> >>> function to grab free regions (at the moment
> >>> add_ext_regions grabs banks >= 64M but you still discards banks >= 128M, 
> >>> so it's a bit wasteful.
> >>>
> >>> My very short attempt to make the function as generic as possible in the 
> >>> first iteration:
> >>> https://paste.debian.net/1338334/
> >>
> >> This looks better, but I wonder why we need still need to exclude the
> >> static regions? Wouldn't it be sufficient to exclude just reserved regions?
> > Static shared memory banks are not part of reserved memory (i.e. 
> > bootinfo.reserved_mem) if that's what you're asking.
> > They are stored in bootinfo.shmem, hence we need to take them into account 
> > when searching for unused address space.
>
> Oh I missed the fact you now pass "mem_banks" as a parameter. I thought
> they would still get excluded for cache coloring case.
>
> >
> > If you and Carlo are ok with my proposed solution for making the function 
> > generic, I can send a patch as a prerequisite
> > patch for Carlo series.
>
> I am fine with the approach.
>
> Cheers,
>
> --
> Julien Grall
>

> @@ -2152,7 +2238,10 @@ static int __init construct_dom0(struct domain *d)
>      /* type must be set before allocate_memory */
>      d->arch.type = kinfo.type;
>  #endif
> -    allocate_memory_11(d, &kinfo);
> +    if ( is_domain_direct_mapped(d) )
> +        allocate_memory_11(d, &kinfo);
> +    else
> +        allocate_memory(d, &kinfo);
>      find_gnttab_region(d, &kinfo);

Since find_gnttab_region() is called after allocate_memory(), kinfo->gnttab_*
fields aren't initialized and the call to find_unallocated_memory() with
gnttab as the region to exclude, fails ending in a crash since memory for
dom0 can't be allocated.

Can the solution be to call find_gnttab_region() before the above if?
Or should I just call it before allocate_memory() in one case, but still after
allocate_memory_11() in the other?

Thanks.