5:21 a.m.
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
5:21 a.m.
New subject: [libvirt] [PATCH v3 1/3] qemu: Add capability for the HTM pSeries feature
Signed-off-by: Andrea Bolognani <abologna(a)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
6:23 a.m.
New subject: [libvirt] [PATCH v3 1/3] qemu: Add capability for the HTM pSeries feature
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna(a)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(a)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.
3:04 a.m.
New subject: [libvirt] [PATCH v3 1/3] qemu: Add capability for the HTM pSeries feature
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
5:21 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
https://bugzilla.redhat.com/show_bug.cgi?id=1525599
Signed-off-by: Andrea Bolognani <abologna(a)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
6:23 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
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(a)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(a)redhat.com>
John
3:19 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
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(a)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
7:12 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
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(a)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...
5:21 a.m.
New subject: [libvirt] [PATCH v3 3/3] news: Update for the HTM pSeries feature
Signed-off-by: Andrea Bolognani <abologna(a)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
6:24 a.m.
New subject: [libvirt] [PATCH v3 3/3] news: Update for the HTM pSeries feature
On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna(a)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(a)redhat.com>
John
2337
days inactive
2343
days old
9 comments
2 participants
participants (2)
-
Andrea Bolognani -
John Ferlan