Re: [PATCH V2 3/3] libxl/arm: Add handling of extended regions for DomU

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Stefano

[snip]



> > > +static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image 
> > > *dom)
> > > +{
> > > +    void *fdt = dom->devicetree_blob;
> > > +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + 
> > > GUEST_ROOT_SIZE_CELLS) * 2];
> > > +    be32 *cells = &regs[0];
> > > +    uint64_t region_size = 0, region_base, bank1end_align, 
> > > bank1end_max;
> > > +    uint32_t gpaddr_bits;
> > > +    libxl_physinfo info;
> > > +    int offset, rc;
> > > +
> > > +    offset = fdt_path_offset(fdt, "/hypervisor");
> > > +    assert(offset > 0);
> > > +
> > > +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> > > +        LOG(WARN, "The extended region is only supported for 64-bit 
> > > guest");
> > > +        goto out;
> > > +    }
> > > +
> > > +    rc = libxl_get_physinfo(CTX, &info);
> > > +    assert(!rc);
> > > +
> > > +    gpaddr_bits = info.gpaddr_bits;
> > > +    assert(gpaddr_bits >= 32 && gpaddr_bits <= 48);
> > > +
> > > +    /*
> > > +     * Try to allocate single 1GB-aligned extended region from the 
> > > second RAM
> > > +     * bank (above 4GB) taking into the account the maximum 
> > > supported guest
> > > +     * address space size and the amount of memory assigned to the 
> > > guest.
> > > +     * The maximum size of the region is 128GB.
> > > +     */
> > > +    bank1end_max = min(1ULL << gpaddr_bits, GUEST_RAM1_BASE + 
> > > GUEST_RAM1_SIZE);
> > > +    bank1end_align = GUEST_RAM1_BASE +
> > > +        ALIGN_UP_TO_GB((uint64_t)dom->rambank_size[1] << 
> > > XC_PAGE_SHIFT);
> > > +
> > > +    if (bank1end_max <= bank1end_align) {
> > > +        LOG(WARN, "The extended region cannot be allocated, not 
> > > enough space");
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (bank1end_max - bank1end_align > EXT_REGION_SIZE) {
> > > +        region_base = bank1end_max - EXT_REGION_SIZE;
> > > +        region_size = EXT_REGION_SIZE;
> > > +    } else {
> > > +        region_base = bank1end_align;
> > > +        region_size = bank1end_max - bank1end_align;
> > > +    }
> > > +
> > > +out:
> > > +    /*
> > > +     * The first region for grant table space must be always present.
> > > +     * If we managed to allocate the extended region then insert it as
> > > +     * a second region.
> > > +     * TODO If we failed to allocate the region, we end up inserting
> > > +     * zero-sized region. This is because we don't know in advance 
> > > when
> > > +     * creating hypervisor node whether it would be possible to 
> > > allocate
> > > +     * a region, but we have to create a placeholder anyway. The 
> > > Linux driver
> > > +     * is able to deal with by checking the region size. We cannot 
> > > choose
> > > +     * a region when creating hypervisor node because the guest 
> > > memory layout
> > > +     * is not know at that moment (and dom->rambank_size[1] is empty).
> > > +     * We need to find a way not to expose invalid regions.
> > > +     */
> > This is not great -- it would be barely spec compliant.
> Absolutely agree.
>
>
> > When make_hypervisor_node is called we know the max memory of the guest
> > as build_info.max_memkb should be populate, right?
> Right. Just a small change to pass build_info to 
> make_hypervisor_node() is needed.
>
> > If so, we could at least detect whether we can have an extended region
> > (if not caculate the exact start address) from make_hypervisor_node.
> >
> > total_guest_memory = build_info.max_memkb * 1024;
> > rambank1_approx = total_guest_memory - GUEST_RAM0_SIZE;
> > extended_region_size = GUEST_RAM1_SIZE - rambank1_approx;
> >
> > if (extended_region_size >= MIN_EXT_REGION_SIZE)
> >      allocate_ext_region
> Good point! I will recheck that. I would prefer avoid spreading 
> extended region handling (introduce finalise_ext_region())
> and do everything from the make_hypervisor_node().
I experimented with that, so we can indeed calculate the address and 
size of extended region from make_hypervisor_node(),
there is no need for finalise_ext_region(). So, there won't be any 
zero-sized region anymore (due to unpopulated placeholder) if we fail to 
allocate extended region.
Thanks for the idea!

[snip]


--
Regards,

Oleksandr Tyshchenko