Re: [PATCH v3 2/2] tools/arm: exclude iomem from domU extended regions

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

Thanks for taking a look!

On 5/26/25 07:40, Anthony PERARD wrote:
> On Tue, May 13, 2025 at 03:54:50PM -0400, Stewart Hildebrand wrote:
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 75c811053c7c..8ae16a1726fc 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, 
>> struct xc_dom_image *dom)
>>      if (info.gpaddr_bits > 64)
>>          return ERROR_INVAL;
>>  
>> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
>> +          compare_iomem);
>> +
>>      /*
>>       * 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] +
>> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; 
>> i++) {
>> +        struct {
>> +            uint64_t start;
>> +            uint64_t end; /* inclusive */
>> +        } unallocated;
>> +        uint64_t size = 0;
>> +
>> +        unallocated.start = 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;
>> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
>> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 
>> 1);
>> +
>> +        if (unallocated.end > unallocated.start)
>> +            size = unallocated.end - unallocated.start + 1;
>> +
>> +        if (size < EXT_REGION_MIN_SIZE)
>> +            continue;
>> +
>> +        /* Exclude iomem */
>> +        for (j = 0; j < b_info->num_iomem && nr_regions < 
>> MAX_NR_EXT_REGIONS;
>> +             j++) {
>> +            struct {
>> +                uint64_t start;
>> +                uint64_t end; /* inclusive */
>> +            } iomem;
>> +
>> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
>> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
>> +                         << XC_PAGE_SHIFT) - 1;
>> +
>> +            if (iomem.end >= unallocated.start
>> +                && iomem.start <= unallocated.end) {
>> +
>> +                if (iomem.start <= unallocated.start) {
>> +                    unallocated.start = iomem.end + 1;
>> +
>> +                    if (iomem.end >= unallocated.end)
>> +                        /* Complete overlap, discard unallocated region */
>> +                        break;
>> +
>> +                    /* Beginning overlap */
>> +                    continue;
> 
> Instead of a `continue` and a comment that I don't understand what it is
> supposed to mean, you could just do if-else:
> 
> if (iomem.start <= unallocated.start) {
>     // code before this continue
> } else { // we have: iomem.start > unallocated.start
>     // the block of code bellow.
> }

Yep, that makes sense, will do.

>> +                }
>> +
>> +                if (iomem.start > unallocated.start) {
>> +                    assert(unallocated.end > unallocated.start);
> 
> I think this assert should be removed.

OK

> Instead, you could check that this property hold true every time there's
> a modification to `unallocated.start` in this function.
> 
> Maybe one way to make the algo easier to read, and to check that this
> property is still true, is to rewrite:
> 
>     unallocated.start = iomem.end + 1;
>     if (iomem.end >= unallocated.end)
>         // discard `unallocated`
>         break;
> 
> with
> 
>     unallocated.start = iomem.end + 1;
>     if (unallocated.start > unallocated.end)
>         // obvious: all allocated already
>         break;
> 
> Because checking for:
>     iomem.end >= unallocated.end
> is the same as checking for:
>     iomem.end + 1 > unallocated.end
>     unallocated.start > unallocated.end

Ah, yes, this makes more sense. Will do, thanks.

>> +                    size = iomem.start - unallocated.start;
> 
> Isn't `size` the size of the unallocated region? Why is it recalculated
> with `iomem`? I think it would be better to create a new variable.

You're right, "size" is being used here for something different. I'll
create a new variable to better distinguish.

>> +
>> +                    if (size >= EXT_REGION_MIN_SIZE) {
>> +                        region_base[nr_regions] = unallocated.start;
>> +                        region_size[nr_regions] = size;
>> +                        nr_regions++;
>> +                    }
>> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, 
>> struct xc_dom_image *dom)
>>      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;
>> -
>> +    for (i = 0; i < nr_regions; i++) {
>>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
>> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
>> +            i, region_base[i], region_base[i] + region_size[i]);
> 
> Shouldn't we print "base + size - 1" for the end address?

OK