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

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

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;

On 05/21/2018 11:00 AM, Martin Kletzander wrote:

TSEG (Top of Memory Segment) is one of many regions that SMM (System Management Mode) can occupy. This one, however is special, because a) most of the SMM code lives in TSEG nowadays and b) QEMU just (well, some time ago) added support for so called 'extended' TSEG. The difference to the TSEG implemented in real q35's MCH (Memory Controller Hub) is that it can have any size from 1 MiB up to 65534 MiB in 1 MiB increments. But more about that in the QEMU patch.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Which some reader, but not this one, may be eager to find ;-) Still is there a valid range QEMU would accept? Or do we just let QEMU fail if the range is too high? I think QEMU has MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 39 +++++++++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++- src/conf/domain_conf.h | 1 + tests/genericxml2xmlindata/tseg.xml | 23 +++++++++++ tests/genericxml2xmltest.c | 2 + 6 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/tseg.xml

In the category of I hate it when that happens, git am -3 "merged" in two chunks incorrectly! Probably wouldn't have happened if I'd done this sooner! The virDomainDefFeaturesCheckABIStability hunk got merged into virDomainRedirFilterDefCheckABIStability and the tseg grammar got merged under "vmport" and just before "gic". As you can imagine the results weren't pretty ;-).

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 403b638bd4bd..39ebfe398bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1897,6 +1897,9 @@ <ioapic driver='qemu'/> <hpt resizing='required'/> <vmcoreinfo state='on'/> + <smm state='on'> + <tseg unit='MiB'>48</tseg> + </smm> </features> ...</pre>

@@ -2056,6 +2059,42 @@ <code>off</code>, default <code>on</code>) enable or disable System Management Mode. <span class="since">Since 2.1.0</span> + + Optional sub-element <code>tseg</code> can be used to specify the + amount of memory dedicated to SMM TSEG. The size can be specified as a + value of that element, optional attribute <code>unit</code> can be + used to specify the unit of the aforementioned value (defaults to + 'MiB'). +

If this is to be a true paragraph break then these paragraphs needs to be wrapped in <p> ... </p>; otherwise, this just becomes one long run on (and quite ugly IMO) paragraph.

+ This value is configurable due to the fact that the calculation cannot + be done right with the guarantee that it will work correctly. For + QEMU TSEG was disabled up to and including <code>pc-q35-2.9</code> (it + does not make sense fo any other machine type than q35).

s/fo/for/ This also appears to be a paragraph break...

+ From <code>pc-q35-2.10</code> the default value was changed to 16 MiB.

s/From/Starting with/ ??? (not required, just a though)

+ That should suffice for up to 272 VCPUs, 5 GiB guest RAM in total, no + hotplug memory range, and 32 GiB of 64-bit PCI MMIO aperture. Or for + 48 VCPUs, with 1TB of guest RAM, no hotplug DIMM range, and 32GB of + 64-bit PCI MMIO aperture. The values may also vary based on the loader + the VM is using. + + Additional size might be needed for significantly higher VCPU counts + or increased address space (that can be memory, maxMemory, 64-bit PCI + MMIO aperture size; roughly 8 MiB of TSEG per 1 TiB of address space) + which can also be rounded up.

Uh, oh, hmmm... We seem to have this (perhaps more recent) "thing" about libvirt having to supply some attribute based on some (mostly difficult to describe) algorithm that QEMU would use in order to create the "optimum" size or use for some alternate algorithm. Of course, a few libvir-list patches like that have been NACK'd because of the feeling that providing a useful algorithm for a user to "decide upon" a satisfactory attribute value cannot really be done. Off the top of my head I can come up with two: 1. Add poll-max-ns property of each iothread: https://www.redhat.com/archives/libvir-list/2017-February/msg01047.html 2. Add support for qcow2 cache (many times, but most recently): https://www.redhat.com/archives/libvir-list/2017-September/msg00553.html

+ + Due to the nature of this setting being similar to "how much RAM + should the guest have" users are advised to either consult the + documentation of the guest OS or loader (if there is any), or test + this by trial-and-error changing the value until the VM boots + successfully. Yet another guiding value for users might be the fact + that 48 MiB should be enough for pretty large guests (240 VCPUs and + 4TB guest RAM), but it is on purpose not set as default as 48 MiB of + unavailable RAM might be too much for small guests (e.g. with 512 MiB + of RAM).

and this is the exact reason why patches like this get NACKd - because trial and error should not be a 'desired' means to calculate. Even the bz referenced in patch 5 has an incredible amount of data and calculations that provide even more insight and details that are lost when we try to summarize in a libvirt meaningful patch. What it seems is really needed is an attribute that libvirt provides that tells QEMU to calculate the optimum size.

+ + See <a href="#elementsMemoryAllocation">Memory Allocation</a> + for more details about the <code>unit</code> attribute. + <span class="since">Since 4.5.0</span> (QEMU only)

haha - see you put 4.5.0 and this is the 4.4.0 release - so it was ignored until 4.5.0 was "on the clock" ;-) Ironically this is pointed out as QEMU only; however, genericxml2xmltest is used/updated. So, I personally don't mind if this attribute is added; however, I think we become hypocrites to a certain degree if patches continue to be blocked/NACKed to other subsystems that have attributes that allow certain amount of control, but don't come with exact sizing references. Still if this is pushed, then perhaps those others can use this as the means to provide a reference to other knobs added. You can have my : Reviewed-by: John Ferlan <jferlan@redhat.com> with a few adjustments above; however, another R-By should be obtained here as well as perhaps a policy change so that other similar such series could be merged... I guess I'm curious what "thoughts" others may have regarding adding this "knob" while not allowing others. John

</dd> <dt><code>ioapic</code></dt> <dd>Tune the I/O APIC. Possible values for the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 71ac3d079c32..664ec79933f0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4785,6 +4785,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <element name="tseg"> + <ref name="scaledInteger"/> + </element> + </optional> </element> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3689ac0a82ce..6a4f8243183d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19647,6 +19647,16 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(nodes); }

+ if (def->features[VIR_DOMAIN_FEATURE_SMM] == VIR_TRISTATE_SWITCH_ON && + virDomainParseScaledValue("string(./features/smm/tseg)", + "string(./features/smm/tseg/@unit)", + ctxt, + &def->tseg_size, + 1024 * 1024, /* Defaults to mebibytes */ + ULLONG_MAX, + false) < 0) + goto error; + if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0) goto error;

@@ -21741,6 +21751,23 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } }

+ /* smm */ + if (src->features[VIR_DOMAIN_FEATURE_SMM] == VIR_TRISTATE_SWITCH_ON && + src->tseg_size != dst->tseg_size) { + const char *unit_src, *unit_dst; + unsigned long long short_size_src = virFormatIntPretty(src->tseg_size, + &unit_src); + unsigned long long short_size_dst = virFormatIntPretty(dst->tseg_size, + &unit_dst); + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Size of SMM TSEG size differs: " + "source: '%llu %s', destination: '%llu %s'"), + short_size_src, unit_src, + short_size_dst, unit_dst); + return false; + } + return true; }

@@ -27059,7 +27086,6 @@ virDomainDefFormatInternal(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_VMPORT: - case VIR_DOMAIN_FEATURE_SMM: switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_LAST: case VIR_TRISTATE_SWITCH_ABSENT: @@ -27076,6 +27102,38 @@ virDomainDefFormatInternal(virDomainDefPtr def,

break;

+ case VIR_DOMAIN_FEATURE_SMM: + switch ((virTristateSwitch) def->features[i]) { + case VIR_TRISTATE_SWITCH_LAST: + case VIR_TRISTATE_SWITCH_ABSENT: + break; + + case VIR_TRISTATE_SWITCH_ON: + virBufferAddLit(buf, "<smm state='on'"); + if (!def->tseg_size) { + virBufferAddLit(buf, "/>\n"); + } else { + const char *unit; + unsigned long long short_size = virFormatIntPretty(def->tseg_size, + &unit); + + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<tseg unit='%s'>%llu</tseg>\n", + unit, short_size); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</smm>\n"); + } + + break; + + case VIR_TRISTATE_SWITCH_OFF: + virBufferAddLit(buf, "<smm state='off'/>\n"); + break; + } + + break; + case VIR_DOMAIN_FEATURE_APIC: if (def->features[i] == VIR_TRISTATE_SWITCH_ON) { virBufferAddLit(buf, "<apic"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a78fdee40c65..c230dd3d09af 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2377,6 +2377,7 @@ struct _virDomainDef { virGICVersion gic_version; char *hyperv_vendor_id; int apic_eoi; + unsigned long long tseg_size;

virDomainClockDef clock;

diff --git a/tests/genericxml2xmlindata/tseg.xml b/tests/genericxml2xmlindata/tseg.xml new file mode 100644 index 000000000000..49483f874cd4 --- /dev/null +++ b/tests/genericxml2xmlindata/tseg.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg unit='MiB'>48</tseg> + </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index d8270a6cae82..daad6e0f78d8 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -141,6 +141,8 @@ mymain(void) DO_TEST_FULL("cachetune-colliding-types", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);

+ DO_TEST("tseg"); + virObjectUnref(caps); virObjectUnref(xmlopt);

On 05/31/2018 08:14 AM, Martin Kletzander wrote:

On Wed, May 30, 2018 at 12:00:26PM -0400, John Ferlan wrote:

...

On 05/21/2018 11:00 AM, Martin Kletzander wrote:

...

TSEG (Top of Memory Segment) is one of many regions that SMM (System Management Mode) can occupy.  This one, however is special, because a) most of the SMM code lives in TSEG nowadays and b) QEMU just (well, some time ago) added support for so called 'extended' TSEG.  The difference to the TSEG implemented in real q35's MCH (Memory Controller Hub) is that it can have any size from 1 MiB up to 65534 MiB in 1 MiB increments.  But more about that in the QEMU patch.

                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Which some reader, but not this one, may be eager to find ;-)

Still is there a valid range QEMU would accept? Or do we just let QEMU fail if the range is too high?

I think QEMU has MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX

Rather than promising some value, I adjusted it so that it is no longer false, no matter what the max is there.

...

...

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---  docs/formatdomain.html.in           | 39 +++++++++++++++++++  docs/schemas/domaincommon.rng       |  5 +++  src/conf/domain_conf.c              | 60 ++++++++++++++++++++++++++++-  src/conf/domain_conf.h              |  1 +  tests/genericxml2xmlindata/tseg.xml | 23 +++++++++++  tests/genericxml2xmltest.c          |  2 +  6 files changed, 129 insertions(+), 1 deletion(-)  create mode 100644 tests/genericxml2xmlindata/tseg.xml

In the category of I hate it when that happens, git am -3 "merged" in two chunks incorrectly!  Probably wouldn't have happened if I'd done

You can enable/disable 3-way merges if you do (not) like them.

...

this sooner!  The virDomainDefFeaturesCheckABIStability hunk got merged into virDomainRedirFilterDefCheckABIStability and the tseg grammar got merged under "vmport" and just before "gic".  As you can imagine the results weren't pretty ;-).

Yeah, happened to me as well, I should've resent this, but I forgot about the merge issue and I also wanted to show that this was posted way before the freeze.  Anyway, it's pointless to force it now, I'll leave it for later (meaning "after the release").

*Lots* of things were posted after this that got reviewed and pushed before this - that just seems to be "the norm" (for whatever reason)... Not receiving timely reviews is one of those areas that really bothers me. I've been making the effort more recently to try to go in order of longest without review - sometimes a ping makes me lose order though.

Anyway, I keep my branches updated (every now and then) on my github repo [1], so if you want to check that, you always can.

[1] https://github.com/nertpinx/libvirt

OK - I've added that as a remote...

...

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 403b638bd4bd..39ebfe398bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1897,6 +1897,9 @@    <ioapic driver='qemu'/>    <hpt resizing='required'/>    <vmcoreinfo state='on'/> +  <smm state='on'> +    <tseg unit='MiB'>48</tseg> +  </smm>  </features>  ...</pre>

@@ -2056,6 +2059,42 @@            <code>off</code>, default <code>on</code>) enable or disable            System Management Mode.            <span class="since">Since 2.1.0</span> + +          Optional sub-element <code>tseg</code> can be used to specify the +          amount of memory dedicated to SMM TSEG. The size can be specified as a +          value of that element, optional attribute <code>unit</code> can be +          used to specify the unit of the aforementioned value (defaults to +          'MiB'). +

If this is to be a true paragraph break then these paragraphs needs to be wrapped in <p> ... </p>; otherwise, this just becomes one long run on (and quite ugly IMO) paragraph.

...

+          This value is configurable due to the fact that the calculation cannot +          be done right with the guarantee that it will work correctly.  For +          QEMU TSEG was disabled up to and including <code>pc-q35-2.9</code> (it +          does not make sense fo any other machine type than q35).

s/fo/for/

This also appears to be a paragraph break...

...

+          From <code>pc-q35-2.10</code> the default value was changed to 16 MiB.

s/From/Starting with/ ??? (not required, just a though)

...

+          That should suffice for up to 272 VCPUs, 5 GiB guest RAM in total, no +          hotplug memory range, and 32 GiB of 64-bit PCI MMIO aperture.  Or for +          48 VCPUs, with 1TB of guest RAM, no hotplug DIMM range, and 32GB of +          64-bit PCI MMIO aperture. The values may also vary based on the loader +          the VM is using. + +          Additional size might be needed for significantly higher VCPU counts +          or increased address space (that can be memory, maxMemory, 64-bit PCI +          MMIO aperture size; roughly 8 MiB of TSEG per 1 TiB of address space) +          which can also be rounded up.

Uh, oh, hmmm... We seem to have this (perhaps more recent) "thing" about libvirt having to supply some attribute based on some (mostly difficult to describe) algorithm that QEMU would use in order to create the "optimum" size or use for some alternate algorithm. Of course, a few libvir-list patches like that have been NACK'd because of the feeling that providing a useful algorithm for a user to "decide upon" a satisfactory attribute value cannot really be done. Off the top of my head I can come up with two:

It's kind of a different story.  Think of this as a memory size.  You cannot determine the "right" amount of memory the VM should have.  You can try to boot with X and double it until the OS installation succeeds. And hope you won't need to change it later.

...

1. Add poll-max-ns property of each iothread: https://www.redhat.com/archives/libvir-list/2017-February/msg01047.html

This is about tunables.  It might change the performance/latency of QEMU slightly, but that's about it.

and there are those that would find it useful to have (bz 1545732)... If you don't have enough memory and your VM is paging like crazy, you just add more memory. Requires a reboot. Likewise, if your VM doesn't boot you add/alter the magic TSEG value using some algorithm as described above. From a 90,000 foot customer view is there a difference? It's just a knob that the hypervisor has to allow something to be accomplished for which libvirt provides the attribute to fine tune.

...

2. Add support for qcow2 cache (many times, but most recently): https://www.redhat.com/archives/libvir-list/2017-September/msg00553.html

Similarly here, it allows setting something that can be (at least slightly) abstracted and in the worst case the performance will be slightly hindered.

This one I understand more why it would be rejected, but still providing the value allows certain things to work a whole lot better. I also know Berto has been "fine tuning" the algorithm in later QEMU releases - so that's like hitting a moving target.

Contrast this with TSEG which, in case it is set incorrectly, will prevent the machine from booting at all.  If we go to the extreme, not only can you easily try to find out the right amount to set for a particular machine, but you can even do that programmatically since when OVMF fails due to small extended TSEG size it will reboot very fast. And you can get that in form of events.  When I tried it now it even looks like you get rtc-change event when the domain doesn't reboot immediately due to small TSEG size.

Is there something that is provided that would give the "hint" to adjust the TSEG size?

I will not put it in the docs because I will not guarantee that this is the right way to go, but this is how events look for default TSEG size for a guest that needs a lot more (it has 240 possible vCPUs and 256 TiB of maximum memory, but because it starts with only 1 vCPU and 1GiB memory I can try it out easily on my machine:

 virsh # event --domain nixos --all --loop --timestamp  2018-05-31 09:42:21.060+0000: event 'lifecycle' for domain nixos: Resumed Unpaused  2018-05-31 09:42:21.066+0000: event 'lifecycle' for domain nixos: Started Booted  2018-05-31 09:42:21.514+0000: event 'reboot' for domain nixos  2018-05-31 09:42:21.964+0000: event 'reboot' for domain nixos  2018-05-31 09:42:22.414+0000: event 'reboot' for domain nixos  2018-05-31 09:42:22.868+0000: event 'reboot' for domain nixos  2018-05-31 09:42:23.325+0000: event 'reboot' for domain nixos  2018-05-31 09:42:23.778+0000: event 'reboot' for domain nixos  2018-05-31 09:42:24.230+0000: event 'reboot' for domain nixos  2018-05-31 09:42:24.681+0000: event 'reboot' for domain nixos  ...

you get the point.

And this is how it looks when I start it with increased size:

 virsh # event --domain nixos --all --loop --timestamp  2018-05-31 09:43:24.578+0000: event 'lifecycle' for domain nixos: Resumed Unpaused  2018-05-31 09:43:24.584+0000: event 'lifecycle' for domain nixos: Started Booted  2018-05-31 09:43:31.808+0000: event 'rtc-change' for domain nixos: 0  2018-05-31 09:43:32.808+0000: event 'rtc-change' for domain nixos: 0

The reasons for this not being done automatically are (from the top of my head):

- The above is just something I figured out myself, but it's not the     recommended way written anywhere.  Maybe I'm wrong and it doesn't     really work, but it still can be done manually.

- You cannot change it however you would like automatically, it is part  of the guest ABI and we are striving for keeping that stable.

- Trying to figure this out by 1 MiB increments might take some time,  but increasing it faster might be wasteful.

Basically there is no one-size-fits-all value, no easy way to do it automatically (maybe what I tried), but very good explanation how to do that manually and very easy way to do that.  Also, from the SW POV, it doesn't even depend on the guest OS, just on the loader/bios so if you have two same domains (like a template in OpenStack for example) you try it out once and then you have the value that just works and will continue working until you change something for the domain.  And what it depends on is clearly written in the documentation.

And there are those that could say if the underlying hypervisor knows that for certain memory sizes and/or vCPU counts that the TSEG will be too small for specific machine types that then the underlying hypervisor should be the one to "choose" a value that's programatically appropriate which to a degree IIUC is the argument being used against allowing a libvirt knob for the poll-max-ns and qcow2 cache sizes.

...

...

+ +          Due to the nature of this setting being similar to "how much RAM +          should the guest have" users are advised to either consult the +          documentation of the guest OS or loader (if there is any), or test +          this by trial-and-error changing the value until the VM boots +          successfully.  Yet another guiding value for users might be the fact +          that 48 MiB should be enough for pretty large guests (240 VCPUs and +          4TB guest RAM), but it is on purpose not set as default as 48 MiB of +          unavailable RAM might be too much for small guests (e.g. with 512 MiB +          of RAM).

and this is the exact reason why patches like this get NACKd - because trial and error should not be a 'desired' means to calculate.

It is not.  They are rejected because either a) there is no documentation how to properly check if the value is the right value when doing trial-end-error (this is not the case here since you can see if the machine boots or not) or b) the values being set are too specific instead of being abstracted -- setting value in KiB between 0 and the size of a disk instead of "max_performance" or "min_latency" (this is not the case here, the documentation explains what the size is and why it is not about few guessable values).

