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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, January 10, 2023 2:23 AM
> 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 Penny,

Hi Julien,

> 
> 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
> >. ;\
> 
> 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.
> 

Yes, If I use the latter "struct shm_info", keeping the shm memory info out of 
the bootinfo,
I think we could avoid "bigger than 2MB" error.

Hmm, out of curiosity, The way to create "distinct" structure is like creating 
another section
for these distinct structures in lds, just like the existing .dtb section?
 
> >
> >>> 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.
>

Ah, so you are suggesting we should extract the domain shared memory 
information only
when dealing with the information from the host device-tree, something like 
this:
struct shm_info {
      char shm_id[MAX_SHM_ID_LENGTH];
      unsigned int nr_shm_borrowers;
}

I'll try and may *bother* you when encountering any problem~ 
 
> Cheers,
> 
> --
> Julien Grall