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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 10/01/2023 03:38, Penny Zheng wrote:
> > -----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?
No. I meant defining a new structure (i.e. struct {}) that would be used 
in kernel_info. So you don't grow the one used by bootinfo.
>   
> > > > > 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 am not entirely sure what you are suggesting. So I will wait for the 
code to understand.
Cheers,

--
Julien Grall