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

[Martin Kletzander <mkletzan@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7160 bytes --]

On Thu, May 31, 2018 at 09:33:54AM +0200, Laszlo Ersek wrote:
>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@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.
>
>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.
>

Oh, it definitely helps.  And I always enjoy when someone explains low level
details of something like you do, it makes me feel very smart after I read it =)

...something about the shoulders of giants and stuff...

I'll fix this up according to what you described, that is smm=on/off will be
possible to be specified whenever -machine supports smm=.

>Thanks,
>Laszlo
>
>>> 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;
>>>>
>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]