Changelog:

    v3
        - rebased on master

    v2
        - numerous cleanups
            - removed "policy fix function" and now properly check
                for policy == -1 in the CPUDef -> JSON parser
            - resolved some memory leaks

        - added string arg to qemuMonitorJSONParseCPUModelData for
            error message to print out proper command name

    v1
        - introduce baseline
        - split patches into small chunks
        - free'd lingering qemuMonitorCPUModelInfo pointer
        - when converting from virCPUDef -> virJSON, consider
            feature policy FORCED for enabled

___

To run these patches, execute the virsh hypervisor-cpu-compare or 
hypervisor-cpu-baseline commands and pass an XML file describing one or 
more CPU definition. You can use the definition from virsh domcapabilities 
or from a guest XML. There is no need extract it from the file and place 
it a new one, as the XML parser will look specifically for the CPU tags.

___

These patches hookup the virsh hypervisor-cpu-compare/baseline commands 
for the s390x architecture. They take an XML file describing some CPU 
definitions and passes the data to QEMU, where the actual CPU model 
comparison / baseline calculation is handled (available since QEMU 2.8.5).
These calculations are compared against / baselined with the hypervisor
CPU model, which can be observed via the virsh domcapabilities command 
for s390x.

When baselining CPU models and the user appends the --features argument 
to the command, s390x will only report back features that supersede the 
base model definition.

*NOTE* if the --features flag is intended to expand /ALL/ features
available to a CPU model (such as the huge list of features reported
by a full CPU model expansion), please let me know and I can resolve 
this.

The first patch pulls some code out of the CPU Model Expansion JSON 
function so that it can be later used for the Comparison and Baseline 
JSON functions.

The rest of the patches follow this sequence:
    - introduce JSON monitor functions
    - introduce capability and update test files
    - hook up monitor functions to virsh command

Patch 7 pulls out some code from the CPUDef XML parser to be
reused in the comparison hookup.

Thanks.

x86 and Power review by Daniel Henrique Barboza (thanks!)
Collin Walling (8):
  qemu_monitor: helper functions for CPU models
  qemu_monitor: implement query-cpu-model-baseline
  qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_BASELINE
  qemu_driver: hook up query-cpu-model-baseline
  qemu_monitor: implement query-cpu-model-comparison
  qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON
  cpu_conf: xml to cpu definition parse helper
  qemu_driver: hook up query-cpu-model-comparison

 src/conf/cpu_conf.c                              |  30 +++
 src/conf/cpu_conf.h                              |   6 +
 src/cpu/cpu.c                                    |  14 +-
 src/libvirt_private.syms                         |   1 +
 src/qemu/qemu_capabilities.c                     | 136 +++++++++++
 src/qemu/qemu_capabilities.h                     |  18 ++
 src/qemu/qemu_driver.c                           |  38 +++
 src/qemu/qemu_monitor.c                          |  44 ++++
 src/qemu/qemu_monitor.h                          |  18 ++
 src/qemu/qemu_monitor_json.c                     | 297 ++++++++++++++++++++---
 src/qemu/qemu_monitor_json.h                     |  20 ++
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml |   2 +
 tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml |   2 +
 tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml |   2 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml  |   2 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml  |   2 +
 tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml  |   2 +
 tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml  |   2 +
 18 files changed, 587 insertions(+), 49 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

First, let me apologize for such a late review. I'll try my best to
review your series earlier next time.

On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
> When baselining CPU models and the user appends the --features argument 
> to the command, s390x will only report back features that supersede the 
> base model definition.
> 
> *NOTE* if the --features flag is intended to expand /ALL/ features
> available to a CPU model (such as the huge list of features reported
> by a full CPU model expansion), please let me know and I can resolve 
> this.

I'm not sure how well this fits s390 world, but the semantics of
VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
features which are hidden behind the CPU model. That is, all feature
which you'd get when starting QEMU with just the CPU model name and no
additional features. The extra features should not be touched at all.
Specifically, removing them when the flag is not used is wrong.

To me this looks like the flag should really result in running
query-cpu-model-expansion (but likely the "static" one rather then
"full" expansion) on the baseline CPU model and reporting the enabled
features along with those already included in the baseline feature set.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 8/20/19 10:06 AM, Jiri Denemark wrote:
> First, let me apologize for such a late review. I'll try my best to
> review your series earlier next time.
> 

Your review is greatly appreciated! I haven't replied to your other
posts on this series as my comments were mostly acknowledgements rather 
than discussion pieces. I'm working through them.

> On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
>> When baselining CPU models and the user appends the --features argument 
>> to the command, s390x will only report back features that supersede the 
>> base model definition.
>>
>> *NOTE* if the --features flag is intended to expand /ALL/ features
>> available to a CPU model (such as the huge list of features reported
>> by a full CPU model expansion), please let me know and I can resolve 
>> this.
> 
> I'm not sure how well this fits s390 world, but the semantics of
> VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
> features which are hidden behind the CPU model. That is, all feature
> which you'd get when starting QEMU with just the CPU model name and no
> additional features. The extra features should not be touched at all.
> Specifically, removing them when the flag is not used is wrong.
> 
> To me this looks like the flag should really result in running
> query-cpu-model-expansion (but likely the "static" one rather then
> "full" expansion) on the baseline CPU model and reporting the enabled
> features along with those already included in the baseline feature set.
> 

Actually, query-cpu-model-baseline will return a CPU model along with a
feature set. The features returned are the same as those produced from a 
static expansion on the model.

Correct me if I am wrong here: virsh should report features *only* if the
--features flag is present. Otherwise, we only report the model name (which
can be accomplished by stripping the result of all reported features).

> Jirka
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

Thank you for your review!

-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
> On 8/20/19 10:06 AM, Jiri Denemark wrote:
> > First, let me apologize for such a late review. I'll try my best to
> > review your series earlier next time.
> > 
> 
> Your review is greatly appreciated! I haven't replied to your other
> posts on this series as my comments were mostly acknowledgements rather 
> than discussion pieces. I'm working through them.
> 
> > On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
> >> When baselining CPU models and the user appends the --features argument 
> >> to the command, s390x will only report back features that supersede the 
> >> base model definition.
> >>
> >> *NOTE* if the --features flag is intended to expand /ALL/ features
> >> available to a CPU model (such as the huge list of features reported
> >> by a full CPU model expansion), please let me know and I can resolve 
> >> this.
> > 
> > I'm not sure how well this fits s390 world, but the semantics of
> > VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
> > features which are hidden behind the CPU model. That is, all feature
> > which you'd get when starting QEMU with just the CPU model name and no
> > additional features. The extra features should not be touched at all.
> > Specifically, removing them when the flag is not used is wrong.
> > 
> > To me this looks like the flag should really result in running
> > query-cpu-model-expansion (but likely the "static" one rather then
> > "full" expansion) on the baseline CPU model and reporting the enabled
> > features along with those already included in the baseline feature set.
> > 
> 
> Actually, query-cpu-model-baseline will return a CPU model along with a
> feature set. The features returned are the same as those produced from a 
> static expansion on the model.
> 
> Correct me if I am wrong here: virsh should report features *only* if the
> --features flag is present. Otherwise, we only report the model name (which
> can be accomplished by stripping the result of all reported features).

No, this is wrong in general (although it maybe correct for s390, I
don't know).

Let me explain with some hypothetical CPU models.

Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
Model2 will enable f1, f2, f3, f4, f5.
Model3 will enable f1, f2, f3, f4, f6.

Running baseline command for [Model1, Model2, Model3] should return
Model1 with no additional features as [f1, f2, f3] are in only ones
common to all three models and they are all enabled by Model1.

However, baseline of [Model2, Model3] should return Model1 + f4. The
common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
f2, f3] only, which means we need to add f4 explicitly.

With --features, even the features enabled implicitly by a specific CPU
model should be returned. That is, the first result would be Model1 +
[f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].

In other words, stripping all features if no --features option is used
is wrong. Only features implicitly enabled by the CPU model should be
stripped.

Specifically, baseline without --features on the following CPU
definition

    <cpu mode='custom' match='exact'>
      <model>Model1</model>
      <feature name='f4' policy='require'/>
    </cpu>

should be the same definition (i.e., Model1 + f4).

The question is whether you can enabled extra features on s390 or you
can only use plain CPU models with no additional features. If additional
features are allowed, I'd guess baseline without features should return
the result of the QMP command minus the features a static expansion of
the CPU model itself would return (this is assuming f4 will be included
in the result we get from QEMU for Model1 + f4). When --features is
used, the result from QEMU should be returned.

I hope it's clear now. However, I'm not sure how CPU models work on
s390 and I'd be happy if you could explain it to me. Especially, whether
something similar to what I described can happen there.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10.09.19 17:22, Jiri Denemark wrote:
> On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
>> On 8/20/19 10:06 AM, Jiri Denemark wrote:
>>> First, let me apologize for such a late review. I'll try my best to
>>> review your series earlier next time.
>>>
>>
>> Your review is greatly appreciated! I haven't replied to your other
>> posts on this series as my comments were mostly acknowledgements rather 
>> than discussion pieces. I'm working through them.
>>
>>> On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
>>>> When baselining CPU models and the user appends the --features argument 
>>>> to the command, s390x will only report back features that supersede the 
>>>> base model definition.
>>>>
>>>> *NOTE* if the --features flag is intended to expand /ALL/ features
>>>> available to a CPU model (such as the huge list of features reported
>>>> by a full CPU model expansion), please let me know and I can resolve 
>>>> this.
>>>
>>> I'm not sure how well this fits s390 world, but the semantics of
>>> VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
>>> features which are hidden behind the CPU model. That is, all feature
>>> which you'd get when starting QEMU with just the CPU model name and no
>>> additional features. The extra features should not be touched at all.
>>> Specifically, removing them when the flag is not used is wrong.
>>>
>>> To me this looks like the flag should really result in running
>>> query-cpu-model-expansion (but likely the "static" one rather then
>>> "full" expansion) on the baseline CPU model and reporting the enabled
>>> features along with those already included in the baseline feature set.
>>>
>>
>> Actually, query-cpu-model-baseline will return a CPU model along with a
>> feature set. The features returned are the same as those produced from a 
>> static expansion on the model.
>>
>> Correct me if I am wrong here: virsh should report features *only* if the
>> --features flag is present. Otherwise, we only report the model name (which
>> can be accomplished by stripping the result of all reported features).
> 
> No, this is wrong in general (although it maybe correct for s390, I
> don't know).

David here: Not correct for s390x

> 
> Let me explain with some hypothetical CPU models.
> 
> Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
> Model2 will enable f1, f2, f3, f4, f5.
> Model3 will enable f1, f2, f3, f4, f6.
> 
> Running baseline command for [Model1, Model2, Model3] should return
> Model1 with no additional features as [f1, f2, f3] are in only ones
> common to all three models and they are all enabled by Model1.
> 
> However, baseline of [Model2, Model3] should return Model1 + f4. The
> common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
> f2, f3] only, which means we need to add f4 explicitly.
> 
> With --features, even the features enabled implicitly by a specific CPU
> model should be returned. That is, the first result would be Model1 +
> [f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].
> 
> In other words, stripping all features if no --features option is used
> is wrong. Only features implicitly enabled by the CPU model should be
> stripped.
> 
> Specifically, baseline without --features on the following CPU
> definition
> 
>     <cpu mode='custom' match='exact'>
>       <model>Model1</model>
>       <feature name='f4' policy='require'/>
>     </cpu>
> 
> should be the same definition (i.e., Model1 + f4).

s390x will fallback to base models when baselining/expanding ("minimum
feature set we expect to be around for a certain cpu generation", which
is independent of the QEMU version and will never change). But if you're
already using base models, it works as you would expect it.

E.g.

Model1 is [f1, f2, f3]
Model1-base is [f1, f2]

Model2 is [f1, f2, f3, f4]
Model2-base is [f1, f2, f3]

Baselining Model1 with Model2 would result in
Model1-base + f3

For this, QMP "query-cpu-model-baseline" can be used.

Note: When expending cpu models (e.g. host), one would already always
get base models. So there, it would work completely as you expect it.

Now, to achieve the "--features" part, simply throw the result of
"query-cpu-model-baseline" into "query-cpu-model-expansion" with
"CpuModelExpansionType" == "full"

> 
> The question is whether you can enabled extra features on s390 or you
> can only use plain CPU models with no additional features. If additional
> features are allowed, I'd guess baseline without features should return
> the result of the QMP command minus the features a static expansion of
> the CPU model itself would return (this is assuming f4 will be included
> in the result we get from QEMU for Model1 + f4). When --features is
> used, the result from QEMU should be returned.

We do have plenty of extra cpu features on s390x. *Plenty* :)

> 
> I hope it's clear now. However, I'm not sure how CPU models work on
> s390 and I'd be happy if you could explain it to me. Especially, whether
> something similar to what I described can happen there.
> 
> Jirka
> 


-- 

Thanks,

David / dhildenb

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 9/10/19 11:38 AM, David Hildenbrand wrote:
> On 10.09.19 17:22, Jiri Denemark wrote:
>> On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
>>> On 8/20/19 10:06 AM, Jiri Denemark wrote:
>>>> First, let me apologize for such a late review. I'll try my best to
>>>> review your series earlier next time.
>>>>
>>>
>>> Your review is greatly appreciated! I haven't replied to your other
>>> posts on this series as my comments were mostly acknowledgements rather
>>> than discussion pieces. I'm working through them.
>>>
>>>> On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
>>>>> When baselining CPU models and the user appends the --features argument
>>>>> to the command, s390x will only report back features that supersede the
>>>>> base model definition.
>>>>>
>>>>> *NOTE* if the --features flag is intended to expand /ALL/ features
>>>>> available to a CPU model (such as the huge list of features reported
>>>>> by a full CPU model expansion), please let me know and I can resolve
>>>>> this.
>>>>
>>>> I'm not sure how well this fits s390 world, but the semantics of
>>>> VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
>>>> features which are hidden behind the CPU model. That is, all feature
>>>> which you'd get when starting QEMU with just the CPU model name and no
>>>> additional features. The extra features should not be touched at all.
>>>> Specifically, removing them when the flag is not used is wrong.
>>>>
>>>> To me this looks like the flag should really result in running
>>>> query-cpu-model-expansion (but likely the "static" one rather then
>>>> "full" expansion) on the baseline CPU model and reporting the enabled
>>>> features along with those already included in the baseline feature set.
>>>>
>>>
>>> Actually, query-cpu-model-baseline will return a CPU model along with a
>>> feature set. The features returned are the same as those produced from a
>>> static expansion on the model.
>>>
>>> Correct me if I am wrong here: virsh should report features *only* if the
>>> --features flag is present. Otherwise, we only report the model name (which
>>> can be accomplished by stripping the result of all reported features).
>>
>> No, this is wrong in general (although it maybe correct for s390, I
>> don't know).
> 
> David here: Not correct for s390x
> 
>>
>> Let me explain with some hypothetical CPU models.
>>
>> Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
>> Model2 will enable f1, f2, f3, f4, f5.
>> Model3 will enable f1, f2, f3, f4, f6.
>>
>> Running baseline command for [Model1, Model2, Model3] should return
>> Model1 with no additional features as [f1, f2, f3] are in only ones
>> common to all three models and they are all enabled by Model1.
>>
>> However, baseline of [Model2, Model3] should return Model1 + f4. The
>> common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
>> f2, f3] only, which means we need to add f4 explicitly.
>>
>> With --features, even the features enabled implicitly by a specific CPU
>> model should be returned. That is, the first result would be Model1 +
>> [f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].
>>
>> In other words, stripping all features if no --features option is used
>> is wrong. Only features implicitly enabled by the CPU model should be
>> stripped.
>>
>> Specifically, baseline without --features on the following CPU
>> definition
>>
>>      <cpu mode='custom' match='exact'>
>>        <model>Model1</model>
>>        <feature name='f4' policy='require'/>
>>      </cpu>
>>
>> should be the same definition (i.e., Model1 + f4).
> 
> s390x will fallback to base models when baselining/expanding ("minimum
> feature set we expect to be around for a certain cpu generation", which
> is independent of the QEMU version and will never change). But if you're
> already using base models, it works as you would expect it.
> 
> E.g.
> 
> Model1 is [f1, f2, f3]
> Model1-base is [f1, f2]
> 
> Model2 is [f1, f2, f3, f4]
> Model2-base is [f1, f2, f3]
> 
> Baselining Model1 with Model2 would result in
> Model1-base + f3
> 
> For this, QMP "query-cpu-model-baseline" can be used.
> 
> Note: When expending cpu models (e.g. host), one would already always
> get base models. So there, it would work completely as you expect it.
> 
> Now, to achieve the "--features" part, simply throw the result of
> "query-cpu-model-baseline" into "query-cpu-model-expansion" with
> "CpuModelExpansionType" == "full"
> 
>>
>> The question is whether you can enabled extra features on s390 or you
>> can only use plain CPU models with no additional features. If additional
>> features are allowed, I'd guess baseline without features should return
>> the result of the QMP command minus the features a static expansion of
>> the CPU model itself would return (this is assuming f4 will be included
>> in the result we get from QEMU for Model1 + f4). When --features is
>> used, the result from QEMU should be returned.
> 
> We do have plenty of extra cpu features on s390x. *Plenty* :)
> 

Indeed, there's quite a few.

>>
>> I hope it's clear now. However, I'm not sure how CPU models work on
>> s390 and I'd be happy if you could explain it to me. Especially, whether
>> something similar to what I described can happen there.
>>
>> Jirka
>>
> 
> 

Yes, both you and David have provided an excellent explanation of what's
expected (from both perspectives). Thank you both. I'll make the
appropriate changes.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list