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

[Oleksandr <olekstysh@gmail.com>]

Hi Stefano


On 13.10.21 23:24, Stefano Stabellini wrote:
> 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

Thank you for confirming. I have just pushed V7:

https://lore.kernel.org/xen-devel/1634211645-26912-1-git-send-email-olekstysh@gmail.com/


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

-- 
Regards,

Oleksandr Tyshchenko