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

adding qemu-devel, Paolo and Gerd; comments below: On 05/30/18 23:08, Martin Kletzander wrote: > 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(a)redhat.com&gt; >>> --- >>>  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. Technically the "smm" machine type property is defined for both i440fx and q35: $ qemu-system-x86_64 -machine pc,\? 2>&1 | grep smm pc-i440fx-2.11.smm=OnOffAuto (Enable SMM (pc & q35)) $ qemu-system-x86_64 -machine q35,\? 2>&1 | grep smm pc-q35-2.11.smm=OnOffAuto (Enable SMM (pc & q35)) The "smm" property controls whether system management mode is emulated, to my knowledge. That's an x86 processor operating mode, which isn't specific to the Q35 board. What's specific to Q35 is the TSEG. The primary reason why OVMF (built with the edk2 SMM driver stack) requires Q35 is not that i440fx does not emulate SMM, it's that i440fx doesn't have a large enough SMRAM area. (The legacy SMRAM areas are too small for OVMF.) For example, SeaBIOS too includes some SMM code (optionally, if it's built like that), and that runs on i440fx just fine, because the legacy SMRAM areas are large enough for SeaBIOS's purposes, AIUI. (Now, there's at least one other reason why OVMF/SMM needs Q35: because the SMI broadcast feature too is only implemented on Q35. But that's rather a consequence of the above "primary reason", and not a stand-alone reason itself -- we restricted the SMI broadcast feature to Q35 *because* OVMF could only run on Q35. Anyhow, you need not care about SMI broadcast because it's automatically negotiated between guest fw and QEMU.) Summary: (1) For making Secure Boot actually secure in OVMF, OVMF needs to be built with -D SMM_REQUIRE, so that it include the SMM driver stack. (2) Booting such a firmware binary requires Q35, because only Q35 offers TSEG, and the legacy -- non-TSEG -- SMRAM ranges are too small even for a "basic boot" of OVMF. (3) QEMU has to be configured with "-machine smm=on"; this is already covered by libvirt via <smm state='on'/>. (4) QEMU has to be configured to restrict pflash writes to code that executes in SMM, with "-global driver=cfi.pflash01,property=secure,value=on". Libvirt covers this already with the @secure=yes attribute in <loader readonly='yes' secure='yes' type='pflash'>. (5) There are two *quality of service* features related to SMM; with both of them being Q35-only. The first feature is SMI broadcast; libvirt need not care because it's auto-negotiated. (6) The second QoS feature is *extended* TSEG (a paravirt feature on top of the standard TSEG), which lets the edk2 SMM driver stack scale to large RAM sizes and high VCPU counts. Libvirt should let the user configure the extended TSEG size, because the necessary minimum is super difficult to calculate (and one "maximum size" doesn't fit all either), and the user should be allowed to experiment with the size. Hope this helps... I realize it is annoyingly complex.

Thanks, Laszlo >> Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; >> >> 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; >>>