RE: [PATCH v5 7/7] xen/arm: introduce allocate_static_memory

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien and Stefano

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Friday, September 3, 2021 3:42 PM
> To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH v5 7/7] xen/arm: introduce allocate_static_memory
> 
> 
> 
> On 03/09/2021 01:39, Stefano Stabellini wrote:
> > On Thu, 2 Sep 2021, Julien Grall wrote:
> >>>> +    kinfo->mem.nr_banks = ++gbank;
> >>>> +    kinfo->unassigned_mem -= tot_size;
> >>>> +    if ( kinfo->unassigned_mem )
> >>>> +    {
> >>>> +        printk(XENLOG_ERR
> >>>> +               "Size of \"memory\" property doesn't match up with
> >>>> + the
> >>>> sum-up of \"xen,static-mem\".\n");
> >>>> +        goto fail;
> >>>
> >>> Do we need to make this a fatal failure?
> >>>
> >>> I am asking because unassigned_mem comes from the "memory" property
> >>> of the domain in device tree which is kind of redundant with the
> >>> introduction of xen,static-mem. In fact, I think it would be better
> >>> to clarify the role of "memory" when "xen,static-mem" is also present.
> >>> In that case, we could even make "memory" optional.
> >>
> >> I requested to make it mandatory. Imagine you have a domU that has
> >> 1GB and now you want to switch to static memory. If we make the
> >> property optional, then there is a risk for the admin to not
> >> correctly pass the amount of memory. This may become unnoticed until
> late.
> >>
> >> So I think making it mandatory is cheap for us and an easy way to
> >> confirm you properly sized the region. It also has the benefits to
> >> easily find out how much memory you gave to the guest (but that's just
> because I am lazy :)).
> >>
> >>> In any case, even if we don't make "memory" optional, it might still
> >>> be good to turn this error into a warning and ignore the remaining
> >>> kinfo->unassigned_mem.
> >>
> >> The behavior is matching the existing function and I think this is a
> >> good idea. If you ask 10MB of memory and we only give you 9MB, then
> >> at some point your guest is not going to be happy.
> >>
> >> It is much better to know it in advance with a failure over
> >> discovering hours later when you see an OOM from your domain.
> >
> > OK, I didn't realize this was discussed already. Let's not revisit
> > this then.
> >
> > My preference is primarily to make the device tree easier to write,
> > but nowadays nobody I know is writing the device tree by hand anymore
> > (they all use ImageBuilder).So if the device tree is generated then we
> > are fine either way as long as the binding is clear. So I am OK to
> > drop my suggestion of making "memory" optional. Let's think of a way
> > to make "memory" and "xen,static-mem" coexist instead.
> >
> >
> > There are two sides of the issue:
> > - the Xen implementation
> > - the binding
> >
> >
> > The Xen implementation is fine to panic if memory != xen,static-mem.
> > In that regard, the current patch is OK.
> >
> >
> >  From the binding perspective, I think it would be good to add a
> > statement to clarify. The binding doesn't necessarily need to match
> > exactly the implementation as the binding should be as future proof
> > and as flexible as possible.
> 
> So I agree that the binding doesn't have to match the implementation.
> But... the binding always have be more restrictive than the implementation.
> Otherwise, someone following the binding may be not able to use it with Xen.
> 
> >
> >  From the binding perspective two properties should mean different
> > things: memory the total memory amount and xen,static-mem the static
> > memory. If one day we'll have more types of memory, memory will cover
> > the total amount while xen,static-mem will cover a subset. So memory
> > must be greater or equal to xen,static-mem (even if today Xen only
> > supports memory == xen,static-mem).
> >
> > So I would add:
> >
> > """
> > As the memory property represents the total memory of the domain,
> > hence the amount of memory selected by the memory property must be
> > greater or equal to the total amount specified by xen,static-mem.
> > Other configurations (memory amount less than xen,static-mem amount)
> > are invalid.
> > """
> >
> > This sentence has the purpose of clarifying that "memory" still need
> > to be populated and have a valid value. Then, it is OK for Xen to
> > error out if memory doesn't match xen,static-mem because that's the
> > only configuration supported.
> How about writing something like "The property 'memory' should match the
> amount of memory given to the guest. Currently, it is only possible to either
> allocate static memory or let Xen chose. *Mixing* is not supported'.
> 
> Then if we add the mixing, we could say 'From Xen XX.YY, mixing will be
> allowed'.
> 

Ok. I'll add the statement to clarify the binding. 

> Cheers,
> 
> --
> Julien Grall