Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

> On 13 Nov 2024, at 14:01, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 06/11/2024 13:41, Luca Fancellu wrote:
>> There are some cases where the device tree exposes a memory range
>> in both /memreserve/ and reserved-memory node, in this case the
>> current code will stop Xen to boot since it will find that the
>> latter range is clashing with the already recorded /memreserve/
>> ranges.
>> Furthermore, u-boot lists boot modules ranges, such as ramdisk,
>> in the /memreserve/ part and even in this case this will prevent
>> Xen to boot since it will see that the module memory range that
>> it is going to add in 'add_boot_module' clashes with a /memreserve/
>> range.
>> When Xen populate the data structure that tracks the memory ranges,
>> it also adds a memory type described in 'enum membank_type', so
>> in order to fix this behavior, allow the 'check_reserved_regions_overlap'
>> function to check for exact memory range match given a specific memory
>> type; allowing reserved-memory node ranges and boot modules to have an
>> exact match with ranges from /memreserve/.
>> While there, set a type for the memory recorded during ACPI boot.
> 
> I am a bit confused which you mention only ACPI boot. Isn't the path also 
> used when booting using Device-Tree?

right, maybe this should be:

“While there, set a type for the memory recorded using meminfo_add_bank() from 
eft-boot.h."

>> 
>>  static bool __init meminfo_overlap_check(const struct membanks *mem,
>>                                           paddr_t region_start,
>> -                                         paddr_t region_size)
>> +                                         paddr_t region_size,
>> +                                         enum membank_type allow_match_type)
> 
> Looking at the callers, you only seem to pass MEMBANK_FDT_RESVMEM or 
> MEMBANK_NONE. So I wonder whether it actually make sense to introduce 
> MEMBANK_NONE. Wouldn't it be better to have a boolean indicating whether we 
> are looking for FDT_RESVMEM?

I wanted to give a more generic approach, but you are right, we could have a 
boolean like allow_match_memreserve.


> 
>>  {
>>      paddr_t bank_start = INVALID_PADDR, bank_end = 0;
>>      paddr_t region_end = region_start + region_size;
>> @@ -113,12 +114,16 @@ static bool __init meminfo_overlap_check(const struct 
>> membanks *mem,
>>          if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
>>               region_start >= bank_end )
>>              continue;
>> -        else
>> -        {
>> -            printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with 
>> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n",
>> -                   region_start, region_end, i, bank_start, bank_end);
>> -            return true;
>> -        }
>> +
>> +        if ( (allow_match_type != MEMBANK_NONE) &&
>> +             (region_start == bank_start) && (region_end == bank_end) &&
> 
> Why is this only an exact match?

Apparently what we are fixing is only a case where memreserve regions matches 
exactly modules or reserved_mem nodes.

> 
>> +             (allow_match_type == mem->bank[i].type) )
>> +            continue;
>> +
>> +        printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with 
>> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n",
>> +                region_start, region_end, i, bank_start, bank_end);
>> +        return true;
>> +
>>      }
>>        return false;
>> @@ -169,9 +174,14 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>>   * with the existing reserved memory regions defined in bootinfo.
>>   * Return true if the input physical address range is overlapping with any
>>   * existing reserved memory regions, otherwise false.
>> + * The 'allow_match_type' parameter can be used to allow exact match of a
>> + * region with another memory region already recorded, but it needs to be 
>> used
>> + * only on memory regions that allows a type (reserved_mem, acpi). For all 
>> the
>> + * other cases, passing 'MEMBANK_NONE' will disable the exact match.
>>   */
>>  bool __init check_reserved_regions_overlap(paddr_t region_start,
>> -                                           paddr_t region_size)
>> +                                           paddr_t region_size,
>> +                                           enum membank_type 
>> allow_match_type)
>>  {
>>      const struct membanks *mem_banks[] = {
>>          bootinfo_get_reserved_mem(),
>> @@ -190,8 +200,21 @@ bool __init check_reserved_regions_overlap(paddr_t 
>> region_start,
>>       * shared memory banks (when static shared memory feature is enabled)
>>       */
>>      for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
>> -        if ( meminfo_overlap_check(mem_banks[i], region_start, region_size) 
>> )
>> +    {
>> +        enum membank_type type = allow_match_type;
>> +
>> +#ifdef CONFIG_STATIC_SHM
>> +        /*
>> +         * Static shared memory banks don't have a type, so for them disable
>> +         * the exact match check.
>> +         */
> 
> This feels a bit of a hack. Why can't we had a default type like you did in 
> meminfo_add_bank()?

This is the structure:

struct membank {
    paddr_t start;
    paddr_t size;
    union {
        enum membank_type type;
#ifdef CONFIG_STATIC_SHM
        struct shmem_membank_extra *shmem_extra;
#endif
    };
};

we did that in order to save space when static shared memory is not enabled, so 
we can’t have the
type for these banks because we are already writing shmem_extra.

We could remove the union but in that case we would waste space when static 
shared memory is
enabled.

Cheers,
Luca