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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,


>>>> -        }
>>>> +
>>>> +        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.
> 
> TBH, as I replied to Michal, I don't understand why we are just focusing on 
> just one issue. It would be good to provide some rationale.

I’m not sure why some DT out there are providing memory ranges reserved in both 
resvmem and reserved_memory node, could it be
that it was needed because some firmware was reading only one of the two before 
starting linux? I’m just thinking out loud.

Is your point that we should allow instead not only exact matching but also 
partial matching?

>>>> 
>>>> +
>>>> +#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
>>     };
>> };
> 
> Anonymous union are really annoying... So effectively in 
> check_reserved_regions_overlap() we are hoping that the caller will not set 
> allow_match_type to another value than MEMBANK_NONE for static memory. This 
> is extremely fragile.
> 
> I can't think of another solution right now, but I am definitely not happy 
> with this approach.

I agree it’s not nice, but the issue is not in 
check_reserved_regions_overlap(), it is in meminfo_overlap_check() which is 
static and so confined to that module.

Anyway I’m trying to think about another solution.

> 
>> 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.
> 
> Can you remind me how much memory this is going to save?

The space issue comes from:

#define NR_MEM_BANKS 256

[…]

struct meminfo {
    struct membanks_hdr common;
    struct membank bank[NR_MEM_BANKS];
};

struct bootinfo {
    struct meminfo mem;
    /* The reserved regions are only used when booting using Device-Tree */
    struct meminfo reserved_mem;

    […]

#ifdef CONFIG_ACPI
    struct meminfo acpi;
#endif
    […]
};

So in case we remove the union and CONFIG_STATIC_SHM=y and so we allow type and 
*shmem_extra to coexist, we have 256 * 2 * sizeof(pointer) byte.
Additionaly if CONFIG_ACPI=y, we have 256 * sizeof(pointer) byte more not used 
space.

Cheers,
Luca