[PATCH v2 0/3] improve pSeries NVDIMM support
Hi, This is a follow up of [1] after Andrea pushed the revert patch as standalone. Changes from v1: - patch 2 (former 3): moved the auto-align code from virDomainMemoryDefParseXML() to virDomainMemoryDefPostParse() - patch 3 (former 4): updated NEWS.rst based on Andrea's changes already upstream [1] https://www.redhat.com/archives/libvir-list/2020-September/msg00864.html Daniel Henrique Barboza (3): conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse() NEWS.rst: update NVDIMM changes entry NEWS.rst | 3 +- src/conf/domain_conf.c | 59 ++++++++++++++++++- src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 42 +------------ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- 6 files changed, 67 insertions(+), 43 deletions(-) -- 2.26.2
We'll use the auto-alignment function during parse time, in domain_conf.c. Let's move the function to that file, renaming it to virDomainNVDimmAlignSizePseries(). This will also make it clearer that, although QEMU is the only driver that currently supports it, pSeries NVDIMM restrictions aren't tied to QEMU. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 42 ++-------------------------------------- 4 files changed, 42 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9289c147fe..ea6a097161 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16866,6 +16866,42 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, return NULL; } +int +virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem) +{ + /* For NVDIMMs in ppc64 in we want to align down the guest + * visible space, instead of align up, to avoid writing + * beyond the end of file by adding a potential 256MiB + * to the user specified size. + * + * The label-size is mandatory for ppc64 as well, meaning that + * the guest visible space will be target_size-label_size. + * + * Finally, target_size must include label_size. + * + * The above can be summed up as follows: + * + * target_size = AlignDown(target_size - label_size) + label_size + */ + unsigned long long ppc64AlignSize = 256 * 1024; + unsigned long long guestArea = mem->size - mem->labelsize; + + /* Align down guest_area. 256MiB is the minimum size. Error + * out if target_size is smaller than 256MiB + label_size, + * since aligning it up will cause QEMU errors. */ + if (mem->size < (ppc64AlignSize + mem->labelsize)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("minimum target size for the NVDIMM " + "must be 256MB plus the label size")); + return -1; + } + + guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; + mem->size = guestArea + mem->labelsize; + + return 0; +} + static virDomainMemoryDefPtr virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr memdevNode, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4724206828..8f1662aae0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3904,6 +3904,9 @@ bool virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, const virDomainBlockIoTuneInfo *b); +int +virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem); + bool virHostdevIsSCSIDevice(const virDomainHostdevDef *hostdev) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bdbe3431b8..6bca87eede 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -543,6 +543,7 @@ virDomainNetTypeToString; virDomainNetUpdate; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; +virDomainNVDimmAlignSizePseries; virDomainObjAssignDef; virDomainObjBroadcast; virDomainObjCheckActive; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9e0d3a15b2..9fa6fac99a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8067,44 +8067,6 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def, } -static int -qemuDomainNVDimmAlignSizePseries(virDomainDefPtr def, - virDomainMemoryDefPtr mem) -{ - /* For NVDIMMs in ppc64 in we want to align down the guest - * visible space, instead of align up, to avoid writing - * beyond the end of file by adding a potential 256MiB - * to the user specified size. - * - * The label-size is mandatory for ppc64 as well, meaning that - * the guest visible space will be target_size-label_size. - * - * Finally, target_size must include label_size. - * - * The above can be summed up as follows: - * - * target_size = AlignDown(target_size - label_size) + label_size - */ - unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def); - unsigned long long guestArea = mem->size - mem->labelsize; - - /* Align down guest_area. 256MiB is the minimum size. Error - * out if target_size is smaller than 256MiB + label_size, - * since aligning it up will cause QEMU errors. */ - if (mem->size < (ppc64AlignSize + mem->labelsize)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("minimum target size for the NVDIMM " - "must be 256MB plus the label size")); - return -1; - } - - guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; - mem->size = guestArea + mem->labelsize; - - return 0; -} - - int qemuDomainAlignMemorySizes(virDomainDefPtr def) { @@ -8153,7 +8115,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) for (i = 0; i < def->nmems; i++) { if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - if (qemuDomainNVDimmAlignSizePseries(def, def->mems[i]) < 0) + if (virDomainNVDimmAlignSizePseries(def->mems[i]) < 0) return -1; } else { align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); @@ -8190,7 +8152,7 @@ qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, { if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - return qemuDomainNVDimmAlignSizePseries(def, mem); + return virDomainNVDimmAlignSizePseries(mem); } else { mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); -- 2.26.2
The alignment for the pSeries NVDIMM does not depend on runtime constraints. This means that it can be done in device parse time, instead of runtime, allowing the domain XML to reflect what the auto-alignment would do when the domain starts. This brings consistency between the NVDIMM size reported by the domain XML and what the guest sees, without impacting existing guests that are using an unaligned size - they'll work as usual, but the domain XML will be updated with the actual size of the NVDIMM. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- Note: the 'hypervisor agnostic' comment mentioned below is true for all ppc64 DIMMs, not just NVDIMMs. In theory it is possible to move a lot of pSeries memory handling code from QEMU to this new function. Given that the only practical gain is extra nerd points (ppc64 is supported only by QEMU), for now let's settle just for NVDIMMs. src/conf/domain_conf.c | 23 ++++++++++++++++++- .../memory-hotplug-nvdimm-ppc64.xml | 2 +- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ea6a097161..22c6ba3b0d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5396,6 +5396,24 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock) } +static int +virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, + const virDomainDef *def) +{ + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + + return 0; +} + + static int virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5437,6 +5455,10 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, ret = virDomainVsockDefPostParse(dev->data.vsock); break; + case VIR_DOMAIN_DEVICE_MEMORY: + ret = virDomainMemoryDefPostParse(dev->data.memory, def); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -5451,7 +5473,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_AUDIO: ret = 0; diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml index ae5a17d3c8..ecb1b83b4a 100644 --- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml @@ -38,7 +38,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>550000</size> + <size unit='KiB'>524416</size> <node>0</node> <label> <size unit='KiB'>128</size> -- 2.26.2
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- NEWS.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/NEWS.rst b/NEWS.rst index 685c5e2225..d75c9a7c9e 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -40,7 +40,8 @@ v6.8.0 (unreleased) The auto-alignment logic was removed in v6.7.0 in favor of requiring the size provided by the user to be already aligned; however, this had the unintended consequence of breaking some existing guests. v6.8.0 restores - the previous behavior. + the previous behavior with an improvement: it also reflects the auto-aligned + value in the domain XML. * **Bug fixes** -- 2.26.2
On Tue, 2020-09-22 at 09:29 -0300, Daniel Henrique Barboza wrote:
Hi,
This is a follow up of [1] after Andrea pushed the revert patch as standalone.
Changes from v1: - patch 2 (former 3): moved the auto-align code from virDomainMemoryDefParseXML() to virDomainMemoryDefPostParse() - patch 3 (former 4): updated NEWS.rst based on Andrea's changes already upstream
[1] https://www.redhat.com/archives/libvir-list/2020-September/msg00864.html
Daniel Henrique Barboza (3): conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse() NEWS.rst: update NVDIMM changes entry
Looks good! Reviewed-by: Andrea Bolognani <abologna@redhat.com> Can you please confirm your GitLab account is 'danielhb'? I would like to add you to the libvirt organization so that you can start pushing your own patches instead of relying on someone else doing that for you. -- Andrea Bolognani / Red Hat / Virtualization
On 9/22/20 11:56 AM, Andrea Bolognani wrote:
On Tue, 2020-09-22 at 09:29 -0300, Daniel Henrique Barboza wrote:
Hi,
This is a follow up of [1] after Andrea pushed the revert patch as standalone.
Changes from v1: - patch 2 (former 3): moved the auto-align code from virDomainMemoryDefParseXML() to virDomainMemoryDefPostParse() - patch 3 (former 4): updated NEWS.rst based on Andrea's changes already upstream
[1] https://www.redhat.com/archives/libvir-list/2020-September/msg00864.html
Daniel Henrique Barboza (3): conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse() NEWS.rst: update NVDIMM changes entry
Looks good!
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Can you please confirm your GitLab account is 'danielhb'? I would like to add you to the libvirt organization so that you can start pushing your own patches instead of relying on someone else doing that for you.
Yes, 'danielhb' is my Gitlab username as well: https://gitlab.com/danielhb DHB
On Tue, 2020-09-22 at 14:52 -0300, Daniel Henrique Barboza wrote:
On 9/22/20 11:56 AM, Andrea Bolognani wrote:
On Tue, 2020-09-22 at 09:29 -0300, Daniel Henrique Barboza wrote:
Daniel Henrique Barboza (3): conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse() NEWS.rst: update NVDIMM changes entry
Looks good!
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Can you please confirm your GitLab account is 'danielhb'? I would like to add you to the libvirt organization so that you can start pushing your own patches instead of relying on someone else doing that for you.
Yes, 'danielhb' is my Gitlab username as well:
Great! I've added you to the group, you should be able to push patches to master now. Please make sure to pick up the R-bs before doing so. And, you know, always keep uncle Ben's wisdom in mind ;) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Daniel Henrique Barboza