Re: [PATCH v5 04/11] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 08/12/2023 16:09, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 07/12/2023 09:38, Michal Orzel wrote:
>> Hi Penny,
>>
>> On 06/12/2023 10:06, Penny Zheng wrote:
>>>
>>>
>>> We split the code of allocate_bank_memory into two parts,
>>> allocate_domheap_memory and guest_physmap_memory.
>>>
>>> One is about allocating guest RAM from heap, which could be re-used later 
>>> for
>>> allocating static shared memory from heap when host address is not provided.
>>> The other is building up guest P2M mapping.
>>>
>>> We also define a set of MACRO helpers to access common fields in data
>>> structure of "meminfo" type, e.g. "struct meminfo" is one of them, and
>>> later new "struct shm_meminfo" is also one of them.
>>> This kind of structures must have the following characteristics:
>>> - an array of "struct membank"
>>> - a member called "nr_banks" indicating current array size
>>> - a field indicating the maximum array size
>>> When introducing a new data structure, according callbacks with function 
>>> type
>>> "retrieve_fn" shall be defined for using MACRO helpers.
>>> This commit defines callback "retrieve_meminfo" for data structure
>>> "struct meminfo".
>> This patch introduces quite a bit of complexity with all these helpers, so 
>> adding a rationale is crucial.
>> AFAIU, all of this is because we don't want to reuse struct meminfo where 
>> NR_MEM_BANKS is defined as 256,
>> which is a lot more than we need for shmem. Am I right?
> 
> +1.
> 
>>
>> I would like others to share the opinion here as well.
> 
> If possible, I'd like to reduce the footprint of the shared memory. But
> this should be balanced with the complexity of the code.
> 
> Briefly looking at the patch series, we have two structures:
> 
> struct meminfo {
>      unsigned int nr_banks;
>      struct membank bank[NR_MEM_BANKS];
> };
> 
> struct shm_meminfo {
>      unsigned int nr_banks;
>      struct membank bank[NR_SHM_BANKS];
>      paddr_t tot_size;
> };
> 
> IIUC, the logic is mostly to be able to know the maximum size of the
> array and also the number of slots already used.
> 
> So we could have the following common structure:
> 
> struct membanks {
>     unsigned int nr_banks;
>     unsigned int max_banks;
>     struct membank *banks;
> }
> 
> Then the definition for the two other structures could be:
> 
> struct meminfo {
>      struct membank holders[NR_MEM_BANKS];
> 
>      struct membanks banks;
> }
> 
> struct shm_meminfo {
>      struct membank holders[NR_SHM_BANKS];
> 
>      struct membanks banks;
> 
>      paddr_t tot_size;
> }
> 
> And then 'banks.banks' would point to the 'holders'. And 'max_banks'
> would be set to the maximum.
> 
> There might be other way to make the structure more nicer. Like (untested):
> 
> struct membanks {
>      unsigned int nr_banks;
>      unsigned int max_banks;
>      struct membank[];
> }
> 
> struct meminfo {
>      struct membanks common;
>      // We should ensure there are no padding
>      struct membank[NR_MEM_BANKS];
> }
> 
> We would then pass &meminfo.common to allocate_domainheap_memory().
> 
> With that there should be no need for extra helpers.
> 
> What do you think?
I would go for flexible array member solution which looks much nicer and as far 
as I can tell
would solve the issue with extra helpers.

The only problem is that there are quite a lot of places where we reference 
nr_banks of meminfo e.g. mem.nr_banks
which we would need to modify to mem.common.nr_banks. Maybe we could have 
*nr_banks in membanks that would point
to nr_banks in meminfo/shm_meminfo? At some point we still need to set 
common.max_banks to e.g. NR_MEM_BANKS.

~Michal