Basically we are NACKing simple pass-through values without understanding them and adding some documentation for them.  For example stuff for which we have documentation along the lines of: "Element asdf can be used to set the asdf of the domain."

I guess it's all a matter of how things are being viewed. I can certainly see how someone without a redhat.com email may view this series as similar to the referenced ones. It's more about mitigating failure possibilities by being able to provide knobs that can help (or get you into trouble). Not everyone has large configurations - so do they really need the value? Not everyone needs every knob, but having them available for when you do need them isn't a bad thing.

...

bz referenced in patch 5 has an incredible amount of data and calculations that provide even more insight and details that are lost when we try to summarize in a libvirt meaningful patch.

Let me know what relevant information from the bz you are missing in the documentation and I'll gladly add it.

If you think what's provided above is enough, then leave it as is. If there's something in that bz or in the ensuing discussion, then add it. Even though it's difficult when you're in the middle of the code - if you were someone who ran into this situation without knowing anything about it - would what's provided above give enough information to help you decide which value to use? That whole trial and error comment is what leaves me wondering as a reader who knew nothing about this before I started. And now I have more I need a pub to help forget ;-)

...

What it seems is really needed is an attribute that libvirt provides that tells QEMU to calculate the optimum size.

...

+ +          See <a href="#elementsMemoryAllocation">Memory Allocation</a> +          for more details about the <code>unit</code> attribute. +          <span class="since">Since 4.5.0</span> (QEMU only)

haha - see you put 4.5.0 and this is the 4.4.0 release - so it was ignored until 4.5.0 was "on the clock" ;-)

Ironically this is pointed out as QEMU only; however, genericxml2xmltest is used/updated.

So?

Why would someone add this generically if it only works for QEMU?

...

So, I personally don't mind if this attribute is added; however, I think we become hypocrites to a certain degree if patches continue to be blocked/NACKed to other subsystems that have attributes that allow certain amount of control, but don't come with exact sizing references. Still if this is pushed, then perhaps those others can use this as the means to provide a reference to other knobs added.

How much more exact would you wanted to be in terms of sizing?  If it could be any more exact we wouldn't need the tunable at all.  Please don't compare it to other tunables that we didn't want exposed just because it "sounds similar".  I hear lot of people just put stuff like this into "unknown knobs" box and they treat it the same.  But there are differences and it's not all

...

You can have my :

Reviewed-by: John Ferlan <jferlan@redhat.com>

I wasn't ever against the R-b, but I guess I'm missing the point.  You disagree with me on the change made here, but then I get a R-b? :) And then I go further down and read that the R-b has actually no value at all and I should wait for another one :D Maybe I'm overthinking this, but it didn't used to happen back when we used ACKs :)

...

with a few adjustments above; however, another R-By should be obtained here as well as perhaps a policy change so that other similar such series could be merged... I guess I'm curious what "thoughts" others may have regarding adding this "knob" while not allowing others.

John

Did I disagree on the change or was I pointing out that from a certain abstraction level it could be said the code is "similar to" the patches I referenced w/r/t libvirt supplying some value that perhaps the underlying hypervisor could/should have adjusted instead based on it's knowledge of the rather involved algorithms. FWIW: I'm not opposed to the referenced patches, but I'm in the minority on them. AIUI, the R-b is essentially stating that I've looked at the patch and in general agree with it. As part of that - there are a few things that need adjustment and since I felt that it had some similarities to the referenced patches w/r/t adding a knob that some may feel isn't necessary I said let's make sure there's "enough consensus" that this should be added (which it seems there is). IIRC, the following was referenced at some point during the R-b discussion: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... John

On Mon, May 21, 2018 at 05:00:51PM +0200, Martin Kletzander wrote:

TSEG (Top of Memory Segment) is one of many regions that SMM (System Management Mode) can occupy. This one, however is special, because a) most of the SMM code lives in TSEG nowadays and b) QEMU just (well, some time ago) added support for so called 'extended' TSEG. The difference to the TSEG implemented in real q35's MCH (Memory Controller Hub) is that it can have any size from 1 MiB up to 65534 MiB in 1 MiB increments. But more about that in the QEMU patch.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 39 +++++++++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++- src/conf/domain_conf.h | 1 + tests/genericxml2xmlindata/tseg.xml | 23 +++++++++++ tests/genericxml2xmltest.c | 2 + 6 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/tseg.xml

@@ -27076,6 +27102,38 @@ virDomainDefFormatInternal(virDomainDefPtr def,

break;

+ case VIR_DOMAIN_FEATURE_SMM: + switch ((virTristateSwitch) def->features[i]) { + case VIR_TRISTATE_SWITCH_LAST: + case VIR_TRISTATE_SWITCH_ABSENT: + break; + + case VIR_TRISTATE_SWITCH_ON: + virBufferAddLit(buf, "<smm state='on'"); + if (!def->tseg_size) { + virBufferAddLit(buf, "/>\n"); + } else { + const char *unit; + unsigned long long short_size = virFormatIntPretty(def->tseg_size, + &unit); + + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<tseg unit='%s'>%llu</tseg>\n", + unit, short_size); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</smm>\n"); + } + + break; + + case VIR_TRISTATE_SWITCH_OFF: + virBufferAddLit(buf, "<smm state='off'/>\n"); + break;

Consider using virXMLFormatElement. Jano

On 05/21/2018 11:00 AM, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 10 +++ src/qemu/qemu_capabilities.h | 2 + .../caps_1.5.3.x86_64.replies | 38 +++++++++-- .../caps_1.5.3.x86_64.xml | 3 +- .../caps_1.6.0.x86_64.replies | 38 +++++++++-- .../caps_1.6.0.x86_64.xml | 3 +- .../caps_1.7.0.x86_64.replies | 38 +++++++++-- .../caps_1.7.0.x86_64.xml | 3 +- .../caps_2.1.1.x86_64.replies | 38 +++++++++-- .../caps_2.1.1.x86_64.xml | 3 +- .../caps_2.10.0.x86_64.replies | 48 ++++++++++--- .../caps_2.10.0.x86_64.xml | 3 +- .../caps_2.12.0.x86_64.replies | 67 +++++++++++++++---- .../caps_2.12.0.x86_64.xml | 4 +- .../caps_2.4.0.x86_64.replies | 38 +++++++++-- .../caps_2.4.0.x86_64.xml | 3 +- .../caps_2.5.0.x86_64.replies | 40 +++++++++-- .../caps_2.5.0.x86_64.xml | 3 +- .../caps_2.6.0.x86_64.replies | 40 +++++++++-- .../caps_2.6.0.x86_64.xml | 3 +- .../caps_2.7.0.x86_64.replies | 40 +++++++++-- .../caps_2.7.0.x86_64.xml | 3 +- .../caps_2.8.0.x86_64.replies | 40 +++++++++-- .../caps_2.8.0.x86_64.xml | 3 +- .../caps_2.9.0.x86_64.replies | 48 ++++++++++--- .../caps_2.9.0.x86_64.xml | 3 +- 26 files changed, 458 insertions(+), 104 deletions(-)

Is there no other way to determine this without getting mch? and needing to update all those replies from earlier releases? I assume those are there because "mch" exists in 1.5.3 and beyond, but we never checked for it. So did you update the .replies files manually or did you run this against each version mentioned? Personally I think it's always a bonus if how the replies adjustments were made is described. It perhaps helps the next person with the same conundrum. Is there no other way to get this without supplying the "mch"/MCH as well? In any case, with some minor updates to the commit message to give a synopsis related to how the .replies were updated... Reviewed-by: John Ferlan <jferlan@redhat.com> John

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ebe35573e7cd..025f3617fee5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -487,6 +487,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 300 */ "sdl-gl", "screendump_device", + "mch", + "mch.extended-tseg-mbytes", );

@@ -1117,6 +1119,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pr-manager-helper", QEMU_CAPS_PR_MANAGER_HELPER }, { "virtual-css-bridge", QEMU_CAPS_CCW }, { "vfio-ccw", QEMU_CAPS_DEVICE_VFIO_CCW }, + { "mch", QEMU_CAPS_DEVICE_MCH }, };

static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { @@ -1262,6 +1265,10 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtualCSSBridge[] = { "cssid-unrestricted", QEMU_CAPS_CCW_CSSID_UNRESTRICTED }, };

+static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsMCH[] = { + { "extended-tseg-mbytes", QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES }, +}; + /* see documentation for virQEMUQAPISchemaPathGet for the query format */ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/options/+gluster/debug-level", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, @@ -1391,6 +1398,9 @@ static virQEMUCapsObjectTypeProps virQEMUCapsDeviceProps[] = { { "virtual-css-bridge", virQEMUCapsObjectPropsVirtualCSSBridge, ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtualCSSBridge), QEMU_CAPS_CCW }, + { "mch", virQEMUCapsDevicePropsMCH, + ARRAY_CARDINALITY(virQEMUCapsDevicePropsMCH), + QEMU_CAPS_DEVICE_MCH }, };

