1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/5] Add support for extended TSEG
  4. View patch

[libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase

Martin Kletzander posted 5 patches 6 years, 11 months ago
[libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
Posted by Martin Kletzander 6 years, 11 months ago 
We are still hoping all of such checks will be moved there and this is one small
step in that direction.

One of the things that this is improving is the error message you get when
starting a domain with SMM and i440fx, for example.  Instead of saying that the
QEMU binary doesn't support that option, we correctly say that it is only
supported with q35 machine type.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/qemu/qemu_capabilities.c | 21 +++++++++++++++------
 src/qemu/qemu_capabilities.h |  4 ++--
 src/qemu/qemu_command.c      | 12 ++----------
 src/qemu/qemu_domain.c       | 12 +++++++++---
 4 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bface72de272..ebe35573e7cd 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4561,14 +4561,23 @@ virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
 }
 
 
-bool
-virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps,
-                       const virDomainDef *def)
+int
+virQEMUCapsCheckSMMSupport(virQEMUCapsPtr qemuCaps,
+                           const virDomainDef *def)
 {
-    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT))
-        return false;
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("smm is not available with this QEMU binary"));
+        return -1;
+    }
 
-    return qemuDomainIsQ35(def);
+    if (!qemuDomainIsQ35(def)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("smm is only supported with q35 machine type"));
+        return -1;
+    }
+
+    return 0;
 }
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6f9953478a4e..8b6c0c89f4f5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -497,8 +497,8 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
 bool virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps,
                                const virDomainDef *def);
 
-bool virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps,
-                            const virDomainDef *def);
+int virQEMUCapsCheckSMMSupport(virQEMUCapsPtr qemuCaps,
+                               const virDomainDef *def);
 
 char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps);
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9da2d609e8b7..328f3c0a2386 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7041,16 +7041,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
                           virTristateSwitchTypeToString(vmport));
     }
 
-    if (smm) {
-        if (!virQEMUCapsSupportsSMM(qemuCaps, def)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("smm is not available with this QEMU binary"));
-            goto cleanup;
-        }
-
-        virBufferAsprintf(&buf, ",smm=%s",
-                          virTristateSwitchTypeToString(smm));
-    }
+    if (smm)
+        virBufferAsprintf(&buf, ",smm=%s", virTristateSwitchTypeToString(smm));
 
     if (def->mem.dump_core) {
         virBufferAsprintf(&buf, ",dump-guest-core=%s",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d3beee5d8760..881d0ea46a75 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def)
 
 
 static int
-qemuDomainDefValidateFeatures(const virDomainDef *def)
+qemuDomainDefValidateFeatures(const virDomainDef *def,
+                              virQEMUCapsPtr qemuCaps)
 {
     size_t i;
 
@@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
             }
             break;
 
+        case VIR_DOMAIN_FEATURE_SMM:
+            if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+                virQEMUCapsCheckSMMSupport(qemuCaps, def) < 0)
+                return -1;
+            break;
+
         case VIR_DOMAIN_FEATURE_ACPI:
         case VIR_DOMAIN_FEATURE_APIC:
         case VIR_DOMAIN_FEATURE_PAE:
@@ -3489,7 +3496,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
         case VIR_DOMAIN_FEATURE_CAPABILITIES:
         case VIR_DOMAIN_FEATURE_PMU:
         case VIR_DOMAIN_FEATURE_VMPORT:
-        case VIR_DOMAIN_FEATURE_SMM:
         case VIR_DOMAIN_FEATURE_VMCOREINFO:
         case VIR_DOMAIN_FEATURE_LAST:
             break;
@@ -3612,7 +3618,7 @@ qemuDomainDefValidate(const virDomainDef *def,
         }
     }
 
-    if (qemuDomainDefValidateFeatures(def) < 0)
+    if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0)
         goto cleanup;
 
     ret = 0;
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
Posted by John Ferlan 6 years, 11 months ago 

On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> We are still hoping all of such checks will be moved there and this is one small
> step in that direction.
> 
> One of the things that this is improving is the error message you get when
> starting a domain with SMM and i440fx, for example.  Instead of saying that the
> QEMU binary doesn't support that option, we correctly say that it is only
> supported with q35 machine type.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 21 +++++++++++++++------
>  src/qemu/qemu_capabilities.h |  4 ++--
>  src/qemu/qemu_command.c      | 12 ++----------
>  src/qemu/qemu_domain.c       | 12 +++++++++---
>  4 files changed, 28 insertions(+), 21 deletions(-)
> 

I know it's outside the bounds of what you're doing; however,
qemuDomainDefValidateFeatures could check the capabilities for other
bits too...

