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

[Julien Grall <julien@xxxxxxx>]

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?

Cheers,

--
Julien Grall