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

[Julien Grall <julien@xxxxxxx>]

On 13/10/2021 14:46, Oleksandr wrote:
> Hi Julien
Hi Oleksandr,

> 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.
>       if (res) return res;
>
>       /*
> @@ -1069,6 +1072,74 @@ 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)))
> +
> +static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image *dom)
The function is doing more than finalizing extend regions, it also 
create the grant table regs. So how about naming it: 
finalize_hypervisor_node()?
> +{
> +    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");
> +    assert(offset > 0);
> +
> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
> +    assert(!rc);
> +
> +    assert(info.gpaddr_bits <= 64);
Neither of the two should be assert(). They should be proper check so we 
don't end up with a disaster (in particularly for the former) if there 
is a problem.
> +
> +    /*
> +     * 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] < GUEST_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);
> +    rc = fdt_setprop(fdt, offset, "reg", regs, len);
> +    assert(!rc);
We should propagate the error.

> +}
> +
>   int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>                                                  uint32_t domid,
> libxl_domain_config *d_config,
> @@ -1109,6 +1180,8 @@ int 
> libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>       }
>
> +    finalise_ext_region(gc, dom);
> +
>       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..7425a78 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)
>
> @@ -451,6 +456,8 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>   #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>
> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
I would prefer if this value is not part of the public header because 
this is not a value that the hypervisor needs to know. So it is better 
to restrict it to the libxl_arm.c
Cheers,

--
Julien Grall