On 19.11.19 20:42, Eduardo Habkost wrote:
On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
On 19.11.19 11:36, Peter Maydell wrote:
On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <da...@redhat.com> wrote:
On 19.11.19 10:22, Peter Maydell wrote:
I don't hugely care about query-cpu-model-expansion. I
just don't want it to have bad effects on the semantics
of user-facing stuff like x- properties.
IMHO, max should really include all features (yes, also the bad
x-features on arm :) ) and we should have a way to give users the
opportunity to specify "just give me the best model independent of the
accelerator" - something like a "best" model, but I don't care about the
name.
I'm in agreement with Peter, here: if "max" is user-visible, it's
better to make it provide more usable defaults.
Agreed, unless we warn the user about the model.
How would "max includes all features" work if we have two
x- features (or even two normal features!) which are incompatible
with each other? How does it work for features which are
I assume the "max" model should at least be consistent, see below.
valid for some other CPU type but not for 'max'? The design
seems to assume a rather simplified system where every
feature is independent and can always be applied to every
CPU, which I don't think is guaranteed to be the case.
I do agree that the use case of "max" for detecting which features can be
enabled for any CPU model is quite simplified and would also not work like
this on s390x. It really means "give me the maximum/latest/greatest you
can". Some examples on s390x:
On s390x, we don't allow to enable new features for older CPU generation.
"vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
available. However, if you select a z12 (zEC12), you cannot enable "vx":
"Feature '%s' is not available for CPU model '%s', it was introduced with
later models."
Also, we have dependency checks
(target/s390x/cpu_models.c:check_consistency()), that at least warn on
inconsistencies between features (feature X requires feature Y).
We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
z13-base.
- z13-max: Maximum features that can be enabled on the current
accel/host for a z13.
- z13-best: Best features that can be enabled on the current
accel/host for a z13.
- z13-base: base feature set, independent of current
accel/host for a z13. (we do have this already on s390x)
We don't need to create new CPU types for that, do we? These
different modes could be configured by a CPU option (e.g.
"z13,features=max"[1], "z13,features=best").
I somewhat don't like such options mixed into ordinary feature
configuration if we can avoid it. It allows for things like
z13,features=max,features=best
The z13 model is migration-safe. So would be "z13,vx=off".
"z13,features=best" would no longer be migration safe.
IOW, reusing an existing model along with that feels wrong, read below.
Especially, I dislike that one config option (features=best) disables
and enables features at the same time. Read below.
If we do add new CPU options to tune feature defaults, libvirt
can start using these options on query-cpu-model-expansion, and
everybody will be happy. No need to change defaults in the "max"
CPU model in a way that makes it less usable just to make
query-cpu-model-expansion work.
[1] Probably we need to call it something else instead of
"features=max", just to avoid confusion with the "max" CPU
model. Maybe "experimental-features=on",
"recommended-features=on"?
We already do have feature groups on s390x. So these could behave like
"host/accelerator dependent" feature groups. I would prefer that over
the suggestion above.
The only downside is that z13,recommended-features=on could actually
"disable" features that are already in z13 (e.g., "vx" is part of z13,
but if it's not available in the host we would have to disable it). I
don't like these semantic. Maybe introducing new types of models that
don't have any features as default could come into play.
E.g.,
z13-best => z13-raw,recommended-features=on
z13-max => z13-raw,recommended-features=on,experimental-features=on
Maybe we can find a better name for "recommended-features" and
"experimental-features" to highlight that these are only "features
available via the accelerator in the current host"
We could also expose:
z13-full => z13-raw,all-features=on
Which would include all features theoretically valid for a model (but
even if not available).
--
Thanks,
David / dhildenb
