[libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
Martin Kletzander
mkletzan at redhat.com
Thu May 31 08:19:52 UTC 2018
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 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.
>
>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 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/20180531/adcdcca2/attachment-0001.sig>
More information about the libvir-list
mailing list