[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
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'.
Cheers,
--
Julien Grall
|
