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

[Martin Kletzander]

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 at 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 at 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;
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180530/0e99aa10/attachment-0001.sig>