The default is actually `on` when `<smm/>` is specified. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- docs/formatdomain.html.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d0fd3b9f3ea..403b638bd4bd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2052,9 +2052,9 @@ <span class="since">Since 1.2.16</span> </dd> <dt><code>smm</code></dt> - <dd>Enable System Management Mode. Possible values are - <code>on</code> and <code>off</code>. The default is left - for hypervisor to decide. + <dd>Depending on the <code>state</code> attribute (values <code>on</code>, + <code>off</code>, default <code>on</code>) enable or disable + System Management Mode. <span class="since">Since 2.1.0</span> </dd> <dt><code>ioapic</code></dt> -- 2.17.0

We are still hoping all of such checks will be moved there and this is one small step in that direction. One of the things that this is improving is the error message you get when starting a domain with SMM and i440fx, for example. Instead of saying that the QEMU binary doesn't support that option, we correctly say that it is only supported with q35 machine type. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 21 +++++++++++++++------ src/qemu/qemu_capabilities.h | 4 ++-- src/qemu/qemu_command.c | 12 ++---------- src/qemu/qemu_domain.c | 12 +++++++++--- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index bface72de272..ebe35573e7cd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4561,14 +4561,23 @@ virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps, } -bool -virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps, - const virDomainDef *def) +int +virQEMUCapsCheckSMMSupport(virQEMUCapsPtr qemuCaps, + const virDomainDef *def) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT)) - return false; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("smm is not available with this QEMU binary")); + return -1; + } - return qemuDomainIsQ35(def); + if (!qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("smm is only supported with q35 machine type")); + return -1; + } + + return 0; } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 6f9953478a4e..8b6c0c89f4f5 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -497,8 +497,8 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, bool virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps, const virDomainDef *def); -bool virQEMUCapsSupportsSMM(virQEMUCapsPtr qemuCaps, - const virDomainDef *def); +int virQEMUCapsCheckSMMSupport(virQEMUCapsPtr qemuCaps, + const virDomainDef *def); char *virQEMUCapsFlagsString(virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9da2d609e8b7..328f3c0a2386 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7041,16 +7041,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(vmport)); } - if (smm) { - if (!virQEMUCapsSupportsSMM(qemuCaps, def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("smm is not available with this QEMU binary")); - goto cleanup; - } - - virBufferAsprintf(&buf, ",smm=%s", - virTristateSwitchTypeToString(smm)); - } + if (smm) + virBufferAsprintf(&buf, ",smm=%s", virTristateSwitchTypeToString(smm)); if (def->mem.dump_core) { virBufferAsprintf(&buf, ",dump-guest-core=%s", 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 && + 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; -- 2.17.0

On Wed, May 30, 2018 at 11:02:59AM -0400, John Ferlan wrote: 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. 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.

On Thu, May 31, 2018 at 09:33:54AM +0200, Laszlo Ersek wrote: Oh, it definitely helps. And I always enjoy when someone explains low level details of something like you do, it makes me feel very smart after I read it =) ...something about the shoulders of giants and stuff... I'll fix this up according to what you described, that is smm=on/off will be possible to be specified whenever -machine supports smm=.

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(a)redhat.com&gt; --- 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 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 @@ &lt;ioapic driver='qemu'/&gt; &lt;hpt resizing='required'/&gt; &lt;vmcoreinfo state='on'/&gt; + &lt;smm state='on'&gt; + &lt;tseg unit='MiB'&gt;48&lt;/tseg&gt; + &lt;/smm&gt; &lt;/features&gt; ...</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'). + + 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). + From <code>pc-q35-2.10</code> the default value was changed to 16 MiB. + 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. + + 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). + + 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) </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); -- 2.17.0

On Wed, May 30, 2018 at 12:00:26PM -0400, John Ferlan wrote: Rather than promising some value, I adjusted it so that it is no longer false, no matter what the max is there. You can enable/disable 3-way merges if you do (not) like them. 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"). 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 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. This is about tunables. It might change the performance/latency of QEMU slightly, but that's about it. Similarly here, it allows setting something that can be (at least slightly) abstracted and in the worst case the performance will be slightly hindered. 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. 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. 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." Let me know what relevant information from the bz you are missing in the documentation and I'll gladly add it. So? 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 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 :)

On Thu, May 31, 2018 at 11:19:04AM -0400, John Ferlan wrote: Much of what I read (underscore emphasis mine) suggests otherwise: - "There is a lot of sentiment *against* providing too many low level knobs like this _without proper guidance_ on how they should be set." - "To address this issue QEMU implements self-tuning algorithm that modifies the current polling time to _adapt to different workloads_ and it can also fallback to blocking syscalls." - "The QEMU commits say the tunables all default to sane parameters so I'm inclined to say we ignore them at the libvirt level entirely." - "I'm fine if libvirt doesn't add a dedicated API for setting <iothread> polling parameters. It's _unlikely_ that users will need to change the setting. In an emergency (e.g. disabling it due to a performance regression) _they can_ use <qemu:arg value='-newarg'/>." The only points for the polling to be enabled were along the lines of: It _may_ help in _some_ workloads when you want a bit more throughput for the price of more CPU cycles. With vague definitions of how much CPU, throughput and without description of how to find out if a particular workload fits this. Even when all of that is there, then you need yet another explanation on how to calculate the value to be set. And then it all goes down back to the fact that QEMU is already doing some automated balancing for this (because they can, because this is not part of the guest ABI). That way you can never actually say if it will help and how much. So for this one it is a clear "NO". Yeah, you're right. That's why I think both of them should be exposed. Some small differences to other knobs, just for completeness: - by the time you realize that the VM doesn't have enough memory, it might be too late as reboot isn't that easy of a thing for some production workloads - on the other hand, you have a way to see that happening (compare it to the polling interval above which you have no idea without proper benchmarks) One more thing that's common to the memory size (and I hope TSEG in the future) is that in mgmt apps the TSEG setting already has a place where to live and it is exactly where the memory size lives currently. In templates. You have "small vm" teplate and "ginormous vm". For the latter one you can just add a setting of TSEG _once_ per file. How's TSEG better and easier than memory? You figure it once for the VM settings (and possibly firmware, but that's not going to change much) and then it doesn't depend on the workflow, not even a little bit. Anyway. What I see as the differences between tunables that make it in and tunables that don't is that: - the former are usually understandable and easy to see what they are in bare metal. Everyone knows what memory is in the hardware, how it looks like, how much is "not enough" and how much is "more that needed". We are used to those things back from the hardware times, even to changing them. - The latter is usually something we were not able control in HW or didn't even know it existed. For virtual workloads it might be completely different, but sometimes people are forgetting that. This is very similar, it's just that there is no automatic balancing done by QEMU. But it usually is also about how you write the docs. The option can make very much sense, but if someone writes "Setting asdf can allows fine-tuning of the asdf value in the underlying hypervisor", then no matter how much that value makes sense it is not reflected in the docs. That's why I tried to add all the relevant info into the docs so that it's clear what it is doing, how to set it, to what values and when. Apart from the fact that there is a "link" to some file in the QEMU repository that someone is supposed to read, plus the decision for the value determination are written there (but not why they are not automatically calculated, or maybe I missed it), it: - is not possible to try using the <qemu:arg value='-newarg'/> approach - the docs say: <b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> So in this particular case I wouldn't be totally against having it there. If you don't want to use it, then "just don't touch that" is an approach that shouldn't hurt anyone. I thought there was, but maybe I'll add some more info after the discussion. And they would be wrong as for TSEG the hypervisor a) doesn't know that and b) cannot change that once it was started. Here comes the weekend! The part where the VM is set up based on that is only supported in QEMU. The part that does formatting/parsing is done in src/conf/domain_conf.c, which is used by all drivers. I would add it there because it: a) might be later added to other hypervisor drivers b) even when someone is compiling with the qemu driver turned off they cannot break it (provided they do run tests)

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- 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(-) 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'> -- 2.17.0

On Mon, May 21, 2018 at 05:00:53PM +0200, Martin Kletzander wrote: Please use virParseVersionString Interesting way of writing the % operator. 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. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

On Wed, May 30, 2018 at 11:58:54PM +0200, Martin Kletzander wrote: [...] More like very interesting brainfart [1] I had there. I mean I almost open-coded the VIR_ROUND_UP at first and haven't even thought about anything else. Wow. Actually, in the resulting assembly (comparing -O2) it both streches across 5 lines that are very weirdly different/similar. What's way faster is: if (def->tseg_size & ((1<<20) - 1)) which just expands to xorl %eax, %eax andl $1048575, %edi and to make it even more confusing, the previous two approaches share the `andl` line with this one. Scary. [1] And that's very weak word for this particular "thing".

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote: [...] 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. 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:09:46AM +0200, Martin Kletzander wrote: 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. 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. We might need to somehow specify what guest ABI means and what it should cover and what can be left to hypervisor. Pavel

On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote: 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 :). Pavel

