[libvirt] [RFC PATCH 0/6] qemu: Support pagesize tuning for pSeries guests
The QEMU part, which is RFC as well, can be found at http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02818.html Applies cleanly on top of c49013f26c6b40b741f4d5fc61269898f7fd25b8. Andrea Bolognani (6): conf: Reintroduce virDomainDef::hpt_resizing conf: Tweak HPT parsing and formatting qemu: Introduce QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS tests: Pretend we have pseries.cap-hpt-mps in 2.12 conf: Parse and format HPT maxpagesize qemu: Format pseries.cap-hpt-mps on the command line docs/schemas/domaincommon.rng | 21 +- src/conf/domain_conf.c | 72 ++++++- src/conf/domain_conf.h | 2 + src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 57 ++++-- src/qemu/qemu_domain.c | 2 +- .../caps_2.12.0.aarch64.replies | 24 ++- .../caps_2.12.0.aarch64.xml | 2 +- .../caps_2.12.0.ppc64.replies | 180 +++++++++++++++++- .../caps_2.12.0.ppc64.xml | 3 +- .../caps_2.12.0.s390x.replies | 26 ++- .../caps_2.12.0.s390x.xml | 2 +- .../caps_2.12.0.x86_64.replies | 30 +-- .../caps_2.12.0.x86_64.xml | 2 +- tests/qemuxml2argvdata/pseries-features.args | 3 +- tests/qemuxml2argvdata/pseries-features.xml | 18 +- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 31 ++- tests/qemuxml2xmltest.c | 1 + 20 files changed, 401 insertions(+), 85 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/pseries-features.xml -- 2.17.0
We're going to introduce a second HPT-related setting soon, at which point using a single location to store everything is no longer going to cut it. This mostly, but not completely, reverts 3dd1eb3b2650. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++++------- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 7 ++++--- src/qemu/qemu_domain.c | 2 +- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2253098090..355e497002 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19489,9 +19489,12 @@ virDomainDefParseXML(xmlDocPtr xml, tmp); goto error; } - def->features[val] = value; + def->hpt_resizing = (virDomainHPTResizing) value; VIR_FREE(tmp); } + + if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) + def->features[val] = VIR_TRISTATE_SWITCH_ON; break; /* coverity[dead_error_begin] */ @@ -21621,13 +21624,16 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, break; case VIR_DOMAIN_FEATURE_HPT: - if (src->features[i] != dst->features[i]) { + if (src->features[i] != dst->features[i] || + src->hpt_resizing != dst->hpt_resizing) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " - "source: '%s=%s', destination: '%s=%s'"), + "source: '%s,%s=%s', destination: '%s,%s=%s'"), featureName, - "resizing", virDomainHPTResizingTypeToString(src->features[i]), - "resizing", virDomainHPTResizingTypeToString(dst->features[i])); + virTristateSwitchTypeToString(src->features[i]), + "resizing", virDomainHPTResizingTypeToString(src->hpt_resizing), + virTristateSwitchTypeToString(dst->features[i]), + "resizing", virDomainHPTResizingTypeToString(dst->hpt_resizing)); return false; } break; @@ -27202,11 +27208,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, break; case VIR_DOMAIN_FEATURE_HPT: - if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE) + if (def->features[i] != VIR_TRISTATE_SWITCH_ON || + def->hpt_resizing == VIR_DOMAIN_HPT_RESIZING_NONE) break; virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString(def->features[i])); + virDomainHPTResizingTypeToString(def->hpt_resizing)); break; /* coverity[dead_error_begin] */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 37356df42d..cb0945b6c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2376,6 +2376,7 @@ struct _virDomainDef { int kvm_features[VIR_DOMAIN_KVM_LAST]; unsigned int hyperv_spinlocks; virGICVersion gic_version; + virDomainHPTResizing hpt_resizing; char *hyperv_vendor_id; int apic_eoi; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cb397c7558..ddbde54a8c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7172,17 +7172,18 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } } - if (def->features[VIR_DOMAIN_FEATURE_HPT] != VIR_DOMAIN_HPT_RESIZING_NONE) { + if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) { const char *str; - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) { + if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("HTP resizing is not supported by this " "QEMU binary")); goto cleanup; } - str = virDomainHPTResizingTypeToString(def->features[VIR_DOMAIN_FEATURE_HPT]); + str = virDomainHPTResizingTypeToString(def->hpt_resizing); if (!str) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Invalid setting for HPT resizing")); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ee676a2789..4bf57b74b6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3623,7 +3623,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) break; case VIR_DOMAIN_FEATURE_HPT: - if (def->features[i] != VIR_DOMAIN_HPT_RESIZING_NONE && + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && !qemuDomainIsPSeries(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The '%s' feature is not supported for " -- 2.17.0
This doesn't seem very useful at the moment, but it will make sense once we introduce another HPT-related setting. The output XML is decoupled from the input XML in preparation of future changes as well; while doing so, we can shave a few lines off the latter. This commit is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 25 ++++++++++++--- src/qemu/qemu_command.c | 32 ++++++++++--------- tests/qemuxml2argvdata/pseries-features.xml | 14 ++------ tests/qemuxml2xmloutdata/pseries-features.xml | 29 ++++++++++++++++- 4 files changed, 68 insertions(+), 32 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/pseries-features.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 355e497002..20b845f02a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27207,14 +27207,31 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainIOAPICTypeToString(def->features[i])); break; - case VIR_DOMAIN_FEATURE_HPT: + case VIR_DOMAIN_FEATURE_HPT: { + bool hasResizing = def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE; + char *resizing = NULL; + if (def->features[i] != VIR_TRISTATE_SWITCH_ON || - def->hpt_resizing == VIR_DOMAIN_HPT_RESIZING_NONE) + !hasResizing) { break; + } + + if (hasResizing) { + if (virAsprintf(&resizing, " resizing='%s'", + virDomainHPTResizingTypeToString(def->hpt_resizing)) < 0) { + goto error; + } + } else { + if (VIR_STRDUP(resizing, "") < 0) + goto error; + } - virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString(def->hpt_resizing)); + virBufferAsprintf(buf, "<hpt%s/>\n", + resizing); + + VIR_FREE(resizing); break; + } /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ddbde54a8c..b446a08613 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7173,24 +7173,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) { - const char *str; - if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("HTP resizing is not supported by this " - "QEMU binary")); - goto cleanup; - } + if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) { + const char *str; - str = virDomainHPTResizingTypeToString(def->hpt_resizing); - if (!str) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Invalid setting for HPT resizing")); - goto cleanup; - } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("HTP resizing is not supported by this " + "QEMU binary")); + goto cleanup; + } + + str = virDomainHPTResizingTypeToString(def->hpt_resizing); + if (!str) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Invalid setting for HPT resizing")); + goto cleanup; + } - virBufferAsprintf(&buf, ",resize-hpt=%s", str); + virBufferAsprintf(&buf, ",resize-hpt=%s", str); + } } if (cpu && cpu->model && diff --git a/tests/qemuxml2argvdata/pseries-features.xml b/tests/qemuxml2argvdata/pseries-features.xml index 5dd0dbd0be..5ef1a744c8 100644 --- a/tests/qemuxml2argvdata/pseries-features.xml +++ b/tests/qemuxml2argvdata/pseries-features.xml @@ -2,27 +2,17 @@ <name>guest</name> <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> <memory unit='KiB'>524288</memory> - <currentMemory unit='KiB'>524288</currentMemory> <vcpu placement='static'>1</vcpu> <os> <type arch='ppc64' machine='pseries'>hvm</type> - <boot dev='hd'/> </os> <features> <hpt resizing='required'/> </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-ppc64</emulator> - <controller type='usb' index='0' model='none'/> - <controller type='pci' index='0' model='pci-root'> - <model name='spapr-pci-host-bridge'/> - <target index='0'/> - </controller> + <controller type='pci' model='pci-root'/> + <controller type='usb' model='none'/> <memballoon model='none'/> - <panic model='pseries'/> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml b/tests/qemuxml2xmloutdata/pseries-features.xml deleted file mode 120000 index 1b01dbace6..0000000000 --- a/tests/qemuxml2xmloutdata/pseries-features.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/pseries-features.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml b/tests/qemuxml2xmloutdata/pseries-features.xml new file mode 100644 index 0000000000..e8ed842fb6 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pseries-features.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <hpt resizing='required'/> + </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-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> -- 2.17.0
On Wed, May 23, 2018 at 18:17:58 +0200, Andrea Bolognani wrote:
This doesn't seem very useful at the moment, but it will make sense once we introduce another HPT-related setting.
The output XML is decoupled from the input XML in preparation of future changes as well; while doing so, we can shave a few lines off the latter.
This commit is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 25 ++++++++++++--- src/qemu/qemu_command.c | 32 ++++++++++--------- tests/qemuxml2argvdata/pseries-features.xml | 14 ++------ tests/qemuxml2xmloutdata/pseries-features.xml | 29 ++++++++++++++++- 4 files changed, 68 insertions(+), 32 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/pseries-features.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 355e497002..20b845f02a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27207,14 +27207,31 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainIOAPICTypeToString(def->features[i])); break;
- case VIR_DOMAIN_FEATURE_HPT: + case VIR_DOMAIN_FEATURE_HPT: { + bool hasResizing = def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE; + char *resizing = NULL; + if (def->features[i] != VIR_TRISTATE_SWITCH_ON || - def->hpt_resizing == VIR_DOMAIN_HPT_RESIZING_NONE) + !hasResizing) { break; + } + + if (hasResizing) { + if (virAsprintf(&resizing, " resizing='%s'", + virDomainHPTResizingTypeToString(def->hpt_resizing)) < 0) { + goto error; + } + } else { + if (VIR_STRDUP(resizing, "") < 0) + goto error; + }
- virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString(def->hpt_resizing)); + virBufferAsprintf(buf, "<hpt%s/>\n",
This formulation looks fishy.
+ resizing); + + VIR_FREE(resizing); break; + }
/* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST:
On Wed, 2018-05-23 at 18:42 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:17:58 +0200, Andrea Bolognani wrote:
+ if (hasResizing) { + if (virAsprintf(&resizing, " resizing='%s'", + virDomainHPTResizingTypeToStri ng(def->hpt_resizing)) < 0) { + goto error; + } + } else { + if (VIR_STRDUP(resizing, "") < 0) + goto error; + }
- virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString (def->hpt_resizing)); + virBufferAsprintf(buf, "<hpt%s/>\n",
This formulation looks fishy.
I don't love it either, but I've tried a bunch of alternative approaches and this seemed like the most sane to me. If you have suggestions on how to improve it, considering that the end result is what you see after patch 5/6, please do share! :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, May 23, 2018 at 18:50:04 +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:42 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:17:58 +0200, Andrea Bolognani wrote:
+ if (hasResizing) { + if (virAsprintf(&resizing, " resizing='%s'", + virDomainHPTResizingTypeToStri ng(def->hpt_resizing)) < 0) { + goto error; + } + } else { + if (VIR_STRDUP(resizing, "") < 0) + goto error; + }
- virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString (def->hpt_resizing)); + virBufferAsprintf(buf, "<hpt%s/>\n",
This formulation looks fishy.
I don't love it either, but I've tried a bunch of alternative approaches and this seemed like the most sane to me.
If you have suggestions on how to improve it, considering that the end result is what you see after patch 5/6, please do share! :)
virXMLFormatElement automatically closes the tag if the provided 'attrBuf' is empty. Currently it will not work for this particular case but I think it is worth to add a version which will format the element even if both buffers are empty.
On Thu, 2018-05-24 at 07:18 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:50:04 +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:42 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:17:58 +0200, Andrea Bolognani wrote:
+ if (hasResizing) { + if (virAsprintf(&resizing, " resizing='%s'", + virDomainHPTResizingTypeToStri ng(def->hpt_resizing)) < 0) { + goto error; + } + } else { + if (VIR_STRDUP(resizing, "") < 0) + goto error; + }
- virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString (def->hpt_resizing)); + virBufferAsprintf(buf, "<hpt%s/>\n",
This formulation looks fishy.
Okay, I think I see now why you called it fishy :) Basically there are three possibilities for how the output will look like: <hpt resizing='x'/> <hpt> <maxpagesize>x</maxpagesize> </hpt> <hpt resizing='x'> <maxpagesize>x</maxpagesize> </hpt> Checks against hasResizing and hasMaxPageSize ensure only the combinations that make sense result in output being produced, so <hpt/> will never happen, but that's not immediately apparent by looking at the snippet above.
I don't love it either, but I've tried a bunch of alternative approaches and this seemed like the most sane to me.
If you have suggestions on how to improve it, considering that the end result is what you see after patch 5/6, please do share! :)
virXMLFormatElement automatically closes the tag if the provided 'attrBuf' is empty. Currently it will not work for this particular case but I think it is worth to add a version which will format the element even if both buffers are empty.
I was not aware of that function, thanks for mentioning it! As explained above, the case where both buffers are empty is not supposed to produce any output, so the existing function already does exactly what I need :) -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 1 + .../caps_2.12.0.aarch64.replies | 24 ++- .../caps_2.12.0.aarch64.xml | 2 +- .../caps_2.12.0.ppc64.replies | 175 +++++++++++++++++- .../caps_2.12.0.ppc64.xml | 2 +- .../caps_2.12.0.s390x.replies | 26 ++- .../caps_2.12.0.s390x.xml | 2 +- .../caps_2.12.0.x86_64.replies | 30 +-- .../caps_2.12.0.x86_64.xml | 2 +- 10 files changed, 233 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8a63db5f4f..ab034b7693 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "screendump_device", "hda-output", "blockdev-del", + "machine.pseries.cap-hpt-mps", ); @@ -1401,10 +1402,17 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[] = { "discard-data", QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD }, }; +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsSPAPRMachine[] = { + { "cap-hpt-mps", QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS }, +}; + static virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { { "memory-backend-file", virQEMUCapsObjectPropsMemoryBackendFile, ARRAY_CARDINALITY(virQEMUCapsObjectPropsMemoryBackendFile), QEMU_CAPS_OBJECT_MEMORY_FILE }, + { "spapr-machine", virQEMUCapsObjectPropsSPAPRMachine, + ARRAY_CARDINALITY(virQEMUCapsObjectPropsSPAPRMachine), + -1 }, }; static void diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3e120e64c0..fecf966f05 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -473,6 +473,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_SCREENDUMP_DEVICE, /* screendump command accepts device & head */ QEMU_CAPS_HDA_OUTPUT, /* -device hda-output */ QEMU_CAPS_BLOCKDEV_DEL, /* blockdev-del is supported */ + QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS, /* -machine pseries,cap-hpt-mps= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies index 3ca0ea13fa..5bcbc3e9b7 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies @@ -5329,6 +5329,14 @@ "id": "libvirt-36" } +{ + "id": "libvirt-37", + "error": { + "class": "DeviceNotFound", + "desc": "Class 'spapr-machine' not found" + } +} + { "return": [ { @@ -5623,7 +5631,7 @@ "cpu-max": 1 } ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -5799,20 +5807,20 @@ "static": false } ], - "id": "libvirt-38" + "id": "libvirt-39" } { "return": [ ], - "id": "libvirt-39" + "id": "libvirt-40" } { "return": [ "emulator" ], - "id": "libvirt-40" + "id": "libvirt-41" } { @@ -6973,7 +6981,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" } { @@ -7035,7 +7043,7 @@ "capability": "dirty-bitmaps" } ], - "id": "libvirt-42" + "id": "libvirt-43" } { @@ -18403,7 +18411,7 @@ "meta-type": "object" } ], - "id": "libvirt-43" + "id": "libvirt-44" } { @@ -18419,7 +18427,7 @@ "kernel": false } ], - "id": "libvirt-44" + "id": "libvirt-45" } { diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index 0dbd354887..6efd4b4147 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -168,7 +168,7 @@ <flag name='blockdev-del'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>343099</microcodeVersion> + <microcodeVersion>343234</microcodeVersion> <package>v2.12.0-rc0</package> <arch>aarch64</arch> <cpu type='kvm' name='pxa262'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies index 1e93cd6dca..e71d69519d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies @@ -5376,6 +5376,167 @@ "id": "libvirt-37" } +{ + "return": [ + { + "name": "graphics", + "description": "Set on/off to enable/disable graphics emulation", + "type": "bool" + }, + { + "name": "phandle-start", + "description": "The first phandle ID we may generate dynamically", + "type": "int" + }, + { + "name": "dump-guest-core", + "description": "Include guest memory in a core dump", + "type": "bool" + }, + { + "name": "kernel-irqchip", + "description": "Configure KVM in-kernel irqchip", + "type": "on|off|split" + }, + { + "name": "accel", + "description": "Accelerator list", + "type": "string" + }, + { + "name": "append", + "description": "Linux kernel command line", + "type": "string" + }, + { + "name": "dumpdtb", + "description": "Dump current dtb to a file and quit", + "type": "string" + }, + { + "name": "memory-encryption", + "description": "Set memory encyption object to use", + "type": "string" + }, + { + "name": "igd-passthru", + "description": "Set on/off to enable/disable igd passthrou", + "type": "bool" + }, + { + "name": "dt-compatible", + "description": "Overrides the \"compatible\" property of the dt root node", + "type": "string" + }, + { + "name": "kernel", + "description": "Linux kernel image file", + "type": "string" + }, + { + "name": "usb", + "description": "Set on/off to enable/disable usb", + "type": "bool" + }, + { + "name": "suppress-vmdesc", + "description": "Set on to disable self-describing migration", + "type": "bool" + }, + { + "name": "dtb", + "description": "Linux kernel device tree file", + "type": "string" + }, + { + "name": "firmware", + "description": "Firmware image", + "type": "string" + }, + { + "name": "mem-merge", + "description": "Enable/disable memory merge support", + "type": "bool" + }, + { + "name": "initrd", + "description": "Linux initial ramdisk file", + "type": "string" + }, + { + "name": "enforce-config-section", + "description": "Set on to enforce configuration section migration", + "type": "bool" + }, + { + "name": "kvm-shadow-mem", + "description": "KVM shadow MMU size", + "type": "int" + }, + { + "name": "cap-ibs", + "description": "Indirect Branch Speculation (broken, fixed-ibs, fixed-ccd)", + "type": "string" + }, + { + "name": "cap-cfpc", + "description": "Cache Flush on Privilege Change (broken, workaround, fixed)", + "type": "string" + }, + { + "name": "cap-sbbc", + "description": "Speculation Barrier Bounds Checking (broken, workaround, fixed)", + "type": "string" + }, + { + "name": "cap-dfp", + "description": "Allow Decimal Floating Point (DFP)", + "type": "bool" + }, + { + "name": "cap-htm", + "description": "Allow Hardware Transactional Memory (HTM)", + "type": "bool" + }, + { + "name": "cap-vsx", + "description": "Allow Vector Scalar Extensions (VSX)", + "type": "bool" + }, + { + "name": "cap-ibs", + "description": "Indirect Branch Speculation (broken, fixed-ibs, fixed-ccd)", + "type": "string" + }, + { + "name": "cap-cfpc", + "description": "Cache Flush on Privilege Change (broken, workaround, fixed)", + "type": "string" + }, + { + "name": "cap-sbbc", + "description": "Speculation Barrier Bounds Checking (broken, workaround, fixed)", + "type": "string" + }, + { + "name": "cap-dfp", + "description": "Allow Decimal Floating Point (DFP)", + "type": "bool" + }, + { + "name": "cap-htm", + "description": "Allow Hardware Transactional Memory (HTM)", + "type": "bool" + }, + { + "name": "cap-vsx", + "description": "Allow Vector Scalar Extensions (VSX)", + "type": "bool" + } + ], + "id": "libvirt-38" +} + { "return": [ { @@ -5511,7 +5672,7 @@ "cpu-max": 1 } ], - "id": "libvirt-38" + "id": "libvirt-39" } { @@ -7707,20 +7868,20 @@ "static": false } ], - "id": "libvirt-39" + "id": "libvirt-40" } { "return": [ ], - "id": "libvirt-40" + "id": "libvirt-41" } { "return": [ "emulator" ], - "id": "libvirt-41" + "id": "libvirt-42" } { @@ -8876,7 +9037,7 @@ "option": "drive" } ], - "id": "libvirt-42" + "id": "libvirt-43" } { @@ -8938,7 +9099,7 @@ "capability": "dirty-bitmaps" } ], - "id": "libvirt-43" + "id": "libvirt-44" } { @@ -20306,7 +20467,7 @@ "meta-type": "object" } ], - "id": "libvirt-44" + "id": "libvirt-45" } { diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 9b08993b7e..eb89c6cd2d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -165,7 +165,7 @@ <flag name='blockdev-del'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>419968</microcodeVersion> + <microcodeVersion>423940</microcodeVersion> <package>v2.12.0-rc0</package> <arch>ppc64</arch> <cpu type='kvm' name='default'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.replies b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.replies index 29c3403550..6591843515 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.replies @@ -3682,6 +3682,14 @@ "id": "libvirt-36" } +{ + "id": "libvirt-37", + "error": { + "class": "DeviceNotFound", + "desc": "Class 'spapr-machine' not found" + } +} + { "return": [ { @@ -3737,7 +3745,7 @@ "alias": "s390-ccw-virtio" } ], - "id": "libvirt-37" + "id": "libvirt-38" } { @@ -4272,20 +4280,20 @@ "migration-safe": true } ], - "id": "libvirt-38" + "id": "libvirt-39" } { "return": [ ], - "id": "libvirt-39" + "id": "libvirt-40" } { "return": [ "emulator" ], - "id": "libvirt-40" + "id": "libvirt-41" } { @@ -5410,7 +5418,7 @@ "option": "drive" } ], - "id": "libvirt-41" + "id": "libvirt-42" } { @@ -5472,7 +5480,7 @@ "capability": "dirty-bitmaps" } ], - "id": "libvirt-42" + "id": "libvirt-43" } { @@ -16840,7 +16848,7 @@ "meta-type": "object" } ], - "id": "libvirt-43" + "id": "libvirt-44" } { @@ -16878,11 +16886,11 @@ } } }, - "id": "libvirt-44" + "id": "libvirt-45" } { - "id": "libvirt-45", + "id": "libvirt-46", "error": { "class": "GenericError", "desc": "Property '.migratable' not found" diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index df0f6e4eba..47aed54958 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -132,7 +132,7 @@ <flag name='blockdev-del'/> <version>2012000</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>371055</microcodeVersion> + <microcodeVersion>371190</microcodeVersion> <package></package> <arch>s390x</arch> <hostCPU type='kvm' model='z14-base' migratability='no'> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies index c40046beef..34cd884f2c 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies @@ -4659,6 +4659,14 @@ "id": "libvirt-40" } +{ + "id": "libvirt-41", + "error": { + "class": "DeviceNotFound", + "desc": "Class 'spapr-machine' not found" + } +} + { "return": [ { @@ -4855,7 +4863,7 @@ "cpu-max": 255 } ], - "id": "libvirt-41" + "id": "libvirt-42" } { @@ -5369,7 +5377,7 @@ "migration-safe": true } ], - "id": "libvirt-42" + "id": "libvirt-43" } { @@ -5377,7 +5385,7 @@ "tpm-crb", "tpm-tis" ], - "id": "libvirt-43" + "id": "libvirt-44" } { @@ -5385,7 +5393,7 @@ "passthrough", "emulator" ], - "id": "libvirt-44" + "id": "libvirt-45" } { @@ -6672,7 +6680,7 @@ "option": "drive" } ], - "id": "libvirt-45" + "id": "libvirt-46" } { @@ -6734,7 +6742,7 @@ "capability": "dirty-bitmaps" } ], - "id": "libvirt-46" + "id": "libvirt-47" } { @@ -18102,7 +18110,7 @@ "meta-type": "object" } ], - "id": "libvirt-47" + "id": "libvirt-48" } { @@ -18292,7 +18300,7 @@ } } }, - "id": "libvirt-48" + "id": "libvirt-49" } { @@ -18547,7 +18555,7 @@ } } }, - "id": "libvirt-49" + "id": "libvirt-50" } { @@ -18737,7 +18745,7 @@ } } }, - "id": "libvirt-50" + "id": "libvirt-51" } { @@ -18992,7 +19000,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 3c7dadffcd..7ebc894a0c 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -206,7 +206,7 @@ <flag name='blockdev-del'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>390813</microcodeVersion> + <microcodeVersion>390948</microcodeVersion> <package>v2.12.0-rc0</package> <arch>x86_64</arch> <hostCPU type='kvm' model='base' migratability='yes'> -- 2.17.0
That's not the case, of course, but the relevant QEMU code has not been merged upstream yet and this is a cheap way to show the capability is actually detected correctly. Do not merge. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 5 +++++ tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies index e71d69519d..55f70fb0a0 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies @@ -5532,6 +5532,11 @@ "name": "cap-vsx", "description": "Allow Vector Scalar Extensions (VSX)", "type": "bool" + }, + { + "name": "cap-hpt-mps", + "description": "Maximum page shift for Hash Page Table guests (12, 16, 24, 34)", + "type": "int" } ], "id": "libvirt-38" diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index eb89c6cd2d..155ed2c246 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -163,9 +163,10 @@ <flag name='screendump_device'/> <flag name='hda-output'/> <flag name='blockdev-del'/> + <flag name='machine.pseries.cap-hpt-mps'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>423940</microcodeVersion> + <microcodeVersion>424089</microcodeVersion> <package>v2.12.0-rc0</package> <arch>ppc64</arch> <cpu type='kvm' name='default'/> -- 2.17.0
This commit is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/schemas/domaincommon.rng | 21 ++++++--- src/conf/domain_conf.c | 44 ++++++++++++++++--- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/pseries-features.xml | 4 +- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 4 +- tests/qemuxml2xmltest.c | 1 + 7 files changed, 60 insertions(+), 16 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f16e157397..7b631f7d93 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5018,13 +5018,20 @@ <define name="hpt"> <element name="hpt"> - <attribute name="resizing"> - <choice> - <value>enabled</value> - <value>disabled</value> - <value>required</value> - </choice> - </attribute> + <optional> + <attribute name="resizing"> + <choice> + <value>enabled</value> + <value>disabled</value> + <value>required</value> + </choice> + </attribute> + </optional> + <optional> + <element name="maxpagesize"> + <ref name='scaledInteger'/> + </element> + </optional> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 20b845f02a..e84cfb0d05 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19493,8 +19493,24 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); } - if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) + if (virDomainParseScaledValue("./features/hpt/maxpagesize", + NULL, + ctxt, + &def->hpt_maxpagesize, + 1024, + ULLONG_MAX, + false) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("Unable to parse HPT maxpagesize setting")); + goto error; + } + def->hpt_maxpagesize = VIR_ROUND_UP(def->hpt_maxpagesize, 1024); + + if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE || + def->hpt_maxpagesize > 0) { def->features[val] = VIR_TRISTATE_SWITCH_ON; + } break; /* coverity[dead_error_begin] */ @@ -21625,15 +21641,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_HPT: if (src->features[i] != dst->features[i] || - src->hpt_resizing != dst->hpt_resizing) { + src->hpt_resizing != dst->hpt_resizing || + src->hpt_maxpagesize != dst->hpt_maxpagesize) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " - "source: '%s,%s=%s', destination: '%s,%s=%s'"), + "source: '%s,%s=%s,%s=%llu', destination: '%s,%s=%s,%s=%llu'"), featureName, virTristateSwitchTypeToString(src->features[i]), "resizing", virDomainHPTResizingTypeToString(src->hpt_resizing), + "maxpagesize", src->hpt_maxpagesize, virTristateSwitchTypeToString(dst->features[i]), - "resizing", virDomainHPTResizingTypeToString(dst->hpt_resizing)); + "resizing", virDomainHPTResizingTypeToString(dst->hpt_resizing), + "maxpagesize", dst->hpt_maxpagesize); return false; } break; @@ -27209,10 +27228,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_HPT: { bool hasResizing = def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE; + bool hasMaxPageSize = def->hpt_maxpagesize > 0; char *resizing = NULL; if (def->features[i] != VIR_TRISTATE_SWITCH_ON || - !hasResizing) { + (!hasResizing && !hasMaxPageSize)) { break; } @@ -27226,8 +27246,18 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto error; } - virBufferAsprintf(buf, "<hpt%s/>\n", - resizing); + if (hasMaxPageSize) { + virBufferAsprintf(buf, "<hpt%s>\n", + resizing); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<maxpagesize unit='KiB'>%llu</maxpagesize>\n", + def->hpt_maxpagesize / 1024); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</hpt>\n"); + } else { + virBufferAsprintf(buf, "<hpt%s/>\n", + resizing); + } VIR_FREE(resizing); break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cb0945b6c0..dbc649347c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2377,6 +2377,7 @@ struct _virDomainDef { unsigned int hyperv_spinlocks; virGICVersion gic_version; virDomainHPTResizing hpt_resizing; + unsigned long long hpt_maxpagesize; char *hyperv_vendor_id; int apic_eoi; diff --git a/tests/qemuxml2argvdata/pseries-features.xml b/tests/qemuxml2argvdata/pseries-features.xml index 5ef1a744c8..30cee5b81c 100644 --- a/tests/qemuxml2argvdata/pseries-features.xml +++ b/tests/qemuxml2argvdata/pseries-features.xml @@ -7,7 +7,9 @@ <type arch='ppc64' machine='pseries'>hvm</type> </os> <features> - <hpt resizing='required'/> + <hpt resizing='required'> + <maxpagesize unit='GiB'>1</maxpagesize> + </hpt> </features> <devices> <emulator>/usr/bin/qemu-system-ppc64</emulator> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1d023129ac..a7392ea655 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1822,6 +1822,7 @@ mymain(void) DO_TEST("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS, 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 e8ed842fb6..f36705f011 100644 --- a/tests/qemuxml2xmloutdata/pseries-features.xml +++ b/tests/qemuxml2xmloutdata/pseries-features.xml @@ -9,7 +9,9 @@ <boot dev='hd'/> </os> <features> - <hpt resizing='required'/> + <hpt resizing='required'> + <maxpagesize unit='KiB'>1048576</maxpagesize> + </hpt> </features> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e31d8212fe..128c14d5d1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -617,6 +617,7 @@ mymain(void) DO_TEST("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); DO_TEST("pseries-serial-native", -- 2.17.0
On Wed, May 23, 2018 at 18:18:01 +0200, Andrea Bolognani wrote:
This commit is best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/schemas/domaincommon.rng | 21 ++++++--- src/conf/domain_conf.c | 44 ++++++++++++++++--- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/pseries-features.xml | 4 +- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/pseries-features.xml | 4 +- tests/qemuxml2xmltest.c | 1 + 7 files changed, 60 insertions(+), 16 deletions(-)
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 20b845f02a..e84cfb0d05 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19493,8 +19493,24 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); }
- if (def->hpt_resizing != VIR_DOMAIN_HPT_RESIZING_NONE) + if (virDomainParseScaledValue("./features/hpt/maxpagesize", + NULL, + ctxt, + &def->hpt_maxpagesize, + 1024, + ULLONG_MAX, + false) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("Unable to parse HPT maxpagesize setting")); + goto error; + } + def->hpt_maxpagesize = VIR_ROUND_UP(def->hpt_maxpagesize, 1024);
The code in the patch using it with qemu actually expects so this is a power of 2, so you'll need a different rounding function.
On Wed, 2018-05-23 at 18:36 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:18:01 +0200, Andrea Bolognani wrote:
+ def->hpt_maxpagesize = VIR_ROUND_UP(def->hpt_maxpagesize, 1024);
The code in the patch using it with qemu actually expects so this is a power of 2, so you'll need a different rounding function.
Good point! Looks like VIR_ROUND_UP_POWER_OF_TWO() will do the trick. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, May 23, 2018 at 18:52:57 +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:36 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:18:01 +0200, Andrea Bolognani wrote:
+ def->hpt_maxpagesize = VIR_ROUND_UP(def->hpt_maxpagesize, 1024);
The code in the patch using it with qemu actually expects so this is a power of 2, so you'll need a different rounding function.
Good point!
Looks like VIR_ROUND_UP_POWER_OF_TWO() will do the trick.
Yes. We usually do the rounding in the code which prepares the definition for the startup of the VM though, so that other hypervisor drivers don't need to share the limits of qemu. Given that this is a ppc64 specific option it probably does not matter much.
This makes the feature fully functional. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1571078 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 26 ++++++++++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 3 ++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b446a08613..983839e81c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7193,6 +7193,32 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virBufferAsprintf(&buf, ",resize-hpt=%s", str); } + + if (def->hpt_maxpagesize > 0) { + unsigned long long tmp = def->hpt_maxpagesize; + unsigned int shifts = 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Limiting the page size for HPT guest is " + "not supported by this QEMU binary")); + goto cleanup; + } + + /* QEMU expects the argument to be a number of left shifts: + * for example, if you wanted to limit the guest to 4 KiB pages, + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12 + * to the command line. + * + * Convert from our internal representation, which is bytes, + * to the one QEMU expects */ + while (tmp > 1) { + tmp = tmp >> 1; + shifts++; + } + + virBufferAsprintf(&buf, ",cap-hpt-mps=%u", shifts); + } } if (cpu && cpu->model && diff --git a/tests/qemuxml2argvdata/pseries-features.args b/tests/qemuxml2argvdata/pseries-features.args index f5c1959cca..4e581a69a1 100644 --- a/tests/qemuxml2argvdata/pseries-features.args +++ b/tests/qemuxml2argvdata/pseries-features.args @@ -7,7 +7,8 @@ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-ppc64 \ -name guest \ -S \ --machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required \ +-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\ +cap-hpt-mps=30 \ -m 512 \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ -- 2.17.0
On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
This makes the feature fully functional.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1571078
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 26 ++++++++++++++++++++ tests/qemuxml2argvdata/pseries-features.args | 3 ++- 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b446a08613..983839e81c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7193,6 +7193,32 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(&buf, ",resize-hpt=%s", str); } + + if (def->hpt_maxpagesize > 0) { + unsigned long long tmp = def->hpt_maxpagesize; + unsigned int shifts = 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Limiting the page size for HPT guest is " + "not supported by this QEMU binary")); + goto cleanup; + } + + /* QEMU expects the argument to be a number of left shifts: + * for example, if you wanted to limit the guest to 4 KiB pages, + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12 + * to the command line.
So basically you need to pass the exponent of a power of 2 that yields this number. The number of left shifts may be slightly confusing ...
+ * + * Convert from our internal representation, which is bytes, + * to the one QEMU expects */ + while (tmp > 1) { + tmp = tmp >> 1; + shifts++; + } + + virBufferAsprintf(&buf, ",cap-hpt-mps=%u", shifts); + } }
if (cpu && cpu->model && diff --git a/tests/qemuxml2argvdata/pseries-features.args b/tests/qemuxml2argvdata/pseries-features.args index f5c1959cca..4e581a69a1 100644 --- a/tests/qemuxml2argvdata/pseries-features.args +++ b/tests/qemuxml2argvdata/pseries-features.args @@ -7,7 +7,8 @@ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-ppc64 \ -name guest \ -S \ --machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required \ +-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\ +cap-hpt-mps=30 \ -m 512 \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ -- 2.17.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
+ /* QEMU expects the argument to be a number of left shifts: + * for example, if you wanted to limit the guest to 4 KiB pages, + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12 + * to the command line.
So basically you need to pass the exponent of a power of 2 that yields this number. The number of left shifts may be slightly confusing ...
I guess it depends on the reader; the two definitions are equivalent anyway, so no harm in having both in the comment :) In general, I'd say it's not the most user-friendly interface on QEMU's side, but I believe it's dictated by hardware / emulator details, given how it ends up being used: see http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html To be fair, it would perhaps make sense to perform the conversion directly inside QEMU, in order to make it more convenient not only for libvirt but for for people driving it directly as well. CC'ing David so that he can weigh in on the idea. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
+ /* QEMU expects the argument to be a number of left shifts: + * for example, if you wanted to limit the guest to 4 KiB pages, + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12 + * to the command line.
So basically you need to pass the exponent of a power of 2 that yields this number. The number of left shifts may be slightly confusing ...
I guess it depends on the reader; the two definitions are equivalent anyway, so no harm in having both in the comment :)
Using both are fine, but without mentioning that it is in fact a power of two-only thing I was thinking that the interface is off by one or something: 4096 == 1 << 12 == 2 << 11
In general, I'd say it's not the most user-friendly interface on QEMU's side, but I believe it's dictated by hardware / emulator details, given how it ends up being used: see
http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html
To be fair, it would perhaps make sense to perform the conversion directly inside QEMU, in order to make it more convenient not only for libvirt but for for people driving it directly as well.
If strictly only powers of two make sense for this knob then this gives you input validation for free. On the other hand, specifying a big number can overflow internally if it is ever used in the non-exponent form. I think the format does not matter much, since libvirt's job is to shield users from such weirdness.
On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
+ /* QEMU expects the argument to be a number of left shifts: + * for example, if you wanted to limit the guest to 4 KiB pages, + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12 + * to the command line.
So basically you need to pass the exponent of a power of 2 that yields this number. The number of left shifts may be slightly confusing ...
I guess it depends on the reader; the two definitions are equivalent anyway, so no harm in having both in the comment :)
Using both are fine, but without mentioning that it is in fact a power of two-only thing I was thinking that the interface is off by one or something:
4096 == 1 << 12 == 2 << 11
In general, I'd say it's not the most user-friendly interface on QEMU's side, but I believe it's dictated by hardware / emulator details, given how it ends up being used: see
http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html
To be fair, it would perhaps make sense to perform the conversion directly inside QEMU, in order to make it more convenient not only for libvirt but for for people driving it directly as well.
If strictly only powers of two make sense for this knob then this gives you input validation for free. On the other hand, specifying a big number can overflow internally if it is ever used in the non-exponent form. I think the format does not matter much, since libvirt's job is to shield users from such weirdness.
Yeah, the above is basically my reasoning for using the exponent, not the final page size. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Mon, 2018-06-04 at 11:33 +1000, David Gibson wrote:
On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
To be fair, it would perhaps make sense to perform the conversion directly inside QEMU, in order to make it more convenient not only for libvirt but for for people driving it directly as well.
If strictly only powers of two make sense for this knob then this gives you input validation for free. On the other hand, specifying a big number can overflow internally if it is ever used in the non-exponent form. I think the format does not matter much, since libvirt's job is to shield users from such weirdness.
Yeah, the above is basically my reasoning for using the exponent, not the final page size.
As Peter mentioned, what format is used doesn't matter much from libvirt's point of view, and in fact this RFC already implements the necessary format conversion; however, it might be convenient for people spawning QEMU directly to be able to specify the page size in a more human-friendly format. How do you feel about that? If that's off the table, we'll just go ahead with the current implementation. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Jun 05, 2018 at 04:05:38PM +0200, Andrea Bolognani wrote:
On Mon, 2018-06-04 at 11:33 +1000, David Gibson wrote:
On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
To be fair, it would perhaps make sense to perform the conversion directly inside QEMU, in order to make it more convenient not only for libvirt but for for people driving it directly as well.
If strictly only powers of two make sense for this knob then this gives you input validation for free. On the other hand, specifying a big number can overflow internally if it is ever used in the non-exponent form. I think the format does not matter much, since libvirt's job is to shield users from such weirdness.
Yeah, the above is basically my reasoning for using the exponent, not the final page size.
As Peter mentioned, what format is used doesn't matter much from libvirt's point of view, and in fact this RFC already implements the necessary format conversion; however, it might be convenient for people spawning QEMU directly to be able to specify the page size in a more human-friendly format.
How do you feel about that? If that's off the table, we'll just go ahead with the current implementation.
So, I need to keep the internal representation as a shift, since we only have 8 bits in the capability field. But that doesn't prevent changing the command line representation, since we could convert in the getter/setter functions. Personally I think the shift is *more* usable than a raw page size, since the latter is inevitably going to involve counting a bunch of zeroes to see if it's the number you meant. Allowing forms like "16M" / "16G" could be nicer; not sure if we have existing helper functions which will make that easy. TBH, if the user is already thinking about page sizes at this low level, I don't think doing it by shift is much of a stretch. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
Personally I think the shift is *more* usable than a raw page size, since the latter is inevitably going to involve counting a bunch of zeroes to see if it's the number you meant. Allowing forms like "16M" / "16G" could be nicer; not sure if we have existing helper functions which will make that easy.
I was indeed thinking about the latter.
TBH, if the user is already thinking about page sizes at this low level, I don't think doing it by shift is much of a stretch.
I disagree that it's such a low level setting that only people who can do bitwise shifts off the top of their heads will want to tweak: if it were, then libvirt will probably not need to expose it in the first place :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jun 06, 2018 at 12:02:12 +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
Personally I think the shift is *more* usable than a raw page size, since the latter is inevitably going to involve counting a bunch of zeroes to see if it's the number you meant. Allowing forms like "16M" / "16G" could be nicer; not sure if we have existing helper functions which will make that easy.
I was indeed thinking about the latter.
TBH, if the user is already thinking about page sizes at this low level, I don't think doing it by shift is much of a stretch.
I disagree that it's such a low level setting that only people who can do bitwise shifts off the top of their heads will want to tweak: if it were, then libvirt will probably not need to expose it in the first place :)
Well, it will most probably be configured by some policy code from a upper layer mgmt tool, so I don't think users will actually ever touch it.
On Wed, 2018-06-06 at 12:04 +0200, Peter Krempa wrote:
On Wed, Jun 06, 2018 at 12:02:12 +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
TBH, if the user is already thinking about page sizes at this low level, I don't think doing it by shift is much of a stretch.
I disagree that it's such a low level setting that only people who can do bitwise shifts off the top of their heads will want to tweak: if it were, then libvirt will probably not need to expose it in the first place :)
Well, it will most probably be configured by some policy code from a upper layer mgmt tool, so I don't think users will actually ever touch it.
Probably. Still not a good reason not to ensure it's human-friendly at every layer of the stack IMHO. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
David Gibson -
Peter Krempa