static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 8b6c0c89f4f5..5c4b83d83a1e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -471,6 +471,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 300 */ QEMU_CAPS_SDL_GL, /* -sdl gl */ QEMU_CAPS_SCREENDUMP_DEVICE, /* screendump command accepts device & head */ + QEMU_CAPS_DEVICE_MCH, /* Northbridge in q35 machine types */ + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES, /* -global mch.extended-tseg-mbytes */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies index 8da1b149d09f..dd501221ade9 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.replies @@ -2044,6 +2044,32 @@ "id": "libvirt-36" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-37" +} + { "return": [ { @@ -2114,7 +2140,7 @@ "cpu-max": 1 } ], - "id": "libvirt-37" + "id": "libvirt-38" }

{ @@ -2192,19 +2218,19 @@ "name": "qemu64" } ], - "id": "libvirt-38" + "id": "libvirt-39" }

{ "return": [ ], - "id": "libvirt-39" + "id": "libvirt-40" }

{ "return": [ ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -2905,7 +2931,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ @@ -2915,7 +2941,7 @@ "capability": "xbzrle" } ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml index e7c2e9a181aa..556466b858aa 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml @@ -105,9 +105,10 @@ <flag name='query-cpu-definitions'/> <flag name='kernel-irqchip'/> <flag name='isa-serial'/> + <flag name='mch'/> <version>1005003</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>46523</microcodeVersion> + <microcodeVersion>46889</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies index d53fb576d266..09b2b9d4a4fa 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.replies @@ -2089,6 +2089,32 @@ "id": "libvirt-36" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-37" +} + { "return": [ { @@ -2167,7 +2193,7 @@ "cpu-max": 1 } ], - "id": "libvirt-37" + "id": "libvirt-38" }

{ @@ -2245,19 +2271,19 @@ "name": "qemu64" } ], - "id": "libvirt-38" + "id": "libvirt-39" }

{ "return": [ ], - "id": "libvirt-39" + "id": "libvirt-40" }

{ "return": [ ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -2860,7 +2886,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ @@ -2882,7 +2908,7 @@ "capability": "zero-blocks" } ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ diff --git a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml index e57dec321d4c..9a7ad7c3e6ee 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml @@ -110,9 +110,10 @@ <flag name='query-cpu-definitions'/> <flag name='kernel-irqchip'/> <flag name='isa-serial'/> + <flag name='mch'/> <version>1006000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>44752</microcodeVersion> + <microcodeVersion>45118</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies index 4fcc7aa25a2e..72ba4c27e833 100644 --- a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.replies @@ -2106,6 +2106,32 @@ "id": "libvirt-36" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-37" +} + { "return": [ { @@ -2192,7 +2218,7 @@ "cpu-max": 1 } ], - "id": "libvirt-37" + "id": "libvirt-38" }

{ @@ -2270,19 +2296,19 @@ "name": "qemu64" } ], - "id": "libvirt-38" + "id": "libvirt-39" }

{ "return": [ ], - "id": "libvirt-39" + "id": "libvirt-40" }

{ "return": [ ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3075,7 +3101,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ @@ -3097,7 +3123,7 @@ "capability": "zero-blocks" } ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ diff --git a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml index 39ec8f9b1d71..5f83d26045bf 100644 --- a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml @@ -112,9 +112,10 @@ <flag name='query-cpu-definitions'/> <flag name='kernel-irqchip'/> <flag name='isa-serial'/> + <flag name='mch'/> <version>1007000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>50196</microcodeVersion> + <microcodeVersion>50562</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies index 543bce9defc1..1a21b7e8d9a7 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.replies @@ -2523,6 +2523,32 @@ "id": "libvirt-36" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-37" +} + { "return": [ { @@ -2625,7 +2651,7 @@ "cpu-max": 255 } ], - "id": "libvirt-37" + "id": "libvirt-38" }

{ @@ -2706,21 +2732,21 @@ "name": "qemu64" } ], - "id": "libvirt-38" + "id": "libvirt-39" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-39" + "id": "libvirt-40" }

{ "return": [ "passthrough" ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3580,7 +3606,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ @@ -3602,7 +3628,7 @@ "capability": "zero-blocks" } ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml index afe0882dde28..e3c168de2428 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml @@ -128,9 +128,10 @@ <flag name='query-named-block-nodes'/> <flag name='kernel-irqchip'/> <flag name='isa-serial'/> + <flag name='mch'/> <version>2001001</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>58992</microcodeVersion> + <microcodeVersion>59358</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies index 6c6ecc26874c..a32bcc7ba0fa 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies @@ -4471,6 +4471,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -4657,7 +4683,7 @@ "alias": "q35" } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -4992,21 +5018,21 @@ "migration-safe": true } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -6285,7 +6311,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -6335,7 +6361,7 @@ "capability": "return-path" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -16058,7 +16084,7 @@ "meta-type": "object" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ @@ -16237,7 +16263,7 @@ } } }, - "id": "libvirt-47" + "id": "libvirt-48" }

{ @@ -16480,7 +16506,7 @@ } } }, - "id": "libvirt-48" + "id": "libvirt-49" }

{ @@ -16659,7 +16685,7 @@ } } }, - "id": "libvirt-49" + "id": "libvirt-50" }

{ @@ -16902,7 +16928,7 @@ } } }, - "id": "libvirt-50" + "id": "libvirt-51" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml index 77ca3013b5ab..1fea178f2b68 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -197,9 +197,10 @@ <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2010000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>344938</microcodeVersion> + <microcodeVersion>345304</microcodeVersion> <package> (v2.10.0)</package> <arch>x86_64</arch> <hostCPU type='kvm' model='base' migratability='yes'> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies index c40046beef6b..78e1b450cda3 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies @@ -4605,6 +4605,49 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "x-pcie-lnksta-dllla", + "description": "on/off", + "type": "bool" + }, + { + "name": "multifunction", + "description": "on/off", + "type": "bool" + }, + { + "name": "extended-tseg-mbytes", + "type": "uint16" + }, + { + "name": "romfile", + "type": "str" + }, + { + "name": "x-pcie-extcap-init", + "description": "on/off", + "type": "bool" + }, + { + "name": "command_serr_enable", + "description": "on/off", + "type": "bool" + }, + { + "name": "addr", + "description": "Slot and optional function number, example: 06.0 or 06", + "type": "int32" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -4656,7 +4699,7 @@ "type": "string" } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -4855,7 +4898,7 @@ "cpu-max": 255 } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ @@ -5369,7 +5412,7 @@ "migration-safe": true } ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ @@ -5377,7 +5420,7 @@ "tpm-crb", "tpm-tis" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -5385,7 +5428,7 @@ "passthrough", "emulator" ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -6672,7 +6715,7 @@ "option": "drive" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -6734,7 +6777,7 @@ "capability": "dirty-bitmaps" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ @@ -18102,7 +18145,7 @@ "meta-type": "object" } ], - "id": "libvirt-47" + "id": "libvirt-48" }

{ @@ -18292,7 +18335,7 @@ } } }, - "id": "libvirt-48" + "id": "libvirt-49" }

{ @@ -18547,7 +18590,7 @@ } } }, - "id": "libvirt-49" + "id": "libvirt-50" }

{ @@ -18737,7 +18780,7 @@ } } }, - "id": "libvirt-50" + "id": "libvirt-51" }

{ @@ -18992,7 +19035,7 @@ } } }, - "id": "libvirt-51" + "id": "libvirt-52" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 820b3ef75984..3c4bdcfd9094 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -201,9 +201,11 @@ <flag name='memory-backend-file.discard-data'/> <flag name='sdl-gl'/> <flag name='screendump_device'/> + <flag name='mch'/> + <flag name='mch.extended-tseg-mbytes'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>390813</microcodeVersion> + <microcodeVersion>391586</microcodeVersion> <package>v2.12.0-rc0</package> <arch>x86_64</arch> <hostCPU type='kvm' model='base' migratability='yes'> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies index 68ecb0c17dc7..bf8e7b4379ff 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies @@ -3114,6 +3114,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -3240,7 +3266,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3330,21 +3356,21 @@ "name": "qemu64" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -4352,7 +4378,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -4382,7 +4408,7 @@ "capability": "events" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml index 871eb5e4a702..2c2162460072 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml @@ -154,9 +154,10 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='isa-serial'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2004000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>75406</microcodeVersion> + <microcodeVersion>75772</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies index 5bc505abb32c..57bf70f0e29b 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies @@ -3277,6 +3277,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -3411,7 +3437,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3501,21 +3527,21 @@ "name": "qemu64" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -4560,7 +4586,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -4594,7 +4620,7 @@ "capability": "x-postcopy-ram" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -12139,7 +12165,7 @@ "meta-type": "array" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml index 90dce5a700be..3f33a272677c 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -160,9 +160,10 @@ <flag name='virtio-gpu.max_outputs'/> <flag name='isa-serial'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2005000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>216528</microcodeVersion> + <microcodeVersion>216894</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies index 73a22ed0bb95..436c824c14e8 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies @@ -3359,6 +3359,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -3469,7 +3495,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3559,21 +3585,21 @@ "name": "qemu64" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -4667,7 +4693,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -4701,7 +4727,7 @@ "capability": "postcopy-ram" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -12706,7 +12732,7 @@ "meta-type": "array" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 4c6371a6cd07..88b33038fb81 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -172,9 +172,10 @@ <flag name='dump-completed'/> <flag name='nbd-tls'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2006000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>227332</microcodeVersion> + <microcodeVersion>227698</microcodeVersion> <package></package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies index eaa84d3381a6..9d71070b0831 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies @@ -3554,6 +3554,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -3700,7 +3726,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -3793,21 +3819,21 @@ "name": "qemu64" } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -4905,7 +4931,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -4939,7 +4965,7 @@ "capability": "postcopy-ram" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -13295,7 +13321,7 @@ "meta-type": "object" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index c322d18d7da5..e4e7a7b0e167 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -177,9 +177,10 @@ <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2007000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>239029</microcodeVersion> + <microcodeVersion>239395</microcodeVersion> <package> (v2.7.0)</package> <arch>x86_64</arch> <cpu type='kvm' name='Opteron_G5'/> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies index 30d28c7b5f4a..336f9fbca2f6 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies @@ -3696,6 +3696,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -3862,7 +3888,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -4068,21 +4094,21 @@ "static": false } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -5205,7 +5231,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -5243,7 +5269,7 @@ "capability": "x-colo" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -14013,7 +14039,7 @@ "meta-type": "object" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml index 7c329ad4c702..3b93c5871527 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml @@ -179,9 +179,10 @@ <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2008000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>255684</microcodeVersion> + <microcodeVersion>256050</microcodeVersion> <package> (v2.8.0)</package> <arch>x86_64</arch> <cpu type='kvm' name='host' usable='yes'/> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies index 5da1b41c2ee1..b03e3d495045 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies @@ -4015,6 +4015,32 @@ "id": "libvirt-39" }

