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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

>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. The error message could be:

Size of "memory" property doesn't match up with the sum-up of
"xen,static-mem". Unsupported configuration.