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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 13 Oct 2021, Oleksandr wrote:
> On 13.10.21 21:26, Oleksandr wrote:
> > 
> > On 13.10.21 20:07, Julien Grall wrote:
> > 
> > Hi Julien
> > 
> > > Hi,
> > > 
> > > On 13/10/2021 17:44, Oleksandr wrote:
> > > > On 13.10.21 19:24, Julien Grall wrote:
> > > > > On 13/10/2021 16:49, Oleksandr wrote:
> > > > > > 
> > > > > > On 13.10.21 18:15, Julien Grall wrote:
> > > > > > > On 13/10/2021 14:46, Oleksandr wrote:
> > > > > > > > 
> > > > > > > > Hi Julien
> > > > > > > 
> > > > > > > Hi Oleksandr,
> > > > > > 
> > > > > > Hi Julien
> > > > > > 
> > > > > > 
> > > > > > Thank you for the prompt response.
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > On 13.10.21 00:43, Oleksandr wrote:
> > > > > > > > diff --git a/tools/libs/light/libxl_arm.c
> > > > > > > > b/tools/libs/light/libxl_arm.c
> > > > > > > > index e3140a6..53ae0f3 100644
> > > > > > > > --- a/tools/libs/light/libxl_arm.c
> > > > > > > > +++ b/tools/libs/light/libxl_arm.c
> > > > > > > > @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc
> > > > > > > > *gc, void *fdt,
> > > > > > > >                                 "xen,xen");
> > > > > > > >       if (res) return res;
> > > > > > > > 
> > > > > > > > -    /* reg 0 is grant table space */
> > > > > > > > +    /*
> > > > > > > > +     * reg 0 is a placeholder for grant table space, reg 1...N
> > > > > > > > are
> > > > > > > > +     * the placeholders for extended regions.
> > > > > > > > +     */
> > > > > > > >       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > > > > > > > GUEST_ROOT_SIZE_CELLS,
> > > > > > > > -                            1,GUEST_GNTTAB_BASE,
> > > > > > > > GUEST_GNTTAB_SIZE);
> > > > > > > > +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0,
> > > > > > > > 0);
> > > > > > > 
> > > > > > > Here you are relying on GUEST_RAM_BANKS == 2. I think this is
> > > > > > > pretty fragile as this may change in the future.
> > > > > > > 
> > > > > > > fdt_property_regs() is not really suitable for here. I would
> > > > > > > suggest to create a new helper fdt_property_placeholder() which
> > > > > > > takes the address cells, size cells and the number of ranges. The
> > > > > > > function will then create N range that are zeroed.
> > > > > > 
> > > > > > You are right. Probably, we could add an assert here for now to be
> > > > > > triggered if "GUEST_RAM_BANKS != 2"?
> > > > > > But, if you still think we need to make it with the helper right
> > > > > > now, I will. However, I don't know how long this will take.
> > > > > 
> > > > > I would prefer if we introduce the helper now. Below a potential
> > > > > version (not compiled):
> > > > 
> > > > 
> > > > You wouldn't believe)))
> > > > I decided to not wait for the answer and re-check. So, I ended up with
> > > > the similar to what you suggested below. Thank you.
> > > > Yes, it will work if add missing locals. However, I initially named it
> > > > exactly as was suggested (fdt_property_placeholder) and got a
> > > > compilation error, since fdt_property_placeholder is already present.
> > > > So, I was thinking to choose another name or to even open-code it, but I
> > > > see you already proposed new name fdt_property_reg_placeholder.
> > > 
> > > Ah I didn't realize that libfdt already implemented
> > > fdt_property_placeholder(). The function sounds perfect for us (and the
> > > code is nicer :)), but unfortunately this was introdcued only in 2017. So
> > > it is possible that some distros may not ship the last libfdt.
> > > 
> > > So for now, I think we need to implement our own. In a follow-up we could
> > > try to provide a compat layer as we did for other missing fdt_* helpers.
> > > 
> > > Cheers,
> > 
> > 
> > Well, I hope that I addressed all your comments. If so, I will prepare V7.
> 
> 
> May I please ask, are you *preliminary* OK with new changes requested by
> Julien? The main changes are to bring finalize_*() back (hopefully there is a
> way how to downsize a placeholder), avoid relying on the fact that Bank 0 is
> always below 4GB, adding a convenient helper to create a placeholder for N
> ranges plus some code hardening, etc.

Yes, I am OK with it


> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index e3140a6..a780155 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -269,6 +269,21 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
> >      return fdt_property(fdt, "reg", regs, sizeof(regs));
> >  }
> > 
> > +static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
> > +                                        unsigned int addr_cells,
> > +                                        unsigned int size_cells,
> > +                                        unsigned int num_regs)
> > +{
> > +    uint32_t regs[num_regs * (addr_cells + size_cells)];
> > +    be32 *cells = &regs[0];
> > +    unsigned int i;
> > +
> > +    for (i = 0; i < num_regs; i++)
> > +        set_range(&cells, addr_cells, size_cells, 0, 0);
> > +
> > +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> > +}
> > +
> >  static int make_root_properties(libxl__gc *gc,
> >                                  const libxl_version_info *vers,
> >                                  void *fdt)
> > @@ -615,9 +630,13 @@ 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);
> > +    /*
> > +     * reg 0 is a placeholder for grant table space, reg 1...N are
> > +     * the placeholders for extended regions.
> > +     */
> > +    res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > +                                       GUEST_ROOT_SIZE_CELLS,
> > +                                       GUEST_RAM_BANKS + 1);
> >      if (res) return res;
> > 
> >      /*
> > @@ -1069,20 +1088,93 @@ static void finalise_one_node(libxl__gc *gc, void
> > *fdt, const char *uname,
> >      }
> >  }
> > 
> > +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> > +
> > +#define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> > +
> > +static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image
> > *dom)
> > +{
> > +    void *fdt = dom->devicetree_blob;
> > +    uint64_t region_size[GUEST_RAM_BANKS] = {0},
> > region_base[GUEST_RAM_BANKS],
> > +        bankend[GUEST_RAM_BANKS];
> > +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> > +                  (GUEST_RAM_BANKS + 1)];
> > +    be32 *cells = &regs[0];
> > +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> > +    unsigned int i, len, nr_regions = 0;
> > +    libxl_dominfo info;
> > +    int offset, rc;
> > +
> > +    offset = fdt_path_offset(fdt, "/hypervisor");
> > +    if (offset < 0)
> > +        return offset;
> > +
> > +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
> > +    if (rc)
> > +        return rc;
> > +
> > +    if (info.gpaddr_bits > 64)
> > +        return ERROR_INVAL;
> > +
> > +    /*
> > +     * Try to allocate separate 2MB-aligned extended regions from the first
> > +     * and second RAM banks taking into the account the maximum supported
> > +     * guest physical address space size and the amount of memory assigned
> > +     * to the guest.
> > +     */
> > +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> > +        region_base[i] = bankbase[i] +
> > +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] <<
> > XC_PAGE_SHIFT);
> > +
> > +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> > +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> > +        if (bankend[i] > region_base[i])
> > +            region_size[i] = bankend[i] - region_base[i] + 1;
> > +    }
> > +
> > +    /*
> > +     * The region 0 for grant table space must be always present. If we
> > managed
> > +     * to allocate the extended regions then insert them as regions 1...N.
> > +     */
> > +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> > +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> > +
> > +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> > +        if (region_size[i] < EXT_REGION_MIN_SIZE)
> > +            continue;
> > +
> > +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> > +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> > +
> > +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> > +                  region_base[i], region_size[i]);
> > +        nr_regions++;
> > +    }
> > +
> > +    if (!nr_regions)
> > +        LOG(WARN, "The extended regions cannot be allocated, not enough
> > space");
> > +
> > +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS +
> > GUEST_ROOT_SIZE_CELLS) *
> > +        (nr_regions + 1);
> > +
> > +    return fdt_setprop(fdt, offset, "reg", regs, len);
> > +}
> > +
> >  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >                                                 uint32_t domid,
> > libxl_domain_config *d_config,
> >                                                 struct xc_dom_image *dom)
> >  {
> >      void *fdt = dom->devicetree_blob;
> > -    int i;
> > +    int i, res;
> >      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > 
> >      const struct xc_dom_seg *ramdisk = dom->modules[0].blob ?
> >          &dom->modules[0].seg : NULL;
> > 
> >      if (ramdisk) {
> > -        int chosen, res;
> > +        int chosen;
> >          uint64_t val;
> > 
> >          /* Neither the fdt_path_offset() nor either of the
> > @@ -1109,6 +1201,10 @@ int
> > libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> > 
> >      }
> > 
> > +    res = finalize_hypervisor_node(gc, dom);
> > +    if (res)
> > +        return res;
> > +
> >      for (i = 0; i < GUEST_RAM_BANKS; i++) {
> >          const uint64_t size = (uint64_t)dom->rambank_size[i] <<
> > XC_PAGE_SHIFT;
> > 
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index d46c61f..96ead3d 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
> > 
> >  #define GUEST_RAM_BANKS   2
> > 
> > +/*
> > + * The way to find the extended regions (to be exposed to the guest as
> > unused
> > + * address space) relies on the fact that the regions reserved for the RAM
> > + * below are big enough to also accommodate such regions.
> > + */
> >  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB
> > */
> >  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
> > 
> > (END)
> > 
> > 
> -- 
> Regards,
> 
> Oleksandr Tyshchenko
>