[...]

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d3beee5d8760..881d0ea46a75 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def)
>  
>  
>  static int
> -qemuDomainDefValidateFeatures(const virDomainDef *def)
> +qemuDomainDefValidateFeatures(const virDomainDef *def,
> +                              virQEMUCapsPtr qemuCaps)
>  {
>      size_t i;
>  
> @@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>              }
>              break;
>  
> +        case VIR_DOMAIN_FEATURE_SMM:
> +            if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&

Probably should change to != _ABSENT, since qemu_command will supply
smm={on|off}

Reviewed-by: John Ferlan <jferlan@redhat.com>

John


> +                virQEMUCapsCheckSMMSupport(qemuCaps, def) < 0)
> +                return -1;
> +            break;
> +
>          case VIR_DOMAIN_FEATURE_ACPI:
>          case VIR_DOMAIN_FEATURE_APIC:
>          case VIR_DOMAIN_FEATURE_PAE:
> @@ -3489,7 +3496,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>          case VIR_DOMAIN_FEATURE_CAPABILITIES:
>          case VIR_DOMAIN_FEATURE_PMU:
>          case VIR_DOMAIN_FEATURE_VMPORT:
> -        case VIR_DOMAIN_FEATURE_SMM:
>          case VIR_DOMAIN_FEATURE_VMCOREINFO:
>          case VIR_DOMAIN_FEATURE_LAST:
>              break;
> @@ -3612,7 +3618,7 @@ qemuDomainDefValidate(const virDomainDef *def,
>          }
>      }
>  
> -    if (qemuDomainDefValidateFeatures(def) < 0)
> +    if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0)
>          goto cleanup;
>  
>      ret = 0;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
Posted by Martin Kletzander 6 years, 11 months ago 
On Wed, May 30, 2018 at 11:02:59AM -0400, John Ferlan wrote:
>
>
>On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>> We are still hoping all of such checks will be moved there and this is one small
>> step in that direction.
>>
>> One of the things that this is improving is the error message you get when
>> starting a domain with SMM and i440fx, for example.  Instead of saying that the
>> QEMU binary doesn't support that option, we correctly say that it is only
>> supported with q35 machine type.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_capabilities.c | 21 +++++++++++++++------
>>  src/qemu/qemu_capabilities.h |  4 ++--
>>  src/qemu/qemu_command.c      | 12 ++----------
>>  src/qemu/qemu_domain.c       | 12 +++++++++---
>>  4 files changed, 28 insertions(+), 21 deletions(-)
>>
>
>I know it's outside the bounds of what you're doing; however,
>qemuDomainDefValidateFeatures could check the capabilities for other
>bits too...
>

Probably, but I mostly wanted to do that because SMM is not only about the
capability, but also about the machine.  Good idea for the future, though.

>[...]
>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index d3beee5d8760..881d0ea46a75 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def)
>>
>>
>>  static int
>> -qemuDomainDefValidateFeatures(const virDomainDef *def)
>> +qemuDomainDefValidateFeatures(const virDomainDef *def,
>> +                              virQEMUCapsPtr qemuCaps)
>>  {
>>      size_t i;
>>
>> @@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>>              }
>>              break;
>>
>> +        case VIR_DOMAIN_FEATURE_SMM:
>> +            if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
>
>Probably should change to != _ABSENT, since qemu_command will supply
>smm={on|off}
>

That makes sense, kind of.  For 'off' we only need to check if we can specify
the smm= option.  The thing is that you can even specify smm=on with non-q35
machine type, but it is unclear what that's going to mean since it doesn't
really make sense.

@Laszlo: What would you say?  Should we allow users to specify smm=on for users?
Or even better, does it makes sense to allow specifying smm=anything for non-q35
machine types?  If not, we'll leave it like this, that is smm=anything is
forbidden for non-q35 machine types.

>Reviewed-by: John Ferlan <jferlan@redhat.com>
>
>John
>
>
>> +                virQEMUCapsCheckSMMSupport(qemuCaps, def) < 0)
>> +                return -1;
>> +            break;
>> +
>>          case VIR_DOMAIN_FEATURE_ACPI:
>>          case VIR_DOMAIN_FEATURE_APIC:
>>          case VIR_DOMAIN_FEATURE_PAE:
>> @@ -3489,7 +3496,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>>          case VIR_DOMAIN_FEATURE_CAPABILITIES:
>>          case VIR_DOMAIN_FEATURE_PMU:
>>          case VIR_DOMAIN_FEATURE_VMPORT:
>> -        case VIR_DOMAIN_FEATURE_SMM:
>>          case VIR_DOMAIN_FEATURE_VMCOREINFO:
>>          case VIR_DOMAIN_FEATURE_LAST:
>>              break;
>> @@ -3612,7 +3618,7 @@ qemuDomainDefValidate(const virDomainDef *def,
>>          }
>>      }
>>
>> -    if (qemuDomainDefValidateFeatures(def) < 0)
>> +    if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0)
>>          goto cleanup;
>>
>>      ret = 0;
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.