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

[Julien Grall <julien@xxxxxxx>]

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?
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.
Cheers,

--
Julien Grall