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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 11/12/2023 11:01, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 11/12/2023 08:31, Michal Orzel wrote:
>> 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.
> 
> Possibly yes. But it is not clear what's the problem here. Are you
> concerned about the churn? Or is it just a long name?
I am concerned about the churn. I did a grep and we have almost 100 instances 
of e.g. mem{.,->}nr_banks,
which in our solution would need to be replaced with mem{.,->}common.nr_banks. 
That said ...

> 
> At least in the case of meminfo. We could possibly replace all the use
> with a pointer to "struct membank common". This would reduce the amount
> of churn and the expression length.
this could help to limit the overall churn.

~Michal