On Tue, Jun 04, 2024 at 11:42:25 -0400, Collin Walling wrote:
The QEMU portion is designed for s390x such that there is a static
list
of hardcoded feature bits that are flagged for deprecation. This list
can be updated in follow-up patches as more features need to be flagged.
Good, a single static list should definitely be easier to handle.
The query-cpu-model-expansion *response* has been modified to now
include an /optional/ "deprecated props" string array (no additional
parameter is required in the query JSON). This array will be present if
and only if there is data to be reported. If an architecture does not
support reporting deprecated features, or if there are no deprecated
features to report, then array is simply omitted from the response.
The deprecated features are filtered against the full-expansion features
for the particular CPU model. In the example for a z14, we would expect
all four currently deprecated features reported: bpb, csske, cte, and
te, since those features are part of the z14 full expansion. On an
older model, say a z900, you'd only see bpb because the others are not
present in the full-expansion.
In that case I think it would be nice if QEMU provided a way of getting
the list itself without any filtering. Mainly to make sure we're
providing a completely list for users which may decide to avoid using
such features in their non-host-model CPU definitions. And I can imagine
management layers might want to get a complete list too.
>> Another conflict is setting this option to "on"
would have no
>> effect on the CPU model (I can't think of a reason why someone
>> would want to explicitly enable these features). This may not
>> communicate well to the user.
>
> I think have a separate configuration is better as it does not limit the
> used to just a single CPU model. But a nested element with a text node
> looks strange. An optional attribute for the <cpu> element would be
> better.
>> For host-model the expected behavior would be to either keep or
> drop deprecated features from the CPU definition. When combined with
> custom CPU mode where such feature may be already present I can imagine
> three different options. Either keep deprecated features (that is do
> nothing, just like we do now), drop such features silently, or fail to
> start a domain in case the definition uses a deprecated feature.
I fear the last option may break some things?
The idea is the behavior would be controllable via a special value in
that attribute, which a used would have to explicitly specify to get
such behavior. In other words, a user would provide a custom CPU
definition and ask for deprecated features to be removed no matter what
they are currently. But I don't think we need to care about it now
because...
> We could even create the attribute, but limit it only to
host-model,
> which would be mostly equivalent to your "host-recomended" mode, but
> extensible to other modes in the future.
I think this is the better approach.
Agreed. Just do the minimal required thing while making it extensible.
In tandem to your suggestion to add a new attribute to the cpu
element,
a quick mock-up of it could look like this:
<cpu mode='host-model' deprecated_features='off'>
Right. Although you may have fun coming with an attribute name, because
we are not very consistent with naming multi-word attributes. Some
people don't like underscores while others don't like camelCase :-)
Anyway, I think you should at least be consistent and use a single name
for the attribute in domain XML and the element in domcapabilities XML.
>> To report these features, a <deprecatedProperties> tag
could be
>> added to the domcapabilities output using the same format I use in
>> my proposed patch for the qemu capabilities file:
>>
>> <cpu>
>> <mode name='host-passthrough' supported='yes'>
>> ...
>> </mode>
>> ...
>> <mode name='host-model' supported='yes'>
>> ...
>> </mode>
>> <deprecatedProperties>
>> <property name='bpb'/>
>> <property name='te'/>
>> <property name='cte'/>
>> <property name='csske'/>
>> </deprecatedProperties>
>> </cpu>
>
> We should stick with "features" rather than calling them
"properties"
> here to avoid confusion. Also this schema would mean the list of
> deprecated features is indeed the same for all CPU models, which does
> not seem to be the case here.
Noted to use "features" instead of "properties".
And you are correct that this makes the assumption that these features
are available for *all* CPU models -- I did not think of this
perspective. This is populated from a query-cpu-model-expansion based
on "host-model".
What would be a better way to report these features in the
domcapabilities response? Perhaps they should only be nested under the
host-model?
Well, the list is static and common to all CPU models so keeping it as
another element under <cpu> as you suggested looks fine.
...another thought would be to implement a new feature policy
"deprecated" that would allow these XML features to be applied to any
CPU model. This would flag the feature to either be disabled if it's
available for that model, or ignore it otherwise. The validity of the
feature would be checked against the list of deprecated features (to
avoid typos or having a user define an imaginary feature).
Additionally, the feature policy would evaluate to "disabled" for a live
guest XML to alleviate migration issues on machines that do not know
about this new policy.
Consider the following for domcapabilities output:
<cpu>
<mode name='host-passthrough' supported='yes'>
...
</mode>
...
<mode name='host-model' supported='yes'>
...
</mode>
<deprecatedFeatures>
<feature policy='deprecated' name='bpb'/>
<feature policy='deprecated' name='te'/>
<feature policy='deprecated' name='cte'/>
<feature policy='deprecated' name='csske'/>
</deprecatedProperties>
</cpu>
I think this is a little bit too complicated.
The benefit here is that this would let a user easily copy+paste
these
features into their guest XML and not have to consider whether or not
these features are available for the CPU model they wish to use.
They should always be able to mark all deprecated features as disabled
if they want. I mean a feature not used by a specific CPU model is
disabled and disabling it explicitly in the XML does not change
anything. Or is s390x different and using a specific CPU model would
also limit what extra features you can list even if you just disable
them?
Alternatively, the <mode name='host-model' ...> could
include the
aforementioned deprecated_features='off' attribute and the nested
features would include the analogous features with policy='disabled'.
This provides the benefit of having a near ready-to-use CPU model XML
with deprecated features that can be provided to the baseline input.
Introducing a new value for an attribute would cause parse failures when
the XML is passed to an older libvirt. I think we should just take
host-model CPUs from all hosts, compute the baseline and drop deprecated
features from the result. This would of course need a new flag for the
baseline API. This way we could also take host-model from a single host
and let deprecated features be removed from it. Internally either the
baseline QMP command would also provide a list of deprecated features or
we could just take the result and let QEMU expand it to get the list.
The way I imagined this working was to either use the
"host-recommended"
CPU model by default (but I believe we are nix'ing that approach), OR
just use the host-model as-is today and rely on the input XML containing
the appropriate list of deprecated features (with policy=disabled). The
input XML containing this CPU model could be extracted from a live guest
that has deprecated features disabled.
Or we could perhaps add a flag for domcapabilities API to show the
host-model with deprecated features already disabled. This way we could
avoid introducing a new policy while letting the originating host limit
the disabled features itself. Or we could do both... limit the input and
also removed the deprecated features from the result :-)
To echo part of the discussion from Daniel's feedback, he seems
in favor
of providing an explicit indication on the cpu definition that
deprecated features are on|off. I think this works in tandem with a
deprecated_features attribute in the <cpu> element?
Yeah, I think that's exactly what the attribute can do.
Jirka