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

[Oleksandr <olekstysh@gmail.com>]

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.


>
>
>>       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()?

ok, I don't mind.


>
>
>> +{
>> +    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.

I looked at the similar finalise_*(), and it looks like no one bothers 
with returning an error. Of course, this is not an excuse, will add a 
proper check.


>
>
>> +
>> +    /*
>> +     * 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.

ok, will propagate, it looks like an upper layer 
libxl__arch_domain_finalise_hw_description() also needs to propagate it.


>
>
>> +}
>> +
>>   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

ok, will do.


-- 
Regards,

Oleksandr Tyshchenko