+{ + "return": [ + { + "name": "command_serr_enable", + "type": "on/off" + }, + { + "name": "multifunction", + "type": "on/off" + }, + { + "name": "rombar", + "type": "uint32" + }, + { + "name": "romfile", + "type": "string" + }, + { + "name": "addr", + "type": "pci-devfn" + } + ], + "id": "libvirt-40" +} + { "return": [ { @@ -4191,7 +4217,7 @@ "cpu-max": 255 } ], - "id": "libvirt-40" + "id": "libvirt-41" }

{ @@ -4473,21 +4499,21 @@ "migration-safe": true } ], - "id": "libvirt-41" + "id": "libvirt-42" }

{ "return": [ "tpm-tis" ], - "id": "libvirt-42" + "id": "libvirt-43" }

{ "return": [ "passthrough" ], - "id": "libvirt-43" + "id": "libvirt-44" }

{ @@ -5736,7 +5762,7 @@ "option": "drive" } ], - "id": "libvirt-44" + "id": "libvirt-45" }

{ @@ -5778,7 +5804,7 @@ "capability": "release-ram" } ], - "id": "libvirt-45" + "id": "libvirt-46" }

{ @@ -15064,7 +15090,7 @@ "meta-type": "object" } ], - "id": "libvirt-46" + "id": "libvirt-47" }

{ @@ -15243,7 +15269,7 @@ } } }, - "id": "libvirt-47" + "id": "libvirt-48" }

{ @@ -15484,7 +15510,7 @@ } } }, - "id": "libvirt-48" + "id": "libvirt-49" }

{ @@ -15663,7 +15689,7 @@ } } }, - "id": "libvirt-49" + "id": "libvirt-50" }

{ @@ -15904,7 +15930,7 @@ } } }, - "id": "libvirt-50" + "id": "libvirt-51" }

{ diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index 0701c244f63a..47fda742634c 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -192,9 +192,10 @@ <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='sdl-gl'/> + <flag name='mch'/> <version>2009000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>320947</microcodeVersion> + <microcodeVersion>321313</microcodeVersion> <package> (v2.9.0)</package> <arch>x86_64</arch> <hostCPU type='kvm' model='base' migratability='yes'>

On Wed, May 30, 2018 at 08:01:10PM +0200, Ján Tomko wrote:

On Mon, May 21, 2018 at 05:00:53PM +0200, Martin Kletzander wrote:

...

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 18 ++++ src/qemu/qemu_domain.c | 84 +++++++++++++++++++ .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ .../tseg-old-machine-type.args | 27 ++++++ .../tseg-old-machine-type.xml | 21 +++++ tests/qemuxml2argvdata/tseg.args | 28 +++++++ tests/qemuxml2argvdata/tseg.xml | 21 +++++ tests/qemuxml2argvtest.c | 48 +++++++++++ .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ .../tseg-old-machine-type.xml | 44 ++++++++++ tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ tests/qemuxml2xmltest.c | 25 ++++++ 15 files changed, 505 insertions(+) create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2argvdata/tseg.args create mode 100644 tests/qemuxml2argvdata/tseg.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2xmloutdata/tseg.xml

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 881d0ea46a75..3ea9e3d47344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) }

+static int +qemuDomainSetDefaultTsegSize(virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + const char *machine = NULL; + char *end_ptr = NULL; + unsigned int major = 0; + unsigned int minor = 0; + + def->tseg_size = 0; + + if (!qemuDomainIsQ35(def)) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) + return 0; + + machine = STRSKIP(def->os.machine, "pc-q35-"); + + if (!machine) + goto error; + + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) + goto error; + + if (*end_ptr != '.') + goto error; + + machine = end_ptr + 1; + + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) + goto error; + if (*end_ptr != '\0') + goto error; + + /* QEMU started defaulting to 16MiB after 2.9 */ + if (major > 2 || (major == 2 && minor > 9))

Please use virParseVersionString

We have that? Cool. There was no patch related to that function since I started working on libvirt =D And only few of that that just used it.

...

+ def->tseg_size = 16 * 1024 * 1024; + + return 0; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse QEMU machine type version '%s'"), + def->os.machine); + return -1; +} + + +static int +qemuDomainDefTsegPostParse(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) + return 0; + + if (!def->tseg_size) + return qemuDomainSetDefaultTsegSize(def, qemuCaps); + + if (!qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SMM TSEG is only supported with q35 machine type")); + return -1; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting TSEG size is not supported with this QEMU binary")); + return -1; + } + + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {

Interesting way of writing the % operator.

...

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SMM TSEG size must be divisible by 1 MiB")); + return -1; + } + + return 0; +} + + static int qemuDomainDefPostParseBasic(virDomainDefPtr def, virCapsPtr caps,

I'm not sure whether this trial-and-error attribute angers some purists, but they'll have plenty of time to object until the freeze is over.

Dou you have any other better idea? Laszlo properly compared it to picking for example the right amount of memory. How can you know how much do you need? It can never be automatically guessed. You can have extra, but each use case will be hindered a different amount by that. Feel free to suggest a better idea or Cc some purists ;)

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

This is way too sparse. On 05/21/2018 11:00 AM, Martin Kletzander wrote:

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 18 ++++ src/qemu/qemu_domain.c | 84 +++++++++++++++++++ .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ .../tseg-old-machine-type.args | 27 ++++++ .../tseg-old-machine-type.xml | 21 +++++ tests/qemuxml2argvdata/tseg.args | 28 +++++++ tests/qemuxml2argvdata/tseg.xml | 21 +++++ tests/qemuxml2argvtest.c | 48 +++++++++++ .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ .../tseg-old-machine-type.xml | 44 ++++++++++ tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ tests/qemuxml2xmltest.c | 25 ++++++ 15 files changed, 505 insertions(+) create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2argvdata/tseg.args create mode 100644 tests/qemuxml2argvdata/tseg.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2xmloutdata/tseg.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 328f3c0a2386..36f557676fb0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7200,6 +7200,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, return ret; }

+ +static void +qemuBuildTSEGCommandLine(virCommandPtr cmd, + const virDomainDef *def) +{ + if (!def->tseg_size)

Since it's not bool or pointer, prefer the tseg_size == 0

+ return; + + virCommandAddArg(cmd, "-global"); + + /* PostParse callback guarantees that the size is divisible by 1 MiB */ + virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu", + def->tseg_size >> 20); +} + + static int qemuBuildSmpCommandLine(virCommandPtr cmd, virDomainDefPtr def) @@ -9921,6 +9937,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0) goto error;

+ qemuBuildTSEGCommandLine(cmd, def); + if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0) goto error;

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 881d0ea46a75..3ea9e3d47344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) }

+static int +qemuDomainSetDefaultTsegSize(virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + const char *machine = NULL; + char *end_ptr = NULL; + unsigned int major = 0; + unsigned int minor = 0; + + def->tseg_size = 0;

Pointless since the only way in here is "if (tseg_size == 0)"

+ + if (!qemuDomainIsQ35(def)) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))

Reading this now makes me realized _MBYTES is probably unnecessary, IDC though since it does follow the QEMU name.

+ return 0; + + machine = STRSKIP(def->os.machine, "pc-q35-"); + + if (!machine) + goto error; + + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) + goto error; + + if (*end_ptr != '.') + goto error; + + machine = end_ptr + 1; + + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) + goto error; + if (*end_ptr != '\0') + goto error; + + /* QEMU started defaulting to 16MiB after 2.9 */ + if (major > 2 || (major == 2 && minor > 9)) + def->tseg_size = 16 * 1024 * 1024;

So, if QEMU defaults to 16MiB, then why do we need so set this at all? This seems to me we are setting policy which based on history of many patches before is a no go. I'd say NAK to this, unless there is some convincing argument made in the commit message and followup responses to the series (or you can take Jan's R-By and ignore me - your call.

+ + return 0; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse QEMU machine type version '%s'"), + def->os.machine); + return -1; +} + + +static int +qemuDomainDefTsegPostParse(virDomainDefPtr def,

While TSEG is what you're adjusting, this is more a 'features' or in particular SMM features post parse callback.

+ virQEMUCapsPtr qemuCaps) +{ + if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) + return 0; + + if (!def->tseg_size)

Similar... prefer tseg_size == 0

+ return qemuDomainSetDefaultTsegSize(def, qemuCaps); + + if (!qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SMM TSEG is only supported with q35 machine type")); + return -1; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting TSEG size is not supported with this QEMU binary")); + return -1; + } + + if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SMM TSEG size must be divisible by 1 MiB")); + return -1; + }

