Using the 'uuid' element for ppc64 NVDIMM memory added in the
previous patch, use it in qemuBuildMemoryDeviceStr() to pass
it over to QEMU.
Another ppc64 restriction is the necessity of a mem->labelsize,
given than ppc64 only support label-area backed NVDIMMs.
Finally, we don't want ppc64 NVDIMMs to align up due to the
high risk of going beyond the end of file with a 256MiB
increment that the user didn't predict. Align it down
instead.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
src/conf/domain_conf.c | 7 +++
src/qemu/qemu_command.c | 7 +++
src/qemu/qemu_domain.c | 47 +++++++++++++++++--
.../memory-hotplug-nvdimm-ppc64.args | 32 +++++++++++++
.../memory-hotplug-nvdimm-ppc64.xml | 5 +-
tests/qemuxml2argvtest.c | 4 ++
.../memory-hotplug-nvdimm-ppc64.xml | 5 +-
7 files changed, 102 insertions(+), 5 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3ae6c181c2..7f8018fed2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16582,6 +16582,13 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0)
goto error;
+ if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ ARCH_IS_PPC64(dom->os.arch) && def->labelsize == 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("label size is required for NVDIMM device"));
+ goto error;
+ }
+
if (virDomainDeviceInfoParseXML(xmlopt, memdevNode,
&def->info, flags) < 0)
goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9e0334a3e7..76f1247329 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3677,6 +3677,13 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
if (mem->labelsize)
virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
+ if (virUUIDIsValid(mem->uuid)) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ virUUIDFormat(mem->uuid, uuidstr);
+ virBufferAsprintf(&buf, "uuid=%s,", uuidstr);
+ }
+
if (mem->readonly) {
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_NVDIMM_UNARMED)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d3f796d85..2f420a43cd 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12418,6 +12418,35 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def,
}
+static void
+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. */
+ guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
+ guestArea = MAX(guestArea, ppc64AlignSize);
+
+ mem->size = guestArea + mem->labelsize;
+}
+
+
int
qemuDomainAlignMemorySizes(virDomainDefPtr def)
{
@@ -12464,8 +12493,14 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
/* Align memory module sizes */
for (i = 0; i < def->nmems; i++) {
- align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
- def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
+ if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ ARCH_IS_PPC64(def->os.arch)) {
+ qemuDomainNVDimmAlignSizePseries(def, def->mems[i]);
+ } else {
+ align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
+ def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
+ }
+
hotplugmem += def->mems[i]->size;
if (def->mems[i]->size > maxmemkb) {
@@ -12494,7 +12529,13 @@ void
qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
{
- mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def));
+ if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+ ARCH_IS_PPC64(def->os.arch)) {
+ qemuDomainNVDimmAlignSizePseries(def, mem);
+ } else {
+ mem->size = VIR_ROUND_UP(mem->size,
+ qemuDomainGetMemorySizeAlignment(def));
+ }
}
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
new file mode 100644
index 0000000000..92e6c538fb
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
@@ -0,0 +1,32 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-name QEMUGuest1 \
+-S \
+-machine pseries,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=1048576k,slots=16,maxmem=1099511627776k \
+-realtime mlock=off \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=1024 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+size=537001984 \
+-device nvdimm,node=0,label-size=131072,\
+uuid=49545eb3-75e1-2d0a-acdd-f0294406c99e,memdev=memnvdimm0,id=nvdimm0,slot=0 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml
index 59352d3c52..ae5a17d3c8 100644
--- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml
@@ -38,8 +38,11 @@
<path>/tmp/nvdimm</path>
</source>
<target>
- <size unit='KiB'>523264</size>
+ <size unit='KiB'>550000</size>
<node>0</node>
+ <label>
+ <size unit='KiB'>128</size>
+ </label>
</target>
<address type='dimm' slot='0'/>
</memory>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 35d413d40b..077f7e7650 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2790,6 +2790,10 @@ mymain(void)
DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-align");
DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-pmem");
DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly");
+ DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
+ QEMU_CAPS_OBJECT_MEMORY_RAM,
+ QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("machine-aeskeywrap-on-caps",
QEMU_CAPS_AES_KEY_WRAP,
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
index 59352d3c52..ae5a17d3c8 100644
--- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
@@ -38,8 +38,11 @@
<path>/tmp/nvdimm</path>
</source>
<target>
- <size unit='KiB'>523264</size>
+ <size unit='KiB'>550000</size>
<node>0</node>
+ <label>
+ <size unit='KiB'>128</size>
+ </label>
</target>
<address type='dimm' slot='0'/>
</memory>
--
2.24.1
On Thu, Mar 12, 2020 at 3:00 AM Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> Using the 'uuid' element for ppc64 NVDIMM memory added in the
> previous patch, use it in qemuBuildMemoryDeviceStr() to pass
> it over to QEMU.
>
> Another ppc64 restriction is the necessity of a mem->labelsize,
> given than ppc64 only support label-area backed NVDIMMs.
>
> Finally, we don't want ppc64 NVDIMMs to align up due to the
> high risk of going beyond the end of file with a 256MiB
> increment that the user didn't predict. Align it down
> instead.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
...
> +static void
> +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. */
> + guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
> + guestArea = MAX(guestArea, ppc64AlignSize);
The math is correct, but we need additional checks when a backing file of
size less than 256MB is attempted to be hot-plugged. The qemu errors
out like below if
the backing file is 240MB and MAX of (240, 256) is chosen.
-object memory-backend-file,id=memnvdimm1,prealloc=yes,mem-path
=/tmp/nvdimm,share=yes,size=268566528: backing store size 0xf000000 does
not match 'size' option 0x10020000
> +
> + mem->size = guestArea + mem->labelsize;
> +}
> +
> +....
> + </label>
> </target>
> <address type='dimm' slot='0'/>
> </memory>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 35d413d40b..077f7e7650 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2790,6 +2790,10 @@ mymain(void)
> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-align");
> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-pmem");
> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly");
> + DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_NVDIMM,
I see the qemucapabilitiesdata/caps-5*.ppc64* not updated with the
nvdimm support. Can you update it as well?
Then this can be changed to just DO_TEST_CAPS_LATEST_PPC64, without
this elaborate list of caps.
> + QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
> + QEMU_CAPS_OBJECT_MEMORY_RAM,
...
Thanks,
Shiva
On 3/20/20 4:53 AM, Shivaprasad bhat wrote:
> On Thu, Mar 12, 2020 at 3:00 AM Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>> Using the 'uuid' element for ppc64 NVDIMM memory added in the
>> previous patch, use it in qemuBuildMemoryDeviceStr() to pass
>> it over to QEMU.
>>
>> Another ppc64 restriction is the necessity of a mem->labelsize,
>> given than ppc64 only support label-area backed NVDIMMs.
>>
>> Finally, we don't want ppc64 NVDIMMs to align up due to the
>> high risk of going beyond the end of file with a 256MiB
>> increment that the user didn't predict. Align it down
>> instead.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
> ...
>> +static void
>> +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. */
>> + guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
>> + guestArea = MAX(guestArea, ppc64AlignSize);
>
> The math is correct, but we need additional checks when a backing file of
> size less than 256MB is attempted to be hot-plugged. The qemu errors
> out like below if
> the backing file is 240MB and MAX of (240, 256) is chosen.
>
> -object memory-backend-file,id=memnvdimm1,prealloc=yes,mem-path
> =/tmp/nvdimm,share=yes,size=268566528: backing store size 0xf000000 does
> not match 'size' option 0x10020000
Got it. I'll error out in this case.
>
>> +
>> + mem->size = guestArea + mem->labelsize;
>> +}
>> +
>> +....
>> + </label>
>> </target>
>> <address type='dimm' slot='0'/>
>> </memory>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 35d413d40b..077f7e7650 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2790,6 +2790,10 @@ mymain(void)
>> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-align");
>> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-pmem");
>> DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly");
>> + DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_NVDIMM,
>
> I see the qemucapabilitiesdata/caps-5*.ppc64* not updated with the
> nvdimm support. Can you update it as well?
> Then this can be changed to just DO_TEST_CAPS_LATEST_PPC64, without
> this elaborate list of caps.
>
>> + QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
>> + QEMU_CAPS_OBJECT_MEMORY_RAM,
> ...
Roger that!
DHB
>
> Thanks,
> Shiva
>
On 11. 3. 2020 22:29, Daniel Henrique Barboza wrote:
> Using the 'uuid' element for ppc64 NVDIMM memory added in the
> previous patch, use it in qemuBuildMemoryDeviceStr() to pass
> it over to QEMU.
>
> Another ppc64 restriction is the necessity of a mem->labelsize,
> given than ppc64 only support label-area backed NVDIMMs.
>
> Finally, we don't want ppc64 NVDIMMs to align up due to the
> high risk of going beyond the end of file with a 256MiB
> increment that the user didn't predict. Align it down
> instead.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> src/conf/domain_conf.c | 7 +++
> src/qemu/qemu_command.c | 7 +++
> src/qemu/qemu_domain.c | 47 +++++++++++++++++--
> .../memory-hotplug-nvdimm-ppc64.args | 32 +++++++++++++
> .../memory-hotplug-nvdimm-ppc64.xml | 5 +-
> tests/qemuxml2argvtest.c | 4 ++
> .../memory-hotplug-nvdimm-ppc64.xml | 5 +-
> 7 files changed, 102 insertions(+), 5 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3ae6c181c2..7f8018fed2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16582,6 +16582,13 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0)
> goto error;
>
> + if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
> + ARCH_IS_PPC64(dom->os.arch) && def->labelsize == 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("label size is required for NVDIMM device"));
> + goto error;
> + }
> +
I think this should go into virDomainMemoryDefValidate() instead,
because this is not strictly a parse error (e.g. like invalid UUID or
missing element).
The rest looks good. Can you please post a v3 of just this patch? I will
handle the review and merge. Sorry for delayed review.
Michal
On 3/23/20 3:46 PM, Michal Prívozník wrote:
> On 11. 3. 2020 22:29, Daniel Henrique Barboza wrote:
>> Using the 'uuid' element for ppc64 NVDIMM memory added in the
>> previous patch, use it in qemuBuildMemoryDeviceStr() to pass
>> it over to QEMU.
>>
>> Another ppc64 restriction is the necessity of a mem->labelsize,
>> given than ppc64 only support label-area backed NVDIMMs.
>>
>> Finally, we don't want ppc64 NVDIMMs to align up due to the
>> high risk of going beyond the end of file with a 256MiB
>> increment that the user didn't predict. Align it down
>> instead.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> src/conf/domain_conf.c | 7 +++
>> src/qemu/qemu_command.c | 7 +++
>> src/qemu/qemu_domain.c | 47 +++++++++++++++++--
>> .../memory-hotplug-nvdimm-ppc64.args | 32 +++++++++++++
>> .../memory-hotplug-nvdimm-ppc64.xml | 5 +-
>> tests/qemuxml2argvtest.c | 4 ++
>> .../memory-hotplug-nvdimm-ppc64.xml | 5 +-
>> 7 files changed, 102 insertions(+), 5 deletions(-)
>> create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3ae6c181c2..7f8018fed2 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -16582,6 +16582,13 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
>> if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0)
>> goto error;
>>
>> + if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
>> + ARCH_IS_PPC64(dom->os.arch) && def->labelsize == 0) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("label size is required for NVDIMM device"));
>> + goto error;
>> + }
>> +
>
> I think this should go into virDomainMemoryDefValidate() instead,
> because this is not strictly a parse error (e.g. like invalid UUID or
> missing element).
>
> The rest looks good. Can you please post a v3 of just this patch? I will
> handle the review and merge. Sorry for delayed review.
No problem,. I'll send a v4 with this change (v3 is already in the ML with other
changes).
Thanks,
DHB
>
> Michal
>
© 2016 - 2026 Red Hat, Inc.