[libvirt] [PATCH v3 0/3] Implement the HTM pSeries feature
Applies cleanly on top of aa51063927a0d48768ff88b11f12075ad5efc89f. Changes from [v2]: * rebase on top of master; the series is much smaller now. Changes from [v1]: * drop qom-list-properties machinery, as equivalent functionality has been merged in the meantime; * don't introduce scaffolding for uniform machine option handling as a requirement for implementing this specific feature: we can do ad-hoc processing for now, per the status quo, then clean up everything (including existing features) with a separate series later. Changes from [RFC v3]: * can be considered for merging, since the QEMU part already has; * fix compatibility issues with QEMU <2.12 spotted by Shivaprasad; * drop all features except for HTM, at least for the time being; * add documentation. Changes from [RFC v2]: * use qom-list-properties to probe availability; * test all features with a single XML file. Changes from [RFC v1]: * don't nest features inside a <pseries/> element; * implement all optional features. [v2] https://www.redhat.com/archives/libvir-list/2018-June/msg01374.html [v1] https://www.redhat.com/archives/libvir-list/2018-March/msg00474.html [RFC v3] https://www.redhat.com/archives/libvir-list/2018-March/msg00042.html [RFC v2] https://www.redhat.com/archives/libvir-list/2018-February/msg00310.html [RFC v1] https://www.redhat.com/archives/libvir-list/2018-January/msg00779.html Andrea Bolognani (3): qemu: Add capability for the HTM pSeries feature qemu: Implement the HTM pSeries feature news: Update for the HTM pSeries feature docs/formatdomain.html.in | 8 ++++++++ docs/news.xml | 9 +++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 19 ++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 20 +++++++++++++++++++ src/qemu/qemu_domain.c | 13 ++++++++++++ .../caps_2.12.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + tests/qemuxml2argvdata/pseries-features.args | 2 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 1 + tests/qemuxml2xmltest.c | 1 + 16 files changed, 85 insertions(+), 1 deletion(-) -- 2.17.1
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + 4 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 37c8fbe3d3..c7da916f9a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -501,6 +501,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 310 */ "sev-guest", "machine.pseries.cap-hpt-max-page-size", + "machine.pseries.cap-htm", ); @@ -1431,6 +1432,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[] = static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsSPAPRMachine[] = { { "cap-hpt-max-page-size", QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE }, + { "cap-htm", QEMU_CAPS_MACHINE_PSERIES_CAP_HTM }, }; static virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index df9bf49abb..a048a1cf02 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -485,6 +485,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 310 */ QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */ QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, /* -machine pseries.cap-hpt-max-page-size */ + QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, /* -machine pseries.cap-htm */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 2ee582f343..7139179304 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -166,6 +166,7 @@ <flag name='vhost-vsock'/> <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> + <flag name='machine.pseries.cap-htm'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>428334</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml index 7e958b2efc..33cd00e613 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml @@ -166,6 +166,7 @@ <flag name='chardev-fd-pass'/> <flag name='tpm-emulator'/> <flag name='machine.pseries.cap-hpt-max-page-size'/> + <flag name='machine.pseries.cap-htm'/> <version>2012050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>446771</microcodeVersion> -- 2.17.1
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + 4 files changed, 5 insertions(+)
Weird to see having htm supported after the max page size - chicken and egg type problem. I mean how do you have hpt without having max page size. Anyway, just one of life's ironies I suppose. I would think just having page size is enough, but what do I really know about pSeries, not much ;-). If that's really not the case, well then fine separate caps it is. Reviewed-by: John Ferlan <jferlan@redhat.com> John BTW: Although it is a bz, it's "debatable" if you're fixing a bug or enabling a feature during freeze even though it was posted before freeze, so this I believe should wait for 4.6 to open.
On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
Weird to see having htm supported after the max page size - chicken and egg type problem. I mean how do you have hpt without having max page size. Anyway, just one of life's ironies I suppose. I would think just having page size is enough, but what do I really know about pSeries, not much ;-). If that's really not the case, well then fine separate caps it is.
Note that this is about HTM, not HPT - that's a completely different TLA! The two features are not related in any way.
BTW: Although it is a bz, it's "debatable" if you're fixing a bug or enabling a feature during freeze even though it was posted before freeze, so this I believe should wait for 4.6 to open.
Sure, I would never have pushed this during the freeze :) -- Andrea Bolognani / Red Hat / Virtualization
https://bugzilla.redhat.com/show_bug.cgi?id=1525599 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 19 ++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 20 +++++++++++++++++++ src/qemu/qemu_domain.c | 13 ++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 2 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 71 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d68596991..73db315c4f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1929,6 +1929,7 @@ <smm state='on'> <tseg unit='MiB'>48</tseg> </smm> + <htm state='on'/> </features> ...</pre> @@ -2162,6 +2163,13 @@ <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug details. <span class="since">Since 4.4.0</span> (QEMU only) </dd> + <dt><code>htm</code></dt> + <dd>Configure HTM (Hardware Transational Memory) availability for + pSeries guests. Possible values for the <code>state</code> attribute + are <code>on</code> and <code>off</code>. If the attribute is not + defined, the hypervisor default will be used. + <span class="since">Since 4.5.0</span> (QEMU/KVM only) + </dd> </dl> <h3><a id="elementsTime">Time keeping</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 450e8ac77b..3dd1bd7974 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4906,6 +4906,11 @@ <optional> <ref name="vmcoreinfo"/> </optional> + <optional> + <element name="htm"> + <ref name="featurestate"/> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8cb7f37f3..ea01d08a3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "ioapic", "hpt", "vmcoreinfo", + "htm", ); VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, @@ -19827,6 +19828,22 @@ virDomainDefParseXML(xmlDocPtr xml, } break; + case VIR_DOMAIN_FEATURE_HTM: + if (!(tmp = virXMLPropString(nodes[i], "state"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + VIR_FREE(tmp); + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; @@ -21961,6 +21978,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_SMM: case VIR_DOMAIN_FEATURE_VMCOREINFO: + case VIR_DOMAIN_FEATURE_HTM: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " @@ -27626,6 +27644,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_VMPORT: + case VIR_DOMAIN_FEATURE_HTM: switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_LAST: case VIR_TRISTATE_SWITCH_ABSENT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0924fc4f3c..a41431ccf9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1771,6 +1771,7 @@ typedef enum { VIR_DOMAIN_FEATURE_IOAPIC, VIR_DOMAIN_FEATURE_HPT, VIR_DOMAIN_FEATURE_VMCOREINFO, + VIR_DOMAIN_FEATURE_HTM, VIR_DOMAIN_FEATURE_LAST } virDomainFeature; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a357d2199c..5342c08f19 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7291,6 +7291,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } } + if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) { + const char *str; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("HTM configuration is not supported by this " + "QEMU binary")); + goto cleanup; + } + + str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]); + if (!str) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Invalid setting for HTM state")); + goto cleanup; + } + + virBufferAsprintf(&buf, ",cap-htm=%s", str); + } + if (cpu && cpu->model && cpu->mode == VIR_CPU_MODE_HOST_MODEL && qemuDomainIsPSeries(def) && diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6d203e1f2e..baf11b4532 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3846,6 +3846,19 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, } break; + case VIR_DOMAIN_FEATURE_HTM: + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is not supported for " + "architecture '%s' or machine type '%s'"), + featureName, + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_APIC: case VIR_DOMAIN_FEATURE_PAE: diff --git a/tests/qemuxml2argvdata/pseries-features.args b/tests/qemuxml2argvdata/pseries-features.args index 12c14715c6..226d43df44 100644 --- a/tests/qemuxml2argvdata/pseries-features.args +++ b/tests/qemuxml2argvdata/pseries-features.args @@ -8,7 +8,7 @@ QEMU_AUDIO_DRV=none \ -name guest \ -S \ -machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\ -cap-hpt-max-page-size=1048576k \ +cap-hpt-max-page-size=1048576k,cap-htm=on \ -m 512 \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ diff --git a/tests/qemuxml2argvdata/pseries-features.xml b/tests/qemuxml2argvdata/pseries-features.xml index 30cee5b81c..5c842fe87b 100644 --- a/tests/qemuxml2argvdata/pseries-features.xml +++ b/tests/qemuxml2argvdata/pseries-features.xml @@ -10,6 +10,7 @@ <hpt resizing='required'> <maxpagesize unit='GiB'>1</maxpagesize> </hpt> + <htm state='on'/> </features> <devices> <emulator>/usr/bin/qemu-system-ppc64</emulator> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ac9593acbe..e64813e430 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1850,6 +1850,7 @@ mymain(void) DO_TEST("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, + QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); DO_TEST_FAILURE("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml b/tests/qemuxml2xmloutdata/pseries-features.xml index f36705f011..55a44c75a0 100644 --- a/tests/qemuxml2xmloutdata/pseries-features.xml +++ b/tests/qemuxml2xmloutdata/pseries-features.xml @@ -12,6 +12,7 @@ <hpt resizing='required'> <maxpagesize unit='KiB'>1048576</maxpagesize> </hpt> + <htm state='on'/> </features> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index eac6d5b073..bbb995656e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -620,6 +620,7 @@ mymain(void) DO_TEST("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, + QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); DO_TEST("pseries-serial-native", -- 2.17.1
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1525599 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 19 ++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 20 +++++++++++++++++++ src/qemu/qemu_domain.c | 13 ++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 2 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 71 insertions(+), 1 deletion(-)
Even though it's QEMU/KVM only - the conf/docs/xml2xml should be separated from the qemu/xml2argv changes. [...]
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0d68596991..73db315c4f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1929,6 +1929,7 @@ <smm state='on'> <tseg unit='MiB'>48</tseg> </smm> + <htm state='on'/> </features> ...</pre>
@@ -2162,6 +2163,13 @@ <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug details. <span class="since">Since 4.4.0</span> (QEMU only) </dd> + <dt><code>htm</code></dt> + <dd>Configure HTM (Hardware Transational Memory) availability for + pSeries guests. Possible values for the <code>state</code> attribute + are <code>on</code> and <code>off</code>. If the attribute is not + defined, the hypervisor default will be used. + <span class="since">Since 4.5.0</span> (QEMU/KVM only)
This'll miss 4.5, so change to 4.6 before pushing
+ </dd> </dl>
<h3><a id="elementsTime">Time keeping</a></h3>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8cb7f37f3..ea01d08a3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "ioapic", "hpt", "vmcoreinfo", + "htm", );
VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, @@ -19827,6 +19828,22 @@ virDomainDefParseXML(xmlDocPtr xml, } break;
+ case VIR_DOMAIN_FEATURE_HTM: + if (!(tmp = virXMLPropString(nodes[i], "state"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
[1] checked here, so that...
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + VIR_FREE(tmp); + break; +
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a357d2199c..5342c08f19 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7291,6 +7291,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } }
+ if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) { + const char *str; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("HTM configuration is not supported by this " + "QEMU binary")); + goto cleanup; + } + + str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]); + if (!str) {
[1] ... this is unnecessary due to virDomainDefParseXML check.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Invalid setting for HTM state")); + goto cleanup; + } + + virBufferAsprintf(&buf, ",cap-htm=%s", str);
Just remove @str, and directly call/use: virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM])
+ } + if (cpu && cpu->model && cpu->mode == VIR_CPU_MODE_HOST_MODEL && qemuDomainIsPSeries(def) &&
[...] with the adjustments and splitting into two patches (I trust you have that capability), Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1525599 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 19 ++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 20 +++++++++++++++++++ src/qemu/qemu_domain.c | 13 ++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 2 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 71 insertions(+), 1 deletion(-)
Even though it's QEMU/KVM only - the conf/docs/xml2xml should be separated from the qemu/xml2argv changes.
Can do.
@@ -2162,6 +2163,13 @@ <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug details. <span class="since">Since 4.4.0</span> (QEMU only) </dd> + <dt><code>htm</code></dt> + <dd>Configure HTM (Hardware Transational Memory) availability for + pSeries guests. Possible values for the <code>state</code> attribute + are <code>on</code> and <code>off</code>. If the attribute is not + defined, the hypervisor default will be used. + <span class="since">Since 4.5.0</span> (QEMU/KVM only)
This'll miss 4.5, so change to 4.6 before pushing
Sure.
+ case VIR_DOMAIN_FEATURE_HTM: + if (!(tmp = virXMLPropString(nodes[i], "state"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
[1] checked here, so that...
+ if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) { + const char *str; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("HTM configuration is not supported by this " + "QEMU binary")); + goto cleanup; + } + + str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]); + if (!str) {
[1] ... this is unnecessary due to virDomainDefParseXML check.
I strongly dislike the practice of skipping checks in a function with the rationale that they're already performed somewhere else in the code base: libvirt is in a state of constant flux, and as stuff gets moved around you might very well find yourself no longer as shielded from invalid values as you thought you were. Local sanity checks should IMHO always be performed locally. tl;dr I'd really rather keep this in. -- Andrea Bolognani / Red Hat / Virtualization
On 07/02/2018 04:19 AM, Andrea Bolognani wrote:
On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1525599 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 19 ++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 20 +++++++++++++++++++ src/qemu/qemu_domain.c | 13 ++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 2 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 71 insertions(+), 1 deletion(-)
Even though it's QEMU/KVM only - the conf/docs/xml2xml should be separated from the qemu/xml2argv changes.
Can do.
@@ -2162,6 +2163,13 @@ <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug details. <span class="since">Since 4.4.0</span> (QEMU only) </dd> + <dt><code>htm</code></dt> + <dd>Configure HTM (Hardware Transational Memory) availability for + pSeries guests. Possible values for the <code>state</code> attribute + are <code>on</code> and <code>off</code>. If the attribute is not + defined, the hypervisor default will be used. + <span class="since">Since 4.5.0</span> (QEMU/KVM only)
This'll miss 4.5, so change to 4.6 before pushing
Sure.
+ case VIR_DOMAIN_FEATURE_HTM: + if (!(tmp = virXMLPropString(nodes[i], "state"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing state attribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
[1] checked here, so that...
+ if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) { + const char *str; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("HTM configuration is not supported by this " + "QEMU binary")); + goto cleanup; + } + + str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]); + if (!str) {
[1] ... this is unnecessary due to virDomainDefParseXML check.
I strongly dislike the practice of skipping checks in a function with the rationale that they're already performed somewhere else in the code base: libvirt is in a state of constant flux, and as stuff gets moved around you might very well find yourself no longer as shielded from invalid values as you thought you were. Local sanity checks should IMHO always be performed locally.
tl;dr I'd really rather keep this in.
That's fine - I don't mind the check, just pointing out it should be unnecessary. You can leave it in and the R-by still stands. John htm vs. hpt, yep the tla's and seeing pseries with memory related functionality for both certainly led me down that path...
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index a32d2b88a5..51e09a8737 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -105,6 +105,15 @@ guest. </description> </change> + <change> + <summary> + qemu: Implement the HTM pSeries feature + </summary> + <description> + Users can now decide whether HTM (Hardware Transactional Memory) + support should be available to the guest. + </description> + </change> </section> <section title="Bug fixes"> <change> -- 2.17.1
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+)
Obviously this moves with the new release, but the text is fine. Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
Andrea Bolognani -
John Ferlan