On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote: 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. 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. My bad for not commenting the function. But with this line the function is self-sufficient and clearly does what the name suggests (set the default TSEG size) no matter where it is called from and under what circumstances/conditions. That's exactly why I chose that naming (after changing it many, _many_ times). I imagined there will come a time when QEMU adds an option named mch.extended_tseg(_maybe_some_suffix) and the person adding a capability for that will not adjust the naming on this one. I see this as: a) rather safe than sorry b) very much precise with what it describes 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? What patches are you referring to? I can add that to the commit message, sure. 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(); ... } Again, are you directing me to do that or saying what version you like more? 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. 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. 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? OK, good point. 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. Well, I could rant about that for a while. Basically it is because we want to do some modifications during parsing, so not only tests, but also the rest of the code has capabilities already available when parsing the domein. If you want the non-polite version ask me offline, preferably near a beer tap and far from people who are easily offended. Have a nice day, Martin [1] Why don't you have that as a word already? Someone was trying to suggest that: https://www.collinsdictionary.com/submission/4813/alibistic Maybe because some other languages have it already: https://en.wiktionary.org/wiki/alibistick%C3%BD

On Thu, May 31, 2018 at 09:45:04AM -0400, John Ferlan wrote: I feel the same way about most of those things. I have some unfinished patches here and there for adding these to the CG so that we don't have that many subjective discussions. Having said that I don't mind having the discussions. I just don't like having some of the discussions multiple times =) I will learn a new way if there is a clearly written consensus, it's just that I don't want to do that just based on some reviews. Until it is written in stone^Wthe repo, I'm not that convinced. But what is a great plus for me, here and now, is that I understand your review more. I wasn't really sure whether that was optional feedback or a requirement before pushing =) Thanks for the explanation. That's why it is very mandatory to have a healthy communication. See: - commit 8dc88aeed612aeea1ae0d249ec760938cecce5aa - commit bf20251048534022efe21785f98949c772ce7a71 - function qemuDomainGetSCSIControllerModel - Andrea's issues with default GIC version (many commits) - commit f55eaccb0c570767d8245f57deae188255ee995e and I'm sure there's more. Even ones that don't only differentiate on the architecture. I'll see how the discussion goes with Pavel in the other thread, if someone chimes in. This patch is in no rush since it's too close to release anyway, so we'll see. I'm okay with setting it to whatever was the default during the first start (getting the value from QEM) for example. Or maybe leaving the code altogether (even though I must admit it's partially because with this mail history I will be able to say "told you so" in case the day comes like 5 years from now). Yeah, most of the stuff is kept in KiB, I keep it in Bytes. Reason? Extensibility in the future just in case some other hypervisor adds similar option. Also because I imagined that if I kept it in MiB someone would tell me the opposite :) They are not doing the same thing. genericxml2xml is testing XML formatting/parsing in domain_conf. qemuxml2xml is testing the driver-specific postparse bits and qemuxml2argv is testing the command-line generation. Yeah, we can do that. The only advantage, though, would be that we keep less files around. The number of tests would be the same. And we can do that with symlinks nowadays. So probably that's why nobody bothered. That's perfectly fine, I just wasn't ever sure whether I understand it corectly. I'm different in this, I guess. I think the review should be readable for all people the same way, no matter whether you know the person, what they are used to say or how they mean things. That way it's unambiguous. OTOH I'm not saying that I'm doing everything the best way and that my opinions are the best, so it's time for me to learn some new ways! ;) Have a nice day, Martin