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

[Oleksandr <olekstysh@gmail.com>]

On 13.10.21 00:22, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien, Ian.


Julien, thank you for the detailed answer, I will analyze it tomorrow.

Ian, I think, there is no reason in providing git branch with the acks 
folded in + my minor fix for the debug message as it was discussed 
before, it sounds like there is more work to do, so it is going to be a 
new version anyway.




>
> On 12/10/2021 18:42, Oleksandr wrote:
>> On 12.10.21 19:05, Julien Grall wrote:
>>> On 11/10/2021 18:48, Oleksandr Tyshchenko wrote:
>>>> ---
>>>>   tools/libs/light/libxl_arm.c  | 76 
>>>> ++++++++++++++++++++++++++++++++++++++++---
>>>>   xen/include/public/arch-arm.h |  2 ++
>>>>   2 files changed, 73 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>> b/tools/libs/light/libxl_arm.c
>>>> index e3140a6..c0e8415 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -598,9 +598,20 @@ static int make_timer_node(libxl__gc *gc, void 
>>>> *fdt,
>>>>       return 0;
>>>>   }
>>>>   +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
>>>> +
>>>>   static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>>>> -                                const libxl_version_info *vers)
>>>> +                                const libxl_version_info *vers,
>>>> +                                const libxl_domain_build_info 
>>>> *b_info,
>>>> +                                const struct xc_dom_image *dom)
>>>>   {
>>>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
>>>> region_base[GUEST_RAM_BANKS],
>>>> +        bank1end, ramsize;
>>>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + 
>>>> GUEST_ROOT_SIZE_CELLS) *
>>>> +                  (GUEST_RAM_BANKS + 1)];
>>>> +    be32 *cells = &regs[0];
>>>> +    unsigned int i, len, nr_regions = 0;
>>>> +    libxl_dominfo info;
>>>>       int res;
>>>>       gic_interrupt intr;
>>>>   @@ -615,9 +626,64 @@ 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);
>>>> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
>>>> +        LOG(WARN, "The extended regions are only supported for 
>>>> 64-bit guest currently");
>>>> +        goto out;
>>>> +    }
>>>
>>> I understand why we want to limit to 64-bit domain for dom0. But I 
>>> am not sure this is warrant for 32-bit domain. At worse, the guest 
>>> will ignore the bank because it is not usable. So could we drop the 
>>> check?
>>
>> Yes.
>>
>>
>>>
>>>
>>>> +
>>>> +    res = libxl_domain_info(CTX, &info, dom->guest_domid);
>>>> +    if (res) return res;
>>>> +
>>>> +    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
>>> What could go wrong below if gpaddr_bits is not within this range?
>>
>> if info.gpaddr_bits is less than 64, then nothing bad, otherwise, I 
>> assume we will get shift count overflow.
>
> So I think the assert() is not suitable here because even if the 
> gpaddr_bits is provided by the hypervisor (and therefore should be 
> trusted), this is a different component so hardening the code is a 
> good practice.
>
> In this case, I would check that info.gpaddr_bits <= 64 and return an 
> error. The reason I am suggesting <= 64 and not 48 is because Arm 
> already supports 52 bits address space. Yet, I still like to avoid 
> this assumption in the code. Something like below should work:
>
> bank1end = GUEST_RAM1_BASE + GUEST_RAM1_SIZE - 1;
> bank1end = min(bank1end, ~(0ULL) >> (64 - info.gpaddr_bits);
>
>>>> +
>>>> +    /*
>>>> +     * Try to allocate separate 2MB-aligned extended regions from 
>>>> the first
>>>> +     * (below 4GB) and second (above 4GB) RAM banks taking into 
>>>> the account
>>>> +     * the maximum supported guest physical address space size and 
>>>> the amount
>>>> +     * of memory assigned to the guest.
>>>> +     * As the guest memory layout is not populated yet we cannot 
>>>> rely on
>>>> +     * dom->rambank_size[], so calculate the actual size of both 
>>>> banks using
>>>> +     * "max_memkb" value.
>>>> +     */
>>>
>>> At the moment, libxl doesn't know how libxc will allocate the 
>>> memory. We may decide in the future to have only a small amount of 
>>> memory below 4GB and then the rest above 4GB. With this approach it 
>>> would be more difficult to modify the memory layout. Instead, I 
>>> think we should create a placeholder that is updated once we know 
>>> the banks in libxl__arch_domain_finalise_hw_description.
>>
>> If I got your point correctly, this is close to how it was done from 
>> the beginning. Yes, we can create placeholder(s) here and then update 
>> them once the memory layout is populated. The problem is that we 
>> won't be able to remove the placeholder(s) if we fail to allocate 
>> region(s) for some reasons. So, we should know for sure in advance 
>> how many region(s) we will be able to allocate later on in order to 
>> create the required number of placeholders right now...  Please, look 
>> at the TODO I wrote in finalise_ext_region() [1]. Or I misread your 
>> point?
>
> You read correctly my point. However, I disagree that it is a problem 
> to remove the placeholder if we fail to allocate the amount of regions 
> expected.
>
> Looking at libfdt, I can see two ways to deal with it:
>   1) Use fdt_setprop()
>   2) Delete the property using fdt_delprop() and then recreate it with 
> fdt_appendprop()
>
> The first solution is ideal and I think can work here to downsize the 
> property. At worse, the second solution should work as the FDT blob 
> will not increase.
>
>>> We also probably want to mention in the memory layout in 
>>> public/arch-arm.h this decision as the suggested way to find 
>>> extended regions will definitely impact our decision to re-order the 
>>> memory layout or shrink some regions in the future (I have in mind 
>>> the PCI Passthrough work).
>>
>> Sorry, I couldn't parse.
>
> So this patch is relying on the fact that the regions reserved for the 
> RAM are big enough to also accommodate the extended regions.
>
> I am happy with this approach. However, I would like the approach to 
> be documented in arch-arm.h because this is the first place one would 
> look to understand the memory layout. This will be helpful if/when we 
> need to modify the guest memory layout.
>
>>
>>
>>>
>>>
>>>> +    ramsize = b_info->max_memkb * 1024;
>>>> +    if (ramsize <= GUEST_RAM0_SIZE) {
>>>> +        region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
>>>> +        region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
>>>> +        region_base[1] = GUEST_RAM1_BASE;
>>>> +    } else
>>>> +        region_base[1] = GUEST_RAM1_BASE +
>>>> +            ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
>>>> +
>>>> +    bank1end = min(1ULL << info.gpaddr_bits, GUEST_RAM1_BASE + 
>>>> GUEST_RAM1_SIZE);
>>>> +    if (bank1end > region_base[1])
>>>> +        region_size[1] = bank1end - region_base[1];
>>>
>>> It would be best to not rely on the fact that Bank on is always 
>>> below 4GB. If the code is too complex then we should look to add a 
>>> BUILD_BUG_ON() to avoid any surprise.
>>
>> Yes, I can add:
>>
>> BUILD_BUG_ON((GUEST_RAM0_BASE + GUEST_RAM0_SIZE) > GB(4));
>
> I am OK with that. But I wonder if we could simply use min(..., ) to 
> avoid the BUILD_BUG_ON().
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko