Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU

[Oleksandr <olekstysh@xxxxxxxxx>]

On 12.10.21 19:05, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien



> On 11/10/2021 18:48, Oleksandr Tyshchenko wrote:
> > ---
> >   tools/libs/light/libxl_arm.c  | 76 
> > ++++++++++++++++++++++++++++++++++++++++---
> >   xen/include/public/arch-arm.h |  2 ++
> >   2 files changed, 73 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index e3140a6..c0e8415 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -598,9 +598,20 @@ static int make_timer_node(libxl__gc *gc, void 
> > *fdt,
> >       return 0;
> >   }
> >   +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> > +
> >   static int make_hypervisor_node(libxl__gc *gc, void *fdt,
> > -                                const libxl_version_info *vers)
> > +                                const libxl_version_info *vers,
> > +                                const libxl_domain_build_info *b_info,
> > +                                const struct xc_dom_image *dom)
> >   {
> > +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
> > region_base[GUEST_RAM_BANKS],
> > +        bank1end, ramsize;
> > +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> > +                  (GUEST_RAM_BANKS + 1)];
> > +    be32 *cells = &regs[0];
> > +    unsigned int i, len, nr_regions = 0;
> > +    libxl_dominfo info;
> >       int res;
> >       gic_interrupt intr;
> >   @@ -615,9 +626,64 @@ static int make_hypervisor_node(libxl__gc *gc, 
> > void *fdt,
> >                                 "xen,xen");
> >       if (res) return res;
> >   -    /* reg 0 is grant table space */
> > -    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
> > GUEST_ROOT_SIZE_CELLS,
> > -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> > +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> > +        LOG(WARN, "The extended regions are only supported for 
> > 64-bit guest currently");
> > +        goto out;
> > +    }
> I understand why we want to limit to 64-bit domain for dom0. But I am 
> not sure this is warrant for 32-bit domain. At worse, the guest will 
> ignore the bank because it is not usable. So could we drop the check?
Yes.


> > +
> > +    res = libxl_domain_info(CTX, &info, dom->guest_domid);
> > +    if (res) return res;
> > +
> > +    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
> What could go wrong below if gpaddr_bits is not within this range?
if info.gpaddr_bits is less than 64, then nothing bad, otherwise, I 
assume we will get shift count overflow.

> > +
> > +    /*
> > +     * Try to allocate separate 2MB-aligned extended regions from 
> > the first
> > +     * (below 4GB) and second (above 4GB) RAM banks taking into the 
> > account
> > +     * the maximum supported guest physical address space size and 
> > the amount
> > +     * of memory assigned to the guest.
> > +     * As the guest memory layout is not populated yet we cannot 
> > rely on
> > +     * dom->rambank_size[], so calculate the actual size of both 
> > banks using
> > +     * "max_memkb" value.
> > +     */
> At the moment, libxl doesn't know how libxc will allocate the memory. 
> We may decide in the future to have only a small amount of memory 
> below 4GB and then the rest above 4GB. With this approach it would be 
> more difficult to modify the memory layout. Instead, I think we should 
> create a placeholder that is updated once we know the banks in 
> libxl__arch_domain_finalise_hw_description.
If I got your point correctly, this is close to how it was done from the 
beginning. Yes, we can create placeholder(s) here and then update them 
once the memory layout is populated. The problem is that we won't be 
able to remove the placeholder(s) if we fail to allocate region(s) for 
some reasons. So, we should know for sure in advance how many region(s) 
we will be able to allocate later on in order to create the required 
number of placeholders right now...  Please, look at the TODO I wrote in 
finalise_ext_region() [1]. Or I misread your point?

> We also probably want to mention in the memory layout in 
> public/arch-arm.h this decision as the suggested way to find extended 
> regions will definitely impact our decision to re-order the memory 
> layout or shrink some regions in the future (I have in mind the PCI 
> Passthrough work).
Sorry, I couldn't parse.


> > +    ramsize = b_info->max_memkb * 1024;
> > +    if (ramsize <= GUEST_RAM0_SIZE) {
> > +        region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
> > +        region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
> > +        region_base[1] = GUEST_RAM1_BASE;
> > +    } else
> > +        region_base[1] = GUEST_RAM1_BASE +
> > +            ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
> > +
> > +    bank1end = min(1ULL << info.gpaddr_bits, GUEST_RAM1_BASE + 
> > GUEST_RAM1_SIZE);
> > +    if (bank1end > region_base[1])
> > +        region_size[1] = bank1end - region_base[1];
> It would be best to not rely on the fact that Bank on is always below 
> 4GB. If the code is too complex then we should look to add a 
> BUILD_BUG_ON() to avoid any surprise.
Yes, I can add:

BUILD_BUG_ON((GUEST_RAM0_BASE + GUEST_RAM0_SIZE) > GB(4));


[1] 
https://lore.kernel.org/xen-devel/1631297924-8658-4-git-send-email-olekstysh@xxxxxxxxx/

> Cheers,
--
Regards,

Oleksandr Tyshchenko