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

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

[Laszlo Ersek]

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.

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;
>>>

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

[Martin Kletzander]

[-- 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 --]

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

[Paolo Bonzini]

On 31/05/2018 10:19, Martin Kletzander wrote:
>> 
> 
> 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=.

That sounds like the right thing, thanks!

Paolo