Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 09/01/2023 11:58, Penny Zheng wrote:
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Monday, January 9, 2023 6:58 PM
> > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> > <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> > when host address not provided
> >
> >
> >
> > On 09/01/2023 07:49, Penny Zheng wrote:
> > > Hi Julien
> > Hi Penny,
> >
> > > Happy new year~~~~
> > Happy new year too!
> >
> > > > -----Original Message-----
> > > > From: Julien Grall <julien@xxxxxxx>
> > > > Sent: Sunday, January 8, 2023 8:53 PM
> > > > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-
> > devel@xxxxxxxxxxxxxxxxxxxx
> > > > Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> > > > <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> > > > <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> > > > <Volodymyr_Babchuk@xxxxxxxx>
> > > > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> > > > when host address not provided
> > > >
> > > > Hi,
> > > A few concerns explained why I didn't choose "struct meminfo" over two
> > > pointers "struct membank*" and "struct meminfo*".
> > > 1) memory usage is the main reason.
> > > If we use "struct meminfo" over the current "struct membank*" and
> > > "struct meminfo*", "struct shm_meminfo" will become a array of 256
> > > "struct shm_membank", with "struct shm_membank" being also an 256-
> > item
> > > array, that is 256 * 256, too big for a structure and If I remembered clearly,
> > it will lead to "more than PAGE_SIZE" compiling error.
> >
> > I am not aware of any place where we would restrict the size of kinfo in
> > upstream. Can you give me a pointer?
> If I remembered correctly, my first version of "struct shm_meminfo" is this
> "big"(256 * 256) structure, and it leads to the whole xen binary is bigger than 
> 2MB. ;\
Ah so the problem is because shm_mem is used in bootinfo. Then I think 
we should create a distinct structure when dealing with domain information.
> > > FWIT, either reworking meminfo or using a different structure, are
> > > both leading to sizing down the array, hmmm, I don't know which size
> > > is suitable. That's why I prefer pointer and dynamic allocation.
> > I would expect that in most cases, you will need only one bank when the host
> > address is not provided. So I think this is a bit odd to me to impose a "large"
> > allocation for them.
> Only if user is not defining size as something like (2^a + 2^b + 2^c + ...). ;\
> So maybe 8 or 16 is enough?
> struct new_meminfo {
"new" is a bit strange. The name would want to be changed. Or maybe 
better the structure been defined within the next structure and anonymized.
>      unsigned int nr_banks;
>      struct membank bank[8];
> };
>
> Correct me if I'm wrong:
> The "struct shm_membank" you are suggesting is looking like this, right?
> struct shm_membank {
>      char shm_id[MAX_SHM_ID_LENGTH];
>      unsigned int nr_shm_borrowers;
>      struct new_meminfo shm_banks;
>      unsigned long total_size;
> };
AFAIU, shm_membank would still be used to get the information from the 
host device-tree. If so, then I am afraid this is not an option to me 
because it would make the code to reserve memory more complex.
Instead, we should create a separate structure that will only be used 
for domain shared memory information.
Cheers,

--
Julien Grall