Does anywhere else that does a VIR_ROUND_UP elicit an error? Why bother. Curious that this differs from qemuDomainMemoryDeviceAlignSize which claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which returns 1024 unless it's PPC64 which returns 256MiB alignments.

+ + return 0; +} + + static int qemuDomainDefPostParseBasic(virDomainDefPtr def, virCapsPtr caps, @@ -3389,6 +3470,9 @@ qemuDomainDefPostParse(virDomainDefPtr def, if (qemuDomainDefCPUPostParse(def) < 0) goto cleanup;

+ if (qemuDomainDefTsegPostParse(def, qemuCaps) < 0) + goto cleanup; + ret = 0; cleanup: virObjectUnref(cfg); diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.args b/tests/qemuxml2argvdata/tseg-explicit-size.args new file mode 100644 index 000000000000..d49c81697e43 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-explicit-size.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \ +-global mch.extended-tseg-mbytes=48 \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/tseg-explicit-size.xml b/tests/qemuxml2argvdata/tseg-explicit-size.xml new file mode 100644 index 000000000000..ae3121048495 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg>48</tseg>

The only difference here from genericxml2xmlindata/tseg.xml is that this one doesn't have "unit='MiB'"

+ </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/tseg-i440fx.xml b/tests/qemuxml2argvdata/tseg-i440fx.xml new file mode 100644 index 000000000000..5bd832d50829 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-i440fx.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg unit='MiB'>48</tseg> + </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/tseg-invalid-size.xml b/tests/qemuxml2argvdata/tseg-invalid-size.xml new file mode 100644 index 000000000000..3ac8069a81ce --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-invalid-size.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg unit='KiB'>12345</tseg> + </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.args b/tests/qemuxml2argvdata/tseg-old-machine-type.args new file mode 100644 index 000000000000..ebbdb15e68f2 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc-q35-2.9,accel=tcg,usb=off,smm=on,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/tseg-old-machine-type.xml b/tests/qemuxml2argvdata/tseg-old-machine-type.xml new file mode 100644 index 000000000000..d1e42586f144 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg-old-machine-type.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.9'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/tseg.args b/tests/qemuxml2argvdata/tseg.args new file mode 100644 index 000000000000..995957ef1d58 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc-q35-2.10,accel=tcg,usb=off,smm=on,dump-guest-core=off \ +-global mch.extended-tseg-mbytes=16 \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvdata/tseg.xml b/tests/qemuxml2argvdata/tseg.xml new file mode 100644 index 000000000000..7a31e348b258 --- /dev/null +++ b/tests/qemuxml2argvdata/tseg.xml

and yet another one similr to genericxml2xmlindata/tseg.xml - same name, different directory

@@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 78454acb1a41..633dfaaee9a4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2827,6 +2827,54 @@ mymain(void)

DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");

+ DO_TEST("tseg", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); + DO_TEST("tseg-explicit-size", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); + DO_TEST("tseg-old-machine-type", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES);

These all should use DO_TEST_CAPS_LATEST

+ DO_TEST_PARSE_ERROR("tseg-i440fx", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); Failure wrong machine type...

+ DO_TEST_PARSE_ERROR("tseg-explicit-size", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI);

Failure because TSEG_MBYTES not provided.

+ DO_TEST_PARSE_ERROR("tseg-invalid-size", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); + /* Test disks with format probing enabled for legacy reasons. * New tests should not go in this section. */ driver.config->allowDiskFormatProbing = true; diff --git a/tests/qemuxml2xmloutdata/tseg-explicit-size.xml b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml new file mode 100644 index 000000000000..e1a6e15b610e --- /dev/null +++ b/tests/qemuxml2xmloutdata/tseg-explicit-size.xml @@ -0,0 +1,46 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg unit='MiB'>48</tseg> + </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml new file mode 100644 index 000000000000..594c5c025d2e --- /dev/null +++ b/tests/qemuxml2xmloutdata/tseg-old-machine-type.xml @@ -0,0 +1,44 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.9'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/tseg.xml b/tests/qemuxml2xmloutdata/tseg.xml new file mode 100644 index 000000000000..954ee9d9ee24 --- /dev/null +++ b/tests/qemuxml2xmloutdata/tseg.xml @@ -0,0 +1,46 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <smm state='on'> + <tseg unit='MiB'>16</tseg> + </smm> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7cedc2b999b3..14824679b81c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1181,6 +1181,31 @@ mymain(void) QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW);

+ DO_TEST("tseg", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); + DO_TEST("tseg-explicit-size", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); + DO_TEST("tseg-old-machine-type", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_MACHINE_SMM_OPT, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); +

Never quite understood why xml2xml tests need CAPS... John

/* Test disks with format probing enabled for legacy reasons. * New tests should not go in this section. */ driver.config->allowDiskFormatProbing = true;

On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:

...

This is way too sparse.

On 05/21/2018 11:00 AM, Martin Kletzander wrote:

...

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 18 ++++ src/qemu/qemu_domain.c | 84 +++++++++++++++++++ .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ .../tseg-old-machine-type.args | 27 ++++++ .../tseg-old-machine-type.xml | 21 +++++ tests/qemuxml2argvdata/tseg.args | 28 +++++++ tests/qemuxml2argvdata/tseg.xml | 21 +++++ tests/qemuxml2argvtest.c | 48 +++++++++++ .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ .../tseg-old-machine-type.xml | 44 ++++++++++ tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ tests/qemuxml2xmltest.c | 25 ++++++ 15 files changed, 505 insertions(+) create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2argvdata/tseg.args create mode 100644 tests/qemuxml2argvdata/tseg.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2xmloutdata/tseg.xml

[...]

...

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 881d0ea46a75..3ea9e3d47344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) }

+static int +qemuDomainSetDefaultTsegSize(virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + const char *machine = NULL; + char *end_ptr = NULL; + unsigned int major = 0; + unsigned int minor = 0; + + def->tseg_size = 0;

Pointless since the only way in here is "if (tseg_size == 0)"

...

+ + if (!qemuDomainIsQ35(def)) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))

Reading this now makes me realized _MBYTES is probably unnecessary, IDC though since it does follow the QEMU name.

...

+ return 0; + + machine = STRSKIP(def->os.machine, "pc-q35-"); + + if (!machine) + goto error; + + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) + goto error; + + if (*end_ptr != '.') + goto error; + + machine = end_ptr + 1; + + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) + goto error; + if (*end_ptr != '\0') + goto error; + + /* QEMU started defaulting to 16MiB after 2.9 */ + if (major > 2 || (major == 2 && minor > 9)) + def->tseg_size = 16 * 1024 * 1024;

So, if QEMU defaults to 16MiB, then why do we need so set this at all?

This seems to me we are setting policy which based on history of many patches before is a no go. I'd say NAK to this, unless there is some convincing argument made in the commit message and followup responses to the series (or you can take Jan's R-By and ignore me - your call.

I agree with John, this whole function should be removed, we don't have to set the default in config XML. However we should record that default value into live/status XML to make sure that it will not be changed during migration and to let the user know what is the default value.

That doesn't make sense. This is part of guest ABI and if you want to be on the safe side when migrating, then you should be way more cautious with the stop/start scenario. Migration will probably fail (or silently corrupt data, but I believe that wouldn't happen), but stop/start without keeping the default will change that in case QEMU changes default and then the guest ABI will change. See my other reply to this particular concern.

In order to do that, look at qemuProcessRefreshState() function where we already gather some information from QEMU after starting domain, the tseg default value can by updated by calling:

qemuMonitorJSONGetObjectProperty() with path: /machine/q35/mch property: extended-tseg-mbytes

Pavel

On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:

On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:

...

On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:

...

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:

...

This is way too sparse.

On 05/21/2018 11:00 AM, Martin Kletzander wrote:

...

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 18 ++++ src/qemu/qemu_domain.c | 84 +++++++++++++++++++ .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ .../tseg-old-machine-type.args | 27 ++++++ .../tseg-old-machine-type.xml | 21 +++++ tests/qemuxml2argvdata/tseg.args | 28 +++++++ tests/qemuxml2argvdata/tseg.xml | 21 +++++ tests/qemuxml2argvtest.c | 48 +++++++++++ .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ .../tseg-old-machine-type.xml | 44 ++++++++++ tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ tests/qemuxml2xmltest.c | 25 ++++++ 15 files changed, 505 insertions(+) create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2argvdata/tseg.args create mode 100644 tests/qemuxml2argvdata/tseg.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml create mode 100644 tests/qemuxml2xmloutdata/tseg.xml

[...]

...

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 881d0ea46a75..3ea9e3d47344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) }

+static int +qemuDomainSetDefaultTsegSize(virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + const char *machine = NULL; + char *end_ptr = NULL; + unsigned int major = 0; + unsigned int minor = 0; + + def->tseg_size = 0;

Pointless since the only way in here is "if (tseg_size == 0)"

...

+ + if (!qemuDomainIsQ35(def)) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))

Reading this now makes me realized _MBYTES is probably unnecessary, IDC though since it does follow the QEMU name.

...

+ return 0; + + machine = STRSKIP(def->os.machine, "pc-q35-"); + + if (!machine) + goto error; + + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) + goto error; + + if (*end_ptr != '.') + goto error; + + machine = end_ptr + 1; + + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) + goto error; + if (*end_ptr != '\0') + goto error; + + /* QEMU started defaulting to 16MiB after 2.9 */ + if (major > 2 || (major == 2 && minor > 9)) + def->tseg_size = 16 * 1024 * 1024;

So, if QEMU defaults to 16MiB, then why do we need so set this at all?

This seems to me we are setting policy which based on history of many patches before is a no go. I'd say NAK to this, unless there is some convincing argument made in the commit message and followup responses to the series (or you can take Jan's R-By and ignore me - your call.

I agree with John, this whole function should be removed, we don't have to set the default in config XML. However we should record that default value into live/status XML to make sure that it will not be changed during migration and to let the user know what is the default value.

That doesn't make sense. This is part of guest ABI and if you want to be on the safe side when migrating, then you should be way more cautious with the stop/start scenario. Migration will probably fail (or silently corrupt data, but I believe that wouldn't happen), but stop/start without keeping the default will change that in case QEMU changes default and then the guest ABI will change. See my other reply to this particular concern.

I'm not sure that's true. You can consider CPU as guest ABI but using host-model the actual CPU is described only in live/status XML but the config XML always has mode set to host-model.

Well, but you used the wrong example. The documentation for host-model properly says: "...but shutting down and restarting the guest may present different hardware to the guest according to the capabilities of the new host." host-model is one of the special cases where we cannot provide stable guest ABI and we explicitly say that.

Another thing is that if user doesn't care about this value they would lose the benefit of using the default hypervisor value and once it get's updated in the hypervisor they would be stuck with the previous default.

Yes. But that's actually precisely the reason we are doing this.

We might need to somehow specify what guest ABI means and what it should cover and what can be left to hypervisor.

Well, we don't, guest ABI is not a term we would thought of. Whatever the guest OS sees as part of the hardware, whatever it can gather about the virtual machine is part of the guest ABI. Don't think of this as host-model, but rather chassisNr. For very brief, but good, explanation see commit 8dc88aeed612aeea1ae0d249ec760938cecce5aa.

Pavel

On Thu, May 31, 2018 at 03:52:26PM +0200, Pavel Hrdina wrote:

On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote:

...

On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:

...

On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:

...

On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:

...

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:

...

This is way too sparse.

On 05/21/2018 11:00 AM, Martin Kletzander wrote: > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338 > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > --- > src/qemu/qemu_command.c | 18 ++++ > src/qemu/qemu_domain.c | 84 +++++++++++++++++++ > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ > .../tseg-old-machine-type.args | 27 ++++++ > .../tseg-old-machine-type.xml | 21 +++++ > tests/qemuxml2argvdata/tseg.args | 28 +++++++ > tests/qemuxml2argvdata/tseg.xml | 21 +++++ > tests/qemuxml2argvtest.c | 48 +++++++++++ > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ > .../tseg-old-machine-type.xml | 44 ++++++++++ > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ > tests/qemuxml2xmltest.c | 25 ++++++ > 15 files changed, 505 insertions(+) > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml > create mode 100644 tests/qemuxml2argvdata/tseg.args > create mode 100644 tests/qemuxml2argvdata/tseg.xml > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml

[...]

...

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 881d0ea46a75..3ea9e3d47344 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) > } > > > +static int > +qemuDomainSetDefaultTsegSize(virDomainDef *def, > + virQEMUCapsPtr qemuCaps) > +{ > + const char *machine = NULL; > + char *end_ptr = NULL; > + unsigned int major = 0; > + unsigned int minor = 0; > + > + def->tseg_size = 0;

Pointless since the only way in here is "if (tseg_size == 0)"

> + > + if (!qemuDomainIsQ35(def)) > + return 0; > + > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))

Reading this now makes me realized _MBYTES is probably unnecessary, IDC though since it does follow the QEMU name.

> + return 0; > + > + machine = STRSKIP(def->os.machine, "pc-q35-"); > + > + if (!machine) > + goto error; > + > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) > + goto error; > + > + if (*end_ptr != '.') > + goto error; > + > + machine = end_ptr + 1; > + > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) > + goto error; > + if (*end_ptr != '\0') > + goto error; > + > + /* QEMU started defaulting to 16MiB after 2.9 */ > + if (major > 2 || (major == 2 && minor > 9)) > + def->tseg_size = 16 * 1024 * 1024;

So, if QEMU defaults to 16MiB, then why do we need so set this at all?

This seems to me we are setting policy which based on history of many patches before is a no go. I'd say NAK to this, unless there is some convincing argument made in the commit message and followup responses to the series (or you can take Jan's R-By and ignore me - your call.

I agree with John, this whole function should be removed, we don't have to set the default in config XML. However we should record that default value into live/status XML to make sure that it will not be changed during migration and to let the user know what is the default value.

That doesn't make sense. This is part of guest ABI and if you want to be on the safe side when migrating, then you should be way more cautious with the stop/start scenario. Migration will probably fail (or silently corrupt data, but I believe that wouldn't happen), but stop/start without keeping the default will change that in case QEMU changes default and then the guest ABI will change. See my other reply to this particular concern.

I'm not sure that's true. You can consider CPU as guest ABI but using host-model the actual CPU is described only in live/status XML but the config XML always has mode set to host-model.

Well, but you used the wrong example. The documentation for host-model properly says:

"...but shutting down and restarting the guest may present different hardware to the guest according to the capabilities of the new host."

host-model is one of the special cases where we cannot provide stable guest ABI and we explicitly say that.

...

Another thing is that if user doesn't care about this value they would lose the benefit of using the default hypervisor value and once it get's updated in the hypervisor they would be stuck with the previous default.

Yes. But that's actually precisely the reason we are doing this.

...

We might need to somehow specify what guest ABI means and what it should cover and what can be left to hypervisor.

Well, we don't, guest ABI is not a term we would thought of. Whatever the guest OS sees as part of the hardware, whatever it can gather about the virtual machine is part of the guest ABI.

Agreed, I should have express myself precisely, it's definitely guest ABI. What I meant is that some changes are not that critical to the guest OS or they don't have any real effect.

In this case it's just a size of memory, it's not like it will disable/enable that feature. FWIU the size can affect whether some guest with lot of vcpus and/or with lot of memory will not boot.

So yes, ideally we should record the default value also into the config XML to make sure that once the guest is installed/configured the value is not changed. The only thing that I don't personally like is that the value is hard-coded into libvirt and in the future we will most likely change it.

One possible compromise could be that we would not set any value at define time, instead if no value is configured we would get the default from QEMU and propagate that default into config as well to make sure that every other time the guest is started again it will have the same ABI. I hope that this idea is not too crazy :).

No, that's one of the possibilities. We might also believe QEMU that they will not change the default without machine type change and be done with it. It will have the same effect as if the VM was launched before with older libvirt and then with an updated one, which got updated with QEMU as well. If QEMU doesn't change the default per machine type, then we're fine. It seems like an okay way to go. At least I can yell "told you so" later in case QEMU changes that default and someone asks us to start recording the 16MiB value and it's going to be too late =) But that's not going to happen, right? I'll send a simplified v2 and we'll see how that goes.

On 05/31/2018 03:24 AM, Martin Kletzander wrote:

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:

...

This is way too sparse.

I can't really think of what to say here that is not already mentioned in the documentation or self-explanatory in the code.  But maybe only I see that as self-explanatory.  I'll write something up here.

The whole default thing is what really drew my attention back to the top... It's where I expected to see an explanation why the default was being set. I think that's something that should be described - you may disagree, but that's why it's a review. [...] See, I know how to cut - I just forgot in my response to patch 4 ;-) I'll try not to overcut as I've seen done in for reviews I've received which means I have to find the context of the feedback (we all have our favorite things to gripe about).

...

...

+ +static void +qemuBuildTSEGCommandLine(virCommandPtr cmd, +                         const virDomainDef *def) +{ +    if (!def->tseg_size)

Since it's not bool or pointer, prefer the tseg_size == 0

I don't know if you mean that as indicative or imperative.  It is very subjective and for such scenarios I would like to execute my right to mention that it is not part of Contributor Guidelines.  I know it might seem rude, but this is one of the things that's very subjective and for such things I like to first reach a consensus before I change what I'm used to writing.  I hope you understand that.

There's a lot of things not specifically listed in the CG that are mentioned in many reviews or that are just now done because they have been mentioned (2 blank lines between functions, formatting of function headers, 1 variable per line, etc.). When I see this style and remember to note it, I do... And I have see others mention it too. Many times it's for enum comparisons. In the long run, whatever. It's a style preference that denotes variable usage for those reading code "in the future". We don't have a rule for it, so go with your style if that's what you prefer. FWIW: back in the dark ages when I first started doing this... Something like the above for an integer, long, etc. value would be converted by the compiler to checking *only* if the low bit was set/cleared (think little endian) because that instruction was really quick. Try to find that needle in a haystack for each even value that could be used ;-). I got very used to the explicit comparison to 0 just to be 100% clear.

...

...

+        return; + +    virCommandAddArg(cmd, "-global"); +

[...]

...

...

+    /* QEMU started defaulting to 16MiB after 2.9 */ +    if (major > 2 || (major == 2 && minor > 9)) +        def->tseg_size = 16 * 1024 * 1024;

So, if QEMU defaults to 16MiB, then why do we need so set this at all?

TL;DR because not setting the default value when it is not explicitly requested has already bitten us in the back multiple times.

Long story short this way we are safe.  No matter what happens (QEMU changing the default, the user changing the config somewhere else and not expecting this to change, us wanting to change the default in the future for some reason, migration to newer libvirt where who-knows-what is checked there, etc.) it is way easier to keep the guest ABI stable. It is visible what the value actually is and we're not hiding it somewhere in the code of the hypervisor.  I know we don't do that for many things.  I could've gone with the (often alibistic [1]) "the default is left for hypervisor to decide", but since we have the opportunity tomake things right (thanks to Laszlo's explanations), why shouldn't we?

...

This seems to me we are setting policy which based on history of many patches before is a no go.  I'd say NAK to this, unless there is some convincing argument made in the commit message and followup responses to the series (or you can take Jan's R-By and ignore me - your call.

What patches are you referring to?  I can add that to the commit message, sure.

In my mind, for starters what I mentioned in my response to patch 3 for formatdomain.html.in changes. Beyond that, I don't really don't have the patience to go through hundreds of patches of history in order to pick out ones that were NACK'd or requested to be changed because the patch made some policy decision or a let's set a default value type decision. I know they've happened though. TL;DR: I'm still of the opinion we don't set (or even save) a default. I would think someone that failed to boot their guest because the memory size or vCPU count was too large for smm/tseg would then be pointed to this value which they would then optionally add and then "manage" going forward. I still don't see the point of setting a default if the default for QEMU has been good enough up to now up to and including a certain memory size of number of vCPU's, then how does us setting that default change anything? Especially if it's the same value that QEMU uses. How do we know that the value in the XML is one we provided or one the user provided? Without providing a default and if QEMU changed the default would a migration fail? If we set a default and then the migration failed, where does the fickle finger of fate fall? Yes, Laszlo's explanations are great, but they're lost to the mailing list and the fading memories of those reading them if not captured in some way in comments to code. If there's a consensus to allow a default to be listed, then I'm not going to even try to block. However, not mentioning it or calling it out as part of the review would to me would be wrong. I believe by doing this you're setting a precedent although I could be wrong as I don't have the entire code base memorized.

...

...

+ +    return 0; + + error: +    virReportError(VIR_ERR_INTERNAL_ERROR, +                   _("Cannot parse QEMU machine type version '%s'"), +                   def->os.machine); +    return -1; +} + + +static int +qemuDomainDefTsegPostParse(virDomainDefPtr def,

While TSEG is what you're adjusting, this is more a 'features' or in particular SMM features post parse callback.

I'm getting the hint of a hidden suggestion from the sentence.  Are you suggesting I should rename the function or create few more layers like this?

qemuDomainDefSMMFeaturesPostParse() {    qemuDomainDefTsegPostParse(); }

qemuDomainDefFeaturesPostParse() {    qemuDomainDefSMMFeaturesPostParse(); }

qemuDomainDefPostParse() {    ...    qemuDomainDefFeaturesPostParse();    ... }

While the above is the progression I had in mind, I cannot make my mind up if we should go through the insanity of all that or just go with the qemuDomainDefSMMFeaturesPostParse which "allows" someone with a future need to create a qemuDomainDefXXXFeaturesPostParse that is then either directly called from qemuDomainDefPostParse or indirectly from a new qemuDomainDefFeaturesPostParse. This another one of those things not explicitly stated in the CG that I know I've received feedback on in the past or have seen done by other patches with respect to the naming of functions and/or creation of shell methods to drill down into the path of the details. Function naming can be so subjective...

...

...

+                           virQEMUCapsPtr qemuCaps) +{ +    if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) +        return 0; + +    if (!def->tseg_size)

Similar... prefer tseg_size == 0

Again, are you directing me to do that or saying what version you like more?

Pointing out a similar construct that IMO should be changed, but you seem to dislike that way of comparison, so whatever. Yes, I know it's functionally equivalent.

...

...

+        return qemuDomainSetDefaultTsegSize(def, qemuCaps); + +    if (!qemuDomainIsQ35(def)) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +                       _("SMM TSEG is only supported with q35 machine type")); +        return -1; +    } + +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +                       _("Setting TSEG size is not supported with this QEMU binary")); +        return -1; +    } + +    if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +                       _("SMM TSEG size must be divisible by 1 MiB")); +        return -1; +    }

Does anywhere else that does a VIR_ROUND_UP elicit an error?  Why bother.

Curious that this differs from qemuDomainMemoryDeviceAlignSize which claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which returns 1024 unless it's PPC64 which returns 256MiB alignments.

It does.  The extended TSEG explicitly allows 1 MiB increments.  Not only because of the naming (*_mbytes), but also because bigger granularity would just be pointless probably.  Also this is not going to exist on PPC64, it only makes sense with q35.

I was partially commenting on why even bother - just force the rounding and indicate that libvirt will do so - that I think has been done numerous times before. Failing just because someone (tester) used 12345KiB or 1025KiB just seems pointless especially if we can and say we will round if you don't know how to make things 1MiB aligned. A value below 1 MiB would be useless - in fact it seems from the default discussion that anything below 16 MiB would be useless, wouldn't it? As for the rest - I went and checked a few other VIR_ROUND_UP consumers and they were claiming the 1MiB rounding, but using 1024 instead of 1024*1024 like is done above which I just found "curious" - I didn't bother to check the basis of the value being stored, but I think for memory it is KB.

...

...

+ +    return 0; +} + +

[...]

...

...

+++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> +  <name>QEMUGuest1</name> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> +  <memory unit='KiB'>219100</memory> +  <currentMemory unit='KiB'>219100</currentMemory> +  <vcpu placement='static'>1</vcpu> +  <os> +    <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> +    <boot dev='hd'/> +  </os> +  <features> +    <smm state='on'> +      <tseg>48</tseg>

The only difference here from genericxml2xmlindata/tseg.xml is that this one doesn't have "unit='MiB'"

Yes, I wanted to check for the correct parsing of both.  Maybe I should put 1 GiB in the other one so that I check that it parses the unit properly?  Actually I have KiB somewhere, so probably not.

Considering you commented recently on another series regarding whether the code should use genericxml2xml - I'm just pointing out the IMO excess of having both generic and qemu tests doing pretty much the same thing... You can do what you want though - if both are left there, that's fine.

...

...

+    </smm> +  </features> +  <clock offset='utc'/> +  <on_poweroff>destroy</on_poweroff> +  <on_reboot>restart</on_reboot> +  <on_crash>destroy</on_crash> +  <devices> +    <emulator>/usr/bin/qemu-system-x86_64</emulator> +  </devices> +</domain>

[...]

...

...

--- /dev/null +++ b/tests/qemuxml2argvdata/tseg.xml

and yet another one similr to genericxml2xmlindata/tseg.xml - same name, different directory

Are you suggesting I create this as a symlink?  Or that I should add info to the commit message that this patch adds checks for formatting the command-line (the functionality the patch is adding) and that the genericxml2xmltest is checking the parsing even for developers who build with QEMU driver disabled?

IMO: More duplication of the same test... I'm not the first person to ever point out duplication of tests in commits. "Some day" maybe qemuxml2xmltest will be able to source from genericxml2xml - then it's up to someone to decide which test to use. [...]

...

...

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 78454acb1a41..633dfaaee9a4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2827,6 +2827,54 @@ mymain(void)

[...]

...

...

+    DO_TEST_PARSE_ERROR("tseg-i440fx", +                        QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, +                        QEMU_CAPS_DEVICE_PCI_BRIDGE, +                        QEMU_CAPS_DEVICE_IOH3420, +                        QEMU_CAPS_ICH9_AHCI, +                        QEMU_CAPS_MACHINE_SMM_OPT, +                        QEMU_CAPS_VIRTIO_SCSI, +                        QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); Failure wrong machine type...

...

+    DO_TEST_PARSE_ERROR("tseg-explicit-size", +                        QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, +                        QEMU_CAPS_DEVICE_PCI_BRIDGE, +                        QEMU_CAPS_DEVICE_IOH3420, +                        QEMU_CAPS_ICH9_AHCI, +                        QEMU_CAPS_MACHINE_SMM_OPT, +                        QEMU_CAPS_VIRTIO_SCSI);

Failure because TSEG_MBYTES not provided.

I'm sorry, don't take this personally, but sometimes I'm not sure whether you are trying to convey some information or you are just adding comments for yourself.  If you are asking me to do something, then please say so.  If you want these messages to be there as comments, let me know, I'll do that (although the only time that would be useful is if someone makes a change and the tests start failing (by the tested code not failing) and that person wants to know what was supposed to be the error.  In which case they can re-run them without the modifications. Or if you are suggesting that it should be DO_TEST_FAILURE instead of DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the code doesn't throw an error during starting the domain, but right away during parsing.  And yes, that can be done because I'm not erroring out for something that existed before, but for a new feature so it will never be read from the disk for a domain that existed before this series.  Or maybe I ran the tests wrong and it is supposed to be DO_TEST_FAILURE and I made a mistake somewhere else as well.

The reason for me not really being sure what you mean by that is that I take all of what's in the previous paragraph as something obvious and known by default for non-newbie libvirt developers (which you most certainly are one since you have years of experience in this codebase), so I'm probably just missing something.  It very well might be a language barrier, because I'm nowhere near understanding most of the nuances in English language and various dialects thereof.  So with no hidden meaning between the lines, please enlighten me.

Nothing personal... and valid question/points. There's no hidden meaning - sometimes it's just notes for myself that I forget to go back and remove and other times it's just validation of what's being tested. Part of my review processing is going from the test to the code and considering the error being tested. Just like some people forget to remove debugging logic from their patches before posting, I just forgot to remove my review debugging notes - mea culpa and apologies for any added stress/anxiety ;-). Since Jan had already provided an R-By for the patch - I figured anything I'm providing becomes feedback for you to pick and choose which you find particularly necessary to incorporate and which to ignore. After a few years of getting reviews I've learned the styles of various reviewers and what feedback can be ignored, heeded, and challenged. You've been working on libvirt longer than me, so I'm sure you have certain filters set already too. John [...]