Transient S390 configurations require using QEMU to compute CPU Model
Baseline and to do CPU Feature Expansion.

Start and use a single QEMU instance to do both the baseline and
expansion transactions required by BaselineHypervisorCPU.

CPU Feature Expansion uses true / false to indicate if property is/isn't
included in model. Baseline only returns property list where all
enumerated properties are included.
---
 src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a35e04a85..6c6107f077 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
     virArch arch;
     virDomainVirtType virttype;
     virDomainCapsCPUModelsPtr cpuModels;
-    bool migratable;
+    bool migratable_only;
     virCPUDefPtr cpu = NULL;
     char *cpustr = NULL;
     char **features = NULL;
+    virQEMUCapsInitQMPCommandPtr cmd = NULL;
+    bool forceTCG = false;
+    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
 
     virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
                   VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
@@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
     if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
         goto cleanup;
 
-    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
-
     if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
         goto cleanup;
 
@@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
     if (!qemuCaps)
         goto cleanup;
 
+    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
+     * migratable_only == true:  ask for and include only migratable features
+     * migratable_only == false: ask for and include all features
+     */
+    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
+
+    if (ARCH_IS_S390(arch)) {
+       /* QEMU for S390 arch only enumerates migratable features
+        * No reason to explicitly ask QEMU for or include non-migratable features
+        */
+       migratable_only = true;
+    }
+
     if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
         cpuModels->nmodels == 0) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
 
     if (ARCH_IS_X86(arch)) {
         int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
-                                           migratable, &features);
+                                           migratable_only, &features);
         if (rc < 0)
             goto cleanup;
         if (features && rc == 0) {
             /* We got only migratable features from QEMU if we asked for them,
              * no further filtering in virCPUBaseline is desired. */
-            migratable = false;
+            migratable_only = false;
         }
 
         if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
-                                   (const char **)features, migratable)))
+                                   (const char **)features, migratable_only)))
             goto cleanup;
+    } else if (ARCH_IS_S390(arch)) {
+
+       const char *binary = virQEMUCapsGetBinary(qemuCaps);
+       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+
+       if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
+                                                      cfg->user, cfg->group,
+                                                      forceTCG)))
+          goto cleanup;
+
+       if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)
+          goto cleanup; /* Content Error */
+
     } else {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
                        _("computing baseline hypervisor CPU is not supported "
@@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
 
     cpu->fallback = VIR_CPU_FALLBACK_FORBID;
 
-    if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
-        virCPUExpandFeatures(arch, cpu) < 0)
-        goto cleanup;
+    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
+       if (ARCH_IS_X86(arch)) {
+          if (virCPUExpandFeatures(arch, cpu) < 0)
+             goto cleanup;
+       } else if (ARCH_IS_S390(arch)) {
+
+          if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu)))
+             goto cleanup;
+
+          virCPUDefFree(cpu); /* Null on failure, repopulated on success */
+
+          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) {
+             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                            _("Feature Expansion not supported with this QEMU binary"));
+             goto cleanup;
+          }
+
+          if (qemuMonitorGetCPUModelExpansion(cmd->mon,
+                                              QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL,
+                                              migratable_only, modelInfo) < 0)
+             goto cleanup;
+
+          /* Expansion enumerates all features
+           * Baseline reply enumerates only in-model (true) features */
+          qemuMonitorCPUModelInfoRemovePropByBoolValue(modelInfo, false);
+
+          if (!(cpu = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, modelInfo)))
+             goto cleanup;
+       }
+    }
 
     cpustr = virCPUDefFormat(cpu, NULL);
 
@@ -13469,6 +13523,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
     virCPUDefFree(cpu);
     virObjectUnref(qemuCaps);
     virStringListFree(features);
+    virQEMUCapsInitQMPCommandFree(cmd);
+    qemuMonitorCPUModelInfoFree(modelInfo);
 
     return cpustr;
 }
-- 
2.17.1

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

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
> Transient S390 configurations require using QEMU to compute CPU Model
> Baseline and to do CPU Feature Expansion.
> 
> Start and use a single QEMU instance to do both the baseline and
> expansion transactions required by BaselineHypervisorCPU.
> 
> CPU Feature Expansion uses true / false to indicate if property is/isn't
> included in model. Baseline only returns property list where all
> enumerated properties are included.

So are you saying on s390 there's no chance there would be a CPU model
with some feature which is included in the CPU model disabled for some
reason? Sounds too good to be true :-) (This is the question I referred
to in one of my replies to the other patches.)

Most of the code you added in this patch is indented by three spaces
while we use four spaces in libvirt.

> ---
>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 65 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9a35e04a85..6c6107f077 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>      virArch arch;
>      virDomainVirtType virttype;
>      virDomainCapsCPUModelsPtr cpuModels;
> -    bool migratable;
> +    bool migratable_only;

Why? The bool says the user specified
VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
CPU back. What does the "_only" part mean? This API does not return
several CPUs, it only returns a single one and it's either migratable or
not.

>      virCPUDefPtr cpu = NULL;
>      char *cpustr = NULL;
>      char **features = NULL;
> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
> +    bool forceTCG = false;
> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>  
>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>          goto cleanup;
>  
> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> -
>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>          goto cleanup;
>  
> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>      if (!qemuCaps)
>          goto cleanup;
>  
> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
> +     * migratable_only == true:  ask for and include only migratable features
> +     * migratable_only == false: ask for and include all features
> +     */
> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> +
> +    if (ARCH_IS_S390(arch)) {
> +       /* QEMU for S390 arch only enumerates migratable features
> +        * No reason to explicitly ask QEMU for or include non-migratable features
> +        */
> +       migratable_only = true;
> +    }
> +

And what if they come up with some features which are not migratable in
the future? I don't think there's any reason for this API to mess with
the value. The code should just provide the same CPU in both cases for
s390.

>      if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
>          cpuModels->nmodels == 0) {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  
>      if (ARCH_IS_X86(arch)) {
>          int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
> -                                           migratable, &features);
> +                                           migratable_only, &features);
>          if (rc < 0)
>              goto cleanup;
>          if (features && rc == 0) {
>              /* We got only migratable features from QEMU if we asked for them,
>               * no further filtering in virCPUBaseline is desired. */
> -            migratable = false;
> +            migratable_only = false;
>          }
>  
>          if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
> -                                   (const char **)features, migratable)))
> +                                   (const char **)features, migratable_only)))
>              goto cleanup;
> +    } else if (ARCH_IS_S390(arch)) {
> +

No need for this extra empty line.

> +       const char *binary = virQEMUCapsGetBinary(qemuCaps);
> +       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> +       if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
> +                                                      cfg->user, cfg->group,
> +                                                      forceTCG)))
> +          goto cleanup;
> +
> +       if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)

Hmm, I think you should only call this when the user passed more than
one CPU. This API is expected to work even with a single CPU in which
case it just expands it if the corresponding flag was set.

> +          goto cleanup; /* Content Error */

What did you want to say with the comment? I think you can safely drop
it.

> +

And this one either.

>      } else {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                         _("computing baseline hypervisor CPU is not supported "
> @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  
>      cpu->fallback = VIR_CPU_FALLBACK_FORBID;
>  
> -    if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
> -        virCPUExpandFeatures(arch, cpu) < 0)
> -        goto cleanup;
> +    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
> +       if (ARCH_IS_X86(arch)) {
> +          if (virCPUExpandFeatures(arch, cpu) < 0)
> +             goto cleanup;
> +       } else if (ARCH_IS_S390(arch)) {
> +

Extra empty line.

> +          if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu)))
> +             goto cleanup;
> +
> +          virCPUDefFree(cpu); /* Null on failure, repopulated on success */

I think it would be better to drop the comment and just do it:

             cpu = NULL;

Oh and since this goes from CPUDef to modelInfo and back, you may
actually need to do some extra work to persist some parts of the
original CPU definitions which get lost during the translation (e.g.,
the CPU vendor) if it's applicable for s390. We have to do similar stuff
for x86 too when we translate CPUDef into CPUID bits and back.

> +
> +          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) {
> +             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                            _("Feature Expansion not supported with this QEMU binary"));
> +             goto cleanup;
> +          }

Is this necessary? Can't qemuMonitorGetCPUModelExpansion just report the
error when it tries to call an unsupported QMP command? Or is the error
message confusing?

> +
> +          if (qemuMonitorGetCPUModelExpansion(cmd->mon,
> +                                              QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL,
> +                                              migratable_only, modelInfo) < 0)
> +             goto cleanup;
> +
> +          /* Expansion enumerates all features
> +           * Baseline reply enumerates only in-model (true) features */
> +          qemuMonitorCPUModelInfoRemovePropByBoolValue(modelInfo, false);
> +
> +          if (!(cpu = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, modelInfo)))
> +             goto cleanup;
> +       }
> +    }
>  
>      cpustr = virCPUDefFormat(cpu, NULL);
>  
> @@ -13469,6 +13523,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>      virCPUDefFree(cpu);
>      virObjectUnref(qemuCaps);
>      virStringListFree(features);
> +    virQEMUCapsInitQMPCommandFree(cmd);
> +    qemuMonitorCPUModelInfoFree(modelInfo);
>  
>      return cpustr;
>  }

Jirka

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

On 13.07.2018 18:00, Jiri Denemark wrote:
> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>> Transient S390 configurations require using QEMU to compute CPU Model
>> Baseline and to do CPU Feature Expansion.
>>
>> Start and use a single QEMU instance to do both the baseline and
>> expansion transactions required by BaselineHypervisorCPU.
>>
>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>> included in model. Baseline only returns property list where all
>> enumerated properties are included.
> 
> So are you saying on s390 there's no chance there would be a CPU model
> with some feature which is included in the CPU model disabled for some
> reason? Sounds too good to be true :-) (This is the question I referred
> to in one of my replies to the other patches.)

Giving some background information: When we expand/baseline CPU models,
we always expand them to the "-base" variants of our CPU models, which
contain some set of features we expect to be around in all sane
configurations ("minimal feature set").

It is very unlikely that we ever have something like
"z14-base,featx=off", but it could happen
 - When using an emulator (TCG)
 - When running nested and the guest hypervisor is started with such a
   strange CPU model
 - When something in the HW is very wrong or eventually removed in the
   future (unlikely but possible)

On some very weird inputs to a baseline request, such a strange model
can also be the result. But it is very unusual.

I assume something like "baseline z14-base,featx=off with z14-base" will
result in "z14-base,featx=off", too.


> 
> Most of the code you added in this patch is indented by three spaces
> while we use four spaces in libvirt.
> 
>> ---
>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9a35e04a85..6c6107f077 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>      virArch arch;
>>      virDomainVirtType virttype;
>>      virDomainCapsCPUModelsPtr cpuModels;
>> -    bool migratable;
>> +    bool migratable_only;
> 
> Why? The bool says the user specified
> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
> CPU back. What does the "_only" part mean? This API does not return
> several CPUs, it only returns a single one and it's either migratable or
> not.
> 
>>      virCPUDefPtr cpu = NULL;
>>      char *cpustr = NULL;
>>      char **features = NULL;
>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
>> +    bool forceTCG = false;
>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>>  
>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>>          goto cleanup;
>>  
>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>> -
>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>>          goto cleanup;
>>  
>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>      if (!qemuCaps)
>>          goto cleanup;
>>  
>> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
>> +     * migratable_only == true:  ask for and include only migratable features
>> +     * migratable_only == false: ask for and include all features
>> +     */
>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>> +
>> +    if (ARCH_IS_S390(arch)) {
>> +       /* QEMU for S390 arch only enumerates migratable features
>> +        * No reason to explicitly ask QEMU for or include non-migratable features
>> +        */
>> +       migratable_only = true;
>> +    }
>> +
> 
> And what if they come up with some features which are not migratable in
> the future? I don't think there's any reason for this API to mess with
> the value. The code should just provide the same CPU in both cases for
> s390.

s390x usually only provides features if they are migratable. Could it
happen it the future that we have something that cannot be migrated?
Possible but very very unlikely. We cared about migration (even for
nested support) right from the beginning. As of now, we have no features
that are supported by QEMU that cannot be migrated - in contrast to e.g.
x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
and we only allow to do so if migration is in place for it.

> 
>>      if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
>>          cpuModels->nmodels == 0) {
>>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>  
>>      if (ARCH_IS_X86(arch)) {
>>          int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
>> -                                           migratable, &features);
>> +                                           migratable_only, &features);
>>          if (rc < 0)
>>              goto cleanup;
>>          if (features && rc == 0) {
>>              /* We got only migratable features from QEMU if we asked for them,
>>               * no further filtering in virCPUBaseline is desired. */
>> -            migratable = false;
>> +            migratable_only = false;
>>          }
>>  
>>          if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
>> -                                   (const char **)features, migratable)))
>> +                                   (const char **)features, migratable_only)))
>>              goto cleanup;
>> +    } else if (ARCH_IS_S390(arch)) {
>> +
> 
> No need for this extra empty line.
> 
>> +       const char *binary = virQEMUCapsGetBinary(qemuCaps);
>> +       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +
>> +       if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
>> +                                                      cfg->user, cfg->group,
>> +                                                      forceTCG)))
>> +          goto cleanup;
>> +
>> +       if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)
> 
> Hmm, I think you should only call this when the user passed more than
> one CPU. This API is expected to work even with a single CPU in which
> case it just expands it if the corresponding flag was set.

The QEMU side will bail out if only one model is passed, so this sounds
like the right thing to do.

> 
>> +          goto cleanup; /* Content Error */
> 
> What did you want to say with the comment? I think you can safely drop
> it.
> 
>> +
> 
> And this one either.
> 
>>      } else {
>>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>                         _("computing baseline hypervisor CPU is not supported "
>> @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>  
>>      cpu->fallback = VIR_CPU_FALLBACK_FORBID;
>>  
>> -    if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
>> -        virCPUExpandFeatures(arch, cpu) < 0)
>> -        goto cleanup;
>> +    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
>> +       if (ARCH_IS_X86(arch)) {
>> +          if (virCPUExpandFeatures(arch, cpu) < 0)
>> +             goto cleanup;
>> +       } else if (ARCH_IS_S390(arch)) {
>> +
> 
> Extra empty line.
> 
>> +          if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu)))
>> +             goto cleanup;
>> +
>> +          virCPUDefFree(cpu); /* Null on failure, repopulated on success */
> 
> I think it would be better to drop the comment and just do it:
> 
>              cpu = NULL;
> 
> Oh and since this goes from CPUDef to modelInfo and back, you may
> actually need to do some extra work to persist some parts of the
> original CPU definitions which get lost during the translation (e.g.,
> the CPU vendor) if it's applicable for s390. We have to do similar stuff
> for x86 too when we translate CPUDef into CPUID bits and back.

My assumption is that there are not such things (e.g. like vendor).

-- 

Thanks,

David / dhildenb

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

On 07/17/2018 05:01 PM, David Hildenbrand wrote:
> On 13.07.2018 18:00, Jiri Denemark wrote:
>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>> Transient S390 configurations require using QEMU to compute CPU Model
>>> Baseline and to do CPU Feature Expansion.
>>>
>>> Start and use a single QEMU instance to do both the baseline and
>>> expansion transactions required by BaselineHypervisorCPU.
>>>
>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>> included in model. Baseline only returns property list where all
>>> enumerated properties are included.
>>
>> So are you saying on s390 there's no chance there would be a CPU model
>> with some feature which is included in the CPU model disabled for some
>> reason? Sounds too good to be true :-) (This is the question I referred
>> to in one of my replies to the other patches.)
> 
> Giving some background information: When we expand/baseline CPU models,
> we always expand them to the "-base" variants of our CPU models, which
> contain some set of features we expect to be around in all sane
> configurations ("minimal feature set").
> 
> It is very unlikely that we ever have something like
> "z14-base,featx=off", but it could happen
>  - When using an emulator (TCG)
>  - When running nested and the guest hypervisor is started with such a
>    strange CPU model
>  - When something in the HW is very wrong or eventually removed in the
>    future (unlikely but possible)
> 
> On some very weird inputs to a baseline request, such a strange model
> can also be the result. But it is very unusual.
> 
> I assume something like "baseline z14-base,featx=off with z14-base" will
> result in "z14-base,featx=off", too.
> 
> 

That's correct. A CPU model with a feature disabled that is baseline with a CPU 
model with that same feature enabled will omit that feature in the QMP response.

e.g. if z14-base has features x, y, z then

"baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"

Since baseline will just report a base cpu and its minimal feature set... I wonder if it
makes sense for libvirt to just report the features resulting from the baseline command 
instead of later calling expansion?

>>
>> Most of the code you added in this patch is indented by three spaces
>> while we use four spaces in libvirt.
>>
>>> ---
>>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 9a35e04a85..6c6107f077 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>      virArch arch;
>>>      virDomainVirtType virttype;
>>>      virDomainCapsCPUModelsPtr cpuModels;
>>> -    bool migratable;
>>> +    bool migratable_only;
>>
>> Why? The bool says the user specified
>> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
>> CPU back. What does the "_only" part mean? This API does not return
>> several CPUs, it only returns a single one and it's either migratable or
>> not.
>>
>>>      virCPUDefPtr cpu = NULL;
>>>      char *cpustr = NULL;
>>>      char **features = NULL;
>>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
>>> +    bool forceTCG = false;
>>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>>>  
>>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
>>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>>>          goto cleanup;
>>>  
>>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>> -
>>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>>>          goto cleanup;
>>>  
>>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>      if (!qemuCaps)
>>>          goto cleanup;
>>>  
>>> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
>>> +     * migratable_only == true:  ask for and include only migratable features
>>> +     * migratable_only == false: ask for and include all features
>>> +     */
>>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>> +
>>> +    if (ARCH_IS_S390(arch)) {
>>> +       /* QEMU for S390 arch only enumerates migratable features
>>> +        * No reason to explicitly ask QEMU for or include non-migratable features
>>> +        */
>>> +       migratable_only = true;
>>> +    }
>>> +
>>
>> And what if they come up with some features which are not migratable in
>> the future? I don't think there's any reason for this API to mess with
>> the value. The code should just provide the same CPU in both cases for
>> s390.
> 
> s390x usually only provides features if they are migratable. Could it
> happen it the future that we have something that cannot be migrated?
> Possible but very very unlikely. We cared about migration (even for
> nested support) right from the beginning. As of now, we have no features
> that are supported by QEMU that cannot be migrated - in contrast to e.g.
> x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
> and we only allow to do so if migration is in place for it.
> 

You're a gold mine on this kind of information.  I may have to pester you about some 
CPU model related stuff in the future :)

>>
>>>      if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
>>>          cpuModels->nmodels == 0) {
>>>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>  
>>>      if (ARCH_IS_X86(arch)) {
>>>          int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
>>> -                                           migratable, &features);
>>> +                                           migratable_only, &features);
>>>          if (rc < 0)
>>>              goto cleanup;
>>>          if (features && rc == 0) {
>>>              /* We got only migratable features from QEMU if we asked for them,
>>>               * no further filtering in virCPUBaseline is desired. */
>>> -            migratable = false;
>>> +            migratable_only = false;
>>>          }
>>>  
>>>          if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
>>> -                                   (const char **)features, migratable)))
>>> +                                   (const char **)features, migratable_only)))
>>>              goto cleanup;
>>> +    } else if (ARCH_IS_S390(arch)) {
>>> +
>>
>> No need for this extra empty line.
>>
>>> +       const char *binary = virQEMUCapsGetBinary(qemuCaps);
>>> +       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>> +
>>> +       if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
>>> +                                                      cfg->user, cfg->group,
>>> +                                                      forceTCG)))
>>> +          goto cleanup;
>>> +
>>> +       if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)
>>
>> Hmm, I think you should only call this when the user passed more than
>> one CPU. This API is expected to work even with a single CPU in which
>> case it just expands it if the corresponding flag was set.
> 
> The QEMU side will bail out if only one model is passed, so this sounds
> like the right thing to do.
> 
>>
>>> +          goto cleanup; /* Content Error */
>>
>> What did you want to say with the comment? I think you can safely drop
>> it.
>>
>>> +
>>
>> And this one either.
>>
>>>      } else {
>>>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>>                         _("computing baseline hypervisor CPU is not supported "
>>> @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>  
>>>      cpu->fallback = VIR_CPU_FALLBACK_FORBID;
>>>  
>>> -    if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
>>> -        virCPUExpandFeatures(arch, cpu) < 0)
>>> -        goto cleanup;
>>> +    if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
>>> +       if (ARCH_IS_X86(arch)) {
>>> +          if (virCPUExpandFeatures(arch, cpu) < 0)
>>> +             goto cleanup;
>>> +       } else if (ARCH_IS_S390(arch)) {
>>> +
>>
>> Extra empty line.
>>
>>> +          if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu)))
>>> +             goto cleanup;
>>> +
>>> +          virCPUDefFree(cpu); /* Null on failure, repopulated on success */
>>
>> I think it would be better to drop the comment and just do it:
>>
>>              cpu = NULL;
>>
>> Oh and since this goes from CPUDef to modelInfo and back, you may
>> actually need to do some extra work to persist some parts of the
>> original CPU definitions which get lost during the translation (e.g.,
>> the CPU vendor) if it's applicable for s390. We have to do similar stuff
>> for x86 too when we translate CPUDef into CPUID bits and back.
> 
> My assumption is that there are not such things (e.g. like vendor).
> 

Correct. AFAIK and from what I've seen, s390 only cares about the arch, model, and
features data with regards to cpu models and libvirt.

-- 
Respectfully,
- Collin Walling

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

On 18.07.2018 00:39, Collin Walling wrote:
> On 07/17/2018 05:01 PM, David Hildenbrand wrote:
>> On 13.07.2018 18:00, Jiri Denemark wrote:
>>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>>> Transient S390 configurations require using QEMU to compute CPU Model
>>>> Baseline and to do CPU Feature Expansion.
>>>>
>>>> Start and use a single QEMU instance to do both the baseline and
>>>> expansion transactions required by BaselineHypervisorCPU.
>>>>
>>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>>> included in model. Baseline only returns property list where all
>>>> enumerated properties are included.
>>>
>>> So are you saying on s390 there's no chance there would be a CPU model
>>> with some feature which is included in the CPU model disabled for some
>>> reason? Sounds too good to be true :-) (This is the question I referred
>>> to in one of my replies to the other patches.)
>>
>> Giving some background information: When we expand/baseline CPU models,
>> we always expand them to the "-base" variants of our CPU models, which
>> contain some set of features we expect to be around in all sane
>> configurations ("minimal feature set").
>>
>> It is very unlikely that we ever have something like
>> "z14-base,featx=off", but it could happen
>>  - When using an emulator (TCG)
>>  - When running nested and the guest hypervisor is started with such a
>>    strange CPU model
>>  - When something in the HW is very wrong or eventually removed in the
>>    future (unlikely but possible)
>>
>> On some very weird inputs to a baseline request, such a strange model
>> can also be the result. But it is very unusual.
>>
>> I assume something like "baseline z14-base,featx=off with z14-base" will
>> result in "z14-base,featx=off", too.
>>
>>
> 
> That's correct. A CPU model with a feature disabled that is baseline with a CPU 
> model with that same feature enabled will omit that feature in the QMP response.
> 
> e.g. if z14-base has features x, y, z then
> 
> "baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"

Usually we try to not chose a model with stripped off base features ("we
try to produce a model that looks sane"), but instead fallback to some
very ancient CPU model. E.g.

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
"name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }

-> {"return": {"model": {"name": "z800-base", "props": {"etf2": true,
"ldisp": true}}}}

We might want to change that behavior in the future however (or maybe it
already is like this for some corner cases) - assume some base feature
gets dropped by HW in a new CPU generation. We don't always want to
fallback to a z900 or so when baselining. So one should assume that we
can have disabled features here.

Especially as there is a BUG in QEMU I'll have to fix:

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
"name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name":
"z14"}} }

-> Segmentation fault

This would have to produce a model with esan3 disabled (very very
unlikely to ever happen in real life :) )

The result should be something like {"model": {"name": "z900-base",
"props": {"esan3": false}}} or an error that they cannot be baselined.



> 
> Since baseline will just report a base cpu and its minimal feature set... I wonder if it
> makes sense for libvirt to just report the features resulting from the baseline command 
> instead of later calling expansion?

Yes it does and the docs say:

"Baseline two CPU models, creating a compatible third model. The created
model will always be a static, migration-safe CPU model (see "static"
CPU model expansion for details)"

> 
>>>
>>> Most of the code you added in this patch is indented by three spaces
>>> while we use four spaces in libvirt.
>>>
>>>> ---
>>>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 9a35e04a85..6c6107f077 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>      virArch arch;
>>>>      virDomainVirtType virttype;
>>>>      virDomainCapsCPUModelsPtr cpuModels;
>>>> -    bool migratable;
>>>> +    bool migratable_only;
>>>
>>> Why? The bool says the user specified
>>> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
>>> CPU back. What does the "_only" part mean? This API does not return
>>> several CPUs, it only returns a single one and it's either migratable or
>>> not.
>>>
>>>>      virCPUDefPtr cpu = NULL;
>>>>      char *cpustr = NULL;
>>>>      char **features = NULL;
>>>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
>>>> +    bool forceTCG = false;
>>>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>>>>  
>>>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>>>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
>>>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>>>>          goto cleanup;
>>>>  
>>>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>> -
>>>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>>>>          goto cleanup;
>>>>  
>>>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>      if (!qemuCaps)
>>>>          goto cleanup;
>>>>  
>>>> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
>>>> +     * migratable_only == true:  ask for and include only migratable features
>>>> +     * migratable_only == false: ask for and include all features
>>>> +     */
>>>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>> +
>>>> +    if (ARCH_IS_S390(arch)) {
>>>> +       /* QEMU for S390 arch only enumerates migratable features
>>>> +        * No reason to explicitly ask QEMU for or include non-migratable features
>>>> +        */
>>>> +       migratable_only = true;
>>>> +    }
>>>> +
>>>
>>> And what if they come up with some features which are not migratable in
>>> the future? I don't think there's any reason for this API to mess with
>>> the value. The code should just provide the same CPU in both cases for
>>> s390.
>>
>> s390x usually only provides features if they are migratable. Could it
>> happen it the future that we have something that cannot be migrated?
>> Possible but very very unlikely. We cared about migration (even for
>> nested support) right from the beginning. As of now, we have no features
>> that are supported by QEMU that cannot be migrated - in contrast to e.g.
>> x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
>> and we only allow to do so if migration is in place for it.
>>
> 
> You're a gold mine on this kind of information.  I may have to pester you about some 
> CPU model related stuff in the future :)

Sure, just CC or ask me and I'm happy to help.


-- 

Thanks,

David / dhildenb

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

Quoting David Hildenbrand (2018-07-18 02:26:24)
> On 18.07.2018 00:39, Collin Walling wrote:
> > On 07/17/2018 05:01 PM, David Hildenbrand wrote:
> >> On 13.07.2018 18:00, Jiri Denemark wrote:
> >>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
> >>>> Transient S390 configurations require using QEMU to compute CPU Model
> >>>> Baseline and to do CPU Feature Expansion.
> >>>>
> >>>> Start and use a single QEMU instance to do both the baseline and
> >>>> expansion transactions required by BaselineHypervisorCPU.
> >>>>
> >>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
> >>>> included in model. Baseline only returns property list where all
> >>>> enumerated properties are included.
> >>>
> >>> So are you saying on s390 there's no chance there would be a CPU model
> >>> with some feature which is included in the CPU model disabled for some
> >>> reason? Sounds too good to be true :-) (This is the question I referred
> >>> to in one of my replies to the other patches.)
> >>
> >> Giving some background information: When we expand/baseline CPU models,
> >> we always expand them to the "-base" variants of our CPU models, which
> >> contain some set of features we expect to be around in all sane
> >> configurations ("minimal feature set").
> >>
> >> It is very unlikely that we ever have something like
> >> "z14-base,featx=off", but it could happen
> >>  - When using an emulator (TCG)
> >>  - When running nested and the guest hypervisor is started with such a
> >>    strange CPU model
> >>  - When something in the HW is very wrong or eventually removed in the
> >>    future (unlikely but possible)
> >>
> >> On some very weird inputs to a baseline request, such a strange model
> >> can also be the result. But it is very unusual.
> >>
> >> I assume something like "baseline z14-base,featx=off with z14-base" will
> >> result in "z14-base,featx=off", too.
> >>
> >>
> > 
> > That's correct. A CPU model with a feature disabled that is baseline with a CPU 
> > model with that same feature enabled will omit that feature in the QMP response.
> > 
> > e.g. if z14-base has features x, y, z then
> > 
> > "baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"

I am runing tests on both S390 and X86 (hacked QEMU to enable baseline).

I don't see a "false" property in the baseline response in any of the logs.

I did try to slip a "zpci":false into the query-cpu-model-baseline but I still 
don't get a false in the response.

Here is the request/response for reference.

{"execute":"query-cpu-model-baseline",
 "arguments":{"modela":{"name":"z14"},
              "modelb":{"name":"z13","props":{"msa5":true,"exrl":true,"zpci":false}}},
 "id":"libvirt-2"} 

{"return": {"model": {"name": "z13-base","props": {"aen": true, "aefsi": true, 
"msa5": true, "msa4": true, "msa3": true, "msa2": true, "msa1": true, "sthyi": 
true, "edat": true, "ri": true, "edat2": true, "vx": true, "ipter": true, 
"esop": true, "cte": true, "sea_esop2": true, "te": true, "cmm": true}}}, "id": 
"libvirt-2"}

 
> Usually we try to not chose a model with stripped off base features ("we
> try to produce a model that looks sane"), but instead fallback to some
> very ancient CPU model. E.g.
> 
> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
> "name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }
> 
> -> {"return": {"model": {"name": "z800-base", "props": {"etf2": true,
> "ldisp": true}}}}
> 
> We might want to change that behavior in the future however (or maybe it
> already is like this for some corner cases) - assume some base feature
> gets dropped by HW in a new CPU generation. We don't always want to
> fallback to a z900 or so when baselining. So one should assume that we
> can have disabled features here.
> 
> Especially as there is a BUG in QEMU I'll have to fix:
> 
> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
> "name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name":
> "z14"}} }
> 
> -> Segmentation fault
> 
> This would have to produce a model with esan3 disabled (very very
> unlikely to ever happen in real life :) )
> 
> The result should be something like {"model": {"name": "z900-base",
> "props": {"esan3": false}}} or an error that they cannot be baselined.
> 

Seems like were saying I should be filtering out or otherwise property excluding
any false properties that are returned. Please correct if I have this wrong.

I currently do filtering / exclusion on the result of expansion but seems like I
should be doing filtering on baseline output too if we don't do the expansion 
just in case baseline would return a false property for some reason.

> 
> > 
> > Since baseline will just report a base cpu and its minimal feature set... I wonder if it
> > makes sense for libvirt to just report the features resulting from the baseline command 
> > instead of later calling expansion?
> 
> Yes it does and the docs say:
> 
> "Baseline two CPU models, creating a compatible third model. The created
> model will always be a static, migration-safe CPU model (see "static"
> CPU model expansion for details)"
> 

Here is my understanding: 

1) query-cpu-model-baseline will only return migratable properties.

2) query-cpu-model-expansion on S390 only returns migratable properties.

In fact, here is what happens if you ask S390 for non-migratable features too:

{"execute":"query-cpu-model-expansion",
 "arguments":{"type":"static","model":{"name":"host","props":{"migratable":false}}},"id":"libvirt-45"} 

Line [{"id": "libvirt-45", "error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}]

3) There seem to be two usecases for expansion

A/ Enumrate migratable properties in the baseline model
B/ Enumerate both migratable and nonmigratable props in baseline model.
    
B/ Doesn't work on S390 but A/ does.  Here is what A/ looks like:

{"execute":"query-cpu-model-baseline",
 "arguments":{"modela":{"name":"z13-base"},"modelb":{"name":"z13-base"}},
 "id":"libvirt-4"} 

Line [{"return": {"model": {"name": "z13-base"}}, "id": "libvirt-4"}]

***

{"execute":"query-cpu-model-expansion",
 "arguments":{"type":"full","model":{"name":"z13-base"}},"id":"libvirt-5"} 

Line [{"return": {"model": {"name": "z13-base", "props": {"pfmfi": false, 
"exrl": true, "stfle45": true, "cmma": false, "dateh2": true, "aen": false, 
"gen13ptff": true, "dateh": true, "cmmnt": false, "iacc2": true, "parseh": true, 
"csst": true, "idter": false, "idtes": true, "msa": true, "aefsi": false, 
"hpma2": false, "csst2": true, "csske": true, "mepoch": false, "msa8": false, 
"msa7": false, "msa6": false, "msa5": false, "msa4": false, "msa3": false, 
"msa2": false, "msa1": false, "sthyi": false, "stckf": true, "stfle": true, 
"edat": false, "etf3": true, "etf2": true, "hfpm": true, "ri": false, "edat2": 
false, "hfpue": true, "dfp": true, "mvcos": true, "sprogp": true, "sigpif": 
false, "ldisphp": true, "vx": false, "ipter": false, "emon": true, "cei": false, 
"cmpsceh": true, "ginste": true, "dfppc": true, "dfpzc": true, "dfphp": true, 
"stfle49": true, "mepochptff": false, "opc": false, "asnlxr": true, "gpereh": 
false, "minste2": false, "vxeh": false, "vxpd": false, "esop": false, "ectg": 
true, "ib": false, "siif": false, "tsi": false, "tpei": false, "esan3": true, 
"fpe": true, "ibs": false, "zarch": true, "stfle53": true, "sief2": false, 
"eimm": true, "iep": false, "irbm": false, "srs": true, "kss": false, "cte": 
false, "ais": false, "fpseh": true, "ltlbc": true, "ldisp": true, "bpb": false, 
"64bscao": false, "ctop": false, "gs": false, "sema": false, "etf3eh": true, 
"etf2eh": true, "eec": false, "ppa15": false, "zpci": false, "nonqks": true, 
"sea_esop2": false, "pfpo": true, "te": false, "cmm": false, "tods": true, 
"plo": true, "gsls": false, "skey": false}}}, "id": "libvirt-5"}]

> > 
> >>>
> >>> Most of the code you added in this patch is indented by three spaces
> >>> while we use four spaces in libvirt.
> >>>
> >>>> ---
> >>>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
> >>>>  1 file changed, 65 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>>> index 9a35e04a85..6c6107f077 100644
> >>>> --- a/src/qemu/qemu_driver.c
> >>>> +++ b/src/qemu/qemu_driver.c
> >>>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> >>>>      virArch arch;
> >>>>      virDomainVirtType virttype;
> >>>>      virDomainCapsCPUModelsPtr cpuModels;
> >>>> -    bool migratable;
> >>>> +    bool migratable_only;
> >>>
> >>> Why? The bool says the user specified
> >>> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
> >>> CPU back. What does the "_only" part mean? This API does not return
> >>> several CPUs, it only returns a single one and it's either migratable or
> >>> not.
> >>>
> >>>>      virCPUDefPtr cpu = NULL;
> >>>>      char *cpustr = NULL;
> >>>>      char **features = NULL;
> >>>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
> >>>> +    bool forceTCG = false;
> >>>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
> >>>>  
> >>>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
> >>>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
> >>>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> >>>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
> >>>>          goto cleanup;
> >>>>  
> >>>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> >>>> -
> >>>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
> >>>>          goto cleanup;
> >>>>  
> >>>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> >>>>      if (!qemuCaps)
> >>>>          goto cleanup;
> >>>>  
> >>>> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
> >>>> +     * migratable_only == true:  ask for and include only migratable features
> >>>> +     * migratable_only == false: ask for and include all features
> >>>> +     */
> >>>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> >>>> +
> >>>> +    if (ARCH_IS_S390(arch)) {
> >>>> +       /* QEMU for S390 arch only enumerates migratable features
> >>>> +        * No reason to explicitly ask QEMU for or include non-migratable features
> >>>> +        */
> >>>> +       migratable_only = true;
> >>>> +    }
> >>>> +
> >>>
> >>> And what if they come up with some features which are not migratable in
> >>> the future? I don't think there's any reason for this API to mess with
> >>> the value. The code should just provide the same CPU in both cases for
> >>> s390.
> >>
> >> s390x usually only provides features if they are migratable. Could it
> >> happen it the future that we have something that cannot be migrated?
> >> Possible but very very unlikely. We cared about migration (even for
> >> nested support) right from the beginning. As of now, we have no features
> >> that are supported by QEMU that cannot be migrated - in contrast to e.g.
> >> x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
> >> and we only allow to do so if migration is in place for it.
> >>
A problem here is if I set migratable_only (or migratable) to false then
property "migratable":false is added to query-cpu-model-expansion.

If X86 && model "name":"host" (limted of names) you get a successfull result.

For all other archs and specific model names like (z13, IvyBridge) you get this:
Line [{"id": "libvirt-45","error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}]

So unless something changes on the QEMU side you get nothing if you try to get
query-cpu-model-expansion to enumerate non-migratable features outside the X86 +
name: host type of usecases.

> > 
> > You're a gold mine on this kind of information.  I may have to pester you about some 
> > CPU model related stuff in the future :)
> 
> Sure, just CC or ask me and I'm happy to help.
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb

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

On 07/21/2018 12:05 AM, Chris Venteicher wrote:
> Quoting David Hildenbrand (2018-07-18 02:26:24)
>> On 18.07.2018 00:39, Collin Walling wrote:
>>> On 07/17/2018 05:01 PM, David Hildenbrand wrote:
>>>> On 13.07.2018 18:00, Jiri Denemark wrote:
>>>>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>>>>> Transient S390 configurations require using QEMU to compute CPU Model
>>>>>> Baseline and to do CPU Feature Expansion.
>>>>>>
>>>>>> Start and use a single QEMU instance to do both the baseline and
>>>>>> expansion transactions required by BaselineHypervisorCPU.
>>>>>>
>>>>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>>>>> included in model. Baseline only returns property list where all
>>>>>> enumerated properties are included.
>>>>>
>>>>> So are you saying on s390 there's no chance there would be a CPU model
>>>>> with some feature which is included in the CPU model disabled for some
>>>>> reason? Sounds too good to be true :-) (This is the question I referred
>>>>> to in one of my replies to the other patches.)
>>>>
>>>> Giving some background information: When we expand/baseline CPU models,
>>>> we always expand them to the "-base" variants of our CPU models, which
>>>> contain some set of features we expect to be around in all sane
>>>> configurations ("minimal feature set").
>>>>
>>>> It is very unlikely that we ever have something like
>>>> "z14-base,featx=off", but it could happen
>>>>  - When using an emulator (TCG)
>>>>  - When running nested and the guest hypervisor is started with such a
>>>>    strange CPU model
>>>>  - When something in the HW is very wrong or eventually removed in the
>>>>    future (unlikely but possible)
>>>>
>>>> On some very weird inputs to a baseline request, such a strange model
>>>> can also be the result. But it is very unusual.
>>>>
>>>> I assume something like "baseline z14-base,featx=off with z14-base" will
>>>> result in "z14-base,featx=off", too.
>>>>
>>>>
>>>
>>> That's correct. A CPU model with a feature disabled that is baseline with a CPU 
>>> model with that same feature enabled will omit that feature in the QMP response.
>>>
>>> e.g. if z14-base has features x, y, z then
>>>
>>> "baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"
> 
> I am runing tests on both S390 and X86 (hacked QEMU to enable baseline).
> 
> I don't see a "false" property in the baseline response in any of the logs.

Right... baseline should not be returning any properties paired with false. It
constructs a third CPU model with properties that can run on both CPUs.

> 
> I did try to slip a "zpci":false into the query-cpu-model-baseline but I still 
> don't get a false in the response.
> 

Sending a property paired with "false" in the JSON object is telling QEMU "I want 
to turn off this feature." The feature will then be omitted from the QMP response.

> Here is the request/response for reference.
> 
> {"execute":"query-cpu-model-baseline",
>  "arguments":{"modela":{"name":"z14"},
>               "modelb":{"name":"z13","props":{"msa5":true,"exrl":true,"zpci":false}}},
>  "id":"libvirt-2"} 
> 
> {"return": {"model": {"name": "z13-base","props": {"aen": true, "aefsi": true, 
> "msa5": true, "msa4": true, "msa3": true, "msa2": true, "msa1": true, "sthyi": 
> true, "edat": true, "ri": true, "edat2": true, "vx": true, "ipter": true, 
> "esop": true, "cte": true, "sea_esop2": true, "te": true, "cmm": true}}}, "id": 
> "libvirt-2"}
> 
>>> Usually we try to not chose a model with stripped off base features ("we
>> try to produce a model that looks sane"), but instead fallback to some
>> very ancient CPU model. E.g.
>>
>> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
>> "name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }
>>
>> -> {"return": {"model": {"name": "z800-base", "props": {"etf2": true,
>> "ldisp": true}}}}
>>
>> We might want to change that behavior in the future however (or maybe it
>> already is like this for some corner cases) - assume some base feature
>> gets dropped by HW in a new CPU generation. We don't always want to
>> fallback to a z900 or so when baselining. So one should assume that we
>> can have disabled features here.
>>
>> Especially as there is a BUG in QEMU I'll have to fix:
>>
>> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
>> "name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name":
>> "z14"}} }
>>
>> -> Segmentation fault
>>
>> This would have to produce a model with esan3 disabled (very very
>> unlikely to ever happen in real life :) )
>>
>> The result should be something like {"model": {"name": "z900-base",
>> "props": {"esan3": false}}} or an error that they cannot be baselined.
>>
> 
> Seems like were saying I should be filtering out or otherwise property excluding
> any false properties that are returned. Please correct if I have this wrong.> 
> I currently do filtering / exclusion on the result of expansion but seems like I
> should be doing filtering on baseline output too if we don't do the expansion 
> just in case baseline would return a false property for some reason.
> 
Filtering is not necessary. All properties in baseline or a static expansion response will 
either be paired with "true" or a feature will be absent. The only occurrence of a property 
paired with "false" is during a "full" expansion, where QEMU will return ALL features supported 
by the hypervisor and weather or not that feature is supported on the specified CPU model.

Looking back at your code, you'll need to make sure the baseline JSON function captures the
props field. If the libvirt command was used with --features flag, then the captured features
should be output. Otherwise just output the model name. (Additionally, the --migratable flag 
won't make a difference for s390).

>>
>>>
>>> Since baseline will just report a base cpu and its minimal feature set... I wonder if it
>>> makes sense for libvirt to just report the features resulting from the baseline command 
>>> instead of later calling expansion?
>>
>> Yes it does and the docs say:
>>
>> "Baseline two CPU models, creating a compatible third model. The created
>> model will always be a static, migration-safe CPU model (see "static"
>> CPU model expansion for details)"
>>
> 
> Here is my understanding: 
> 
> 1) query-cpu-model-baseline will only return migratable properties.
> 

Yes.

> 2) query-cpu-model-expansion on S390 only returns migratable properties.
> 

Yup.

> In fact, here is what happens if you ask S390 for non-migratable features too:
> 
> {"execute":"query-cpu-model-expansion",
>  "arguments":{"type":"static","model":{"name":"host","props":{"migratable":false}}},"id":"libvirt-45"} 
> 
> Line [{"id": "libvirt-45", "error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}]
> 
> 3) There seem to be two usecases for expansion
> 
> A/ Enumrate migratable properties in the baseline model
> B/ Enumerate both migratable and nonmigratable props in baseline model.
>     
> B/ Doesn't work on S390 but A/ does.  Here is what A/ looks like:
> 
> {"execute":"query-cpu-model-baseline",
>  "arguments":{"modela":{"name":"z13-base"},"modelb":{"name":"z13-base"}},
>  "id":"libvirt-4"} 
> 
> Line [{"return": {"model": {"name": "z13-base"}}, "id": "libvirt-4"}]

This looks right. A "-base" model is a CPU model with a "minimum feature set,"
so it makes sense to not see any features listed here. 

> 
> ***
> 
> {"execute":"query-cpu-model-expansion",
>  "arguments":{"type":"full","model":{"name":"z13-base"}},"id":"libvirt-5"} 

AFAIU, a "full" expansion will show *all* features the hypervisor knows about and weather 
or not that feature can be enabled ("true") on the specified CPU model or not ("false"). All
features listed have migration support.

> 
> Line [{"return": {"model": {"name": "z13-base", "props": {"pfmfi": false, 
> "exrl": true, "stfle45": true, "cmma": false, "dateh2": true, "aen": false, 
> "gen13ptff": true, "dateh": true, "cmmnt": false, "iacc2": true, "parseh": true, 
> "csst": true, "idter": false, "idtes": true, "msa": true, "aefsi": false, 
> "hpma2": false, "csst2": true, "csske": true, "mepoch": false, "msa8": false, 
> "msa7": false, "msa6": false, "msa5": false, "msa4": false, "msa3": false, 
> "msa2": false, "msa1": false, "sthyi": false, "stckf": true, "stfle": true, 
> "edat": false, "etf3": true, "etf2": true, "hfpm": true, "ri": false, "edat2":
> false, "hfpue": true, "dfp": true, "mvcos": true, "sprogp": true, "sigpif": 
> false, "ldisphp": true, "vx": false, "ipter": false, "emon": true, "cei": false, 
> "cmpsceh": true, "ginste": true, "dfppc": true, "dfpzc": true, "dfphp": true, 
> "stfle49": true, "mepochptff": false, "opc": false, "asnlxr": true, "gpereh": 
> false, "minste2": false, "vxeh": false, "vxpd": false, "esop": false, "ectg": 
> true, "ib": false, "siif": false, "tsi": false, "tpei": false, "esan3": true, 
> "fpe": true, "ibs": false, "zarch": true, "stfle53": true, "sief2": false, 
> "eimm": true, "iep": false, "irbm": false, "srs": true, "kss": false, "cte": 
> false, "ais": false, "fpseh": true, "ltlbc": true, "ldisp": true, "bpb": false, 
> "64bscao": false, "ctop": false, "gs": false, "sema": false, "etf3eh": true, 
> "etf2eh": true, "eec": false, "ppa15": false, "zpci": false, "nonqks": true, 
> "sea_esop2": false, "pfpo": true, "te": false, "cmm": false, "tods": true, 
> "plo": true, "gsls": false, "skey": false}}}, "id": "libvirt-5"}]
> 
>>>
>>>>>
>>>>> Most of the code you added in this patch is indented by three spaces
>>>>> while we use four spaces in libvirt.
>>>>>
>>>>>> ---
>>>>>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>>>>>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>>> index 9a35e04a85..6c6107f077 100644
>>>>>> --- a/src/qemu/qemu_driver.c
>>>>>> +++ b/src/qemu/qemu_driver.c
>>>>>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>>>      virArch arch;
>>>>>>      virDomainVirtType virttype;
>>>>>>      virDomainCapsCPUModelsPtr cpuModels;
>>>>>> -    bool migratable;
>>>>>> +    bool migratable_only;
>>>>>
>>>>> Why? The bool says the user specified
>>>>> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
>>>>> CPU back. What does the "_only" part mean? This API does not return
>>>>> several CPUs, it only returns a single one and it's either migratable or
>>>>> not.
>>>>>
>>>>>>      virCPUDefPtr cpu = NULL;
>>>>>>      char *cpustr = NULL;
>>>>>>      char **features = NULL;
>>>>>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
>>>>>> +    bool forceTCG = false;
>>>>>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>>>>>>  
>>>>>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>>>>>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
>>>>>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>>>>>>          goto cleanup;
>>>>>>  
>>>>>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>>>> -
>>>>>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>>>>>>          goto cleanup;
>>>>>>  
>>>>>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>>>>      if (!qemuCaps)
>>>>>>          goto cleanup;
>>>>>>  
>>>>>> +    /* QEMU can enumerate non-migratable cpu model features for some archs like x86
>>>>>> +     * migratable_only == true:  ask for and include only migratable features
>>>>>> +     * migratable_only == false: ask for and include all features
>>>>>> +     */
>>>>>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>>>> +
>>>>>> +    if (ARCH_IS_S390(arch)) {
>>>>>> +       /* QEMU for S390 arch only enumerates migratable features
>>>>>> +        * No reason to explicitly ask QEMU for or include non-migratable features
>>>>>> +        */
>>>>>> +       migratable_only = true;
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> And what if they come up with some features which are not migratable in
>>>>> the future? I don't think there's any reason for this API to mess with
>>>>> the value. The code should just provide the same CPU in both cases for
>>>>> s390.
>>>>
>>>> s390x usually only provides features if they are migratable. Could it
>>>> happen it the future that we have something that cannot be migrated?
>>>> Possible but very very unlikely. We cared about migration (even for
>>>> nested support) right from the beginning. As of now, we have no features
>>>> that are supported by QEMU that cannot be migrated - in contrast to e.g.
>>>> x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
>>>> and we only allow to do so if migration is in place for it.
>>>>
> A problem here is if I set migratable_only (or migratable) to false then
> property "migratable":false is added to query-cpu-model-expansion.
> 
> If X86 && model "name":"host" (limted of names) you get a successfull result.
> 
> For all other archs and specific model names like (z13, IvyBridge) you get this:
> Line [{"id": "libvirt-45","error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}]
> 
> So unless something changes on the QEMU side you get nothing if you try to get
> query-cpu-model-expansion to enumerate non-migratable features outside the X86 +
> name: host type of usecases.
> >>>
>>> You're a gold mine on this kind of information.  I may have to pester you about some 
>>> CPU model related stuff in the future :)
>>
>> Sure, just CC or ask me and I'm happy to help.
>>
>>
>> -- 
>>
>> Thanks,
>>
>> David / dhildenb
> 


-- 
Respectfully,
- Collin Walling

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

On 23.07.2018 22:16, Collin Walling wrote:
> On 07/21/2018 12:05 AM, Chris Venteicher wrote:
>> Quoting David Hildenbrand (2018-07-18 02:26:24)
>>> On 18.07.2018 00:39, Collin Walling wrote:
>>>> On 07/17/2018 05:01 PM, David Hildenbrand wrote:
>>>>> On 13.07.2018 18:00, Jiri Denemark wrote:
>>>>>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>>>>>> Transient S390 configurations require using QEMU to compute CPU Model
>>>>>>> Baseline and to do CPU Feature Expansion.
>>>>>>>
>>>>>>> Start and use a single QEMU instance to do both the baseline and
>>>>>>> expansion transactions required by BaselineHypervisorCPU.
>>>>>>>
>>>>>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>>>>>> included in model. Baseline only returns property list where all
>>>>>>> enumerated properties are included.
>>>>>>
>>>>>> So are you saying on s390 there's no chance there would be a CPU model
>>>>>> with some feature which is included in the CPU model disabled for some
>>>>>> reason? Sounds too good to be true :-) (This is the question I referred
>>>>>> to in one of my replies to the other patches.)
>>>>>
>>>>> Giving some background information: When we expand/baseline CPU models,
>>>>> we always expand them to the "-base" variants of our CPU models, which
>>>>> contain some set of features we expect to be around in all sane
>>>>> configurations ("minimal feature set").
>>>>>
>>>>> It is very unlikely that we ever have something like
>>>>> "z14-base,featx=off", but it could happen
>>>>>  - When using an emulator (TCG)
>>>>>  - When running nested and the guest hypervisor is started with such a
>>>>>    strange CPU model
>>>>>  - When something in the HW is very wrong or eventually removed in the
>>>>>    future (unlikely but possible)
>>>>>
>>>>> On some very weird inputs to a baseline request, such a strange model
>>>>> can also be the result. But it is very unusual.
>>>>>
>>>>> I assume something like "baseline z14-base,featx=off with z14-base" will
>>>>> result in "z14-base,featx=off", too.
>>>>>
>>>>>
>>>>
>>>> That's correct. A CPU model with a feature disabled that is baseline with a CPU 
>>>> model with that same feature enabled will omit that feature in the QMP response.
>>>>
>>>> e.g. if z14-base has features x, y, z then
>>>>
>>>> "baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"
>>
>> I am runing tests on both S390 and X86 (hacked QEMU to enable baseline).
>>
>> I don't see a "false" property in the baseline response in any of the logs.
> 
> Right... baseline should not be returning any properties paired with false. It
> constructs a third CPU model with properties that can run on both CPUs.
> 

Let me rephrase: We don't return "false" for any property when
baselining as of now, but this might change in the future. It is
undocumented behavior.


-- 

Thanks,

David / dhildenb

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