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

[Oleksandr <olekstysh@gmail.com>]

On 13.10.21 19:24, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien



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

...

libxl_arm.c:366:12: error: conflicting types for 'fdt_property_placeholder'
   366 | static int fdt_property_placeholder(libxl__gc *gc, void *fdt,
       |            ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from libxl_libfdt_compat.h:64,
                  from libxl_arm.c:3:

...


>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e0039..df59a0521412 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -269,6 +269,20 @@ 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)];
> +
> +    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 = 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.
>
> This is a bit unfortunate. I would prefer if this can be avoided for 
> new code (the more libxl__arch_domain_finalise_hw_description() can 
> already propagate the error).
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko