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
