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

> > 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 {
    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;
};
Let me try~

> > 2) If we use "struct meminfo*" over the current "struct membank*" and
> > "struct meminfo*", we will need a new flag to differentiate two
> > scenarios(host address provided or not), As the special case "struct
> membank*" is already helping us differentiate.
> > And since when host address is provided, the related "struct membank"
> > also needs to be reserved in "bootinfo.reserved_mem". That's why I
> > used pointer " struct membank*" to avoid memory waste.
> 
> I am confused... Today you are defining as:
> 
> struct {
>      struct membank *;
>      struct {
>         struct meminfo *;
>         unsigned long total_size;
>      }
> }
> 
> So on arm64 host, you would use 24 bytes. If we were using an union instead.
> We would use 16 bytes. Adding a 32-bit fields, would bring to
> 20 bytes (assuming we can re-use a padding).
> 
> Therefore, there is no memory waste here with a flag...
> 
> Cheers,
> 
> --
> Julien Grall