Hi Julien,
> On 13 Nov 2024, at 14:01, Julien Grall <jul...@xen.org> 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
