[libvirt] [RFC 0/3] update NVDIMM support

[Luyao Zhong]

Hi libvirt experts,

This is the RFC for updating NVDIMM support in libvirt.

QEMU has supported four more properties which libvirt has not introduced
yet, including 'align', 'pmem', 'nvdimm-persistences' and 'unarmed'.

The 'align' property allows users to specify the proper alignment. The
previous alignment can only be 4K because QEMU use pagesize as alignment.
But some backends may require alignments different from the pagesize.

The 'pmem' property allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory. Then QEMU will know if
it needs to guarrantee the write persistence to the vNVDIMM backend.

The 'nvdimm-persistence' property allows users to set platform-supported
features about NVDIMM data persistence of a guest.

The 'unarmed' property allows users to mark vNVDIMM read-only. Only the
device DAX on the real NVDIMM can guarantee the guest write persistence,
so it's suggested to set 'unarmed' option to 'on' and then vNVDIMM device
will be marked as read-only.

Libvirt introduces 'alignsize', 'pmem', 'persistence' and 'unarmed' config
elements into xml corresponding to 'align', 'pmem', 'nvdimm-persistence'
and 'unarmed' properties in QEMU, and update xml parsing, formating and
qemu command-line generating process for NVDIMM.

Thanks,
Zhong, Luyao

Luyao Zhong (3):
xml: introduce more config elements for NVDIMM memory
xml: update xml parsing and formating about NVDIMM memory
qemu: update qemu command-line generating for NVDIMM memory

docs/formatdomain.html.in | 98 +++++++++++++++---
docs/schemas/domaincommon.rng | 31 +++++-
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
src/qemu/qemu_command.c | 25 +++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++
.../memory-hotplug-nvdimm-align.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 +++++++++++
tests/qemuxml2argvtest.c | 12 +++
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
20 files changed, 636 insertions(+), 25 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml

[Luyao Zhong]

Four new parameters were introduced into libvirt xml, including
'align', 'pmem', 'persistence' and 'unarmed', which are related to
NVDIMM memory device. So we need parse and format the xml to save
these configurations.Besides, more testcases related to these
parameters were added to verify the xml2xml process.

Signed-off-by: Zhong,Luyao <***@intel.com>
---
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
8 files changed, 132 insertions(+), 7 deletions(-)
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56..1326116 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -932,6 +932,12 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
"dimm",
"nvdimm")

+VIR_ENUM_IMPL(virDomainMemoryPersistence,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+ "",
+ "mem-ctrl",
+ "cpu")
+
VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
"ivshmem",
"ivshmem-plain",
@@ -15656,7 +15662,9 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
virDomainMemoryDefPtr def)
{
int ret = -1;
+ int val;
char *nodemask = NULL;
+ char *ndPmem = NULL;
xmlNodePtr save = ctxt->node;
ctxt->node = node;

@@ -15685,6 +15693,21 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
_("path is required for model 'nvdimm'"));
goto cleanup;
}
+
+ if (virDomainParseMemory("./alignsize", "./alignsize/@unit", ctxt,
+ &def->alignsize, false, false) < 0)
+ goto cleanup;
+
+ if ((ndPmem = virXPathString("string(./pmem)", ctxt))) {
+ if ((val = virTristateSwitchTypeFromString(ndPmem)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'pmem': %s"),
+ ndPmem);
+ goto cleanup;
+ }
+ def->nvdimmPmem = val;
+ }
+ VIR_FREE(ndPmem);
break;

case VIR_DOMAIN_MEMORY_MODEL_NONE:
@@ -15696,6 +15719,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,

cleanup:
VIR_FREE(nodemask);
+ VIR_FREE(ndPmem);
ctxt->node = save;
return ret;
}
@@ -15710,6 +15734,9 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
xmlNodePtr save = ctxt->node;
ctxt->node = node;
int rv;
+ int val;
+ char *ndPrst = NULL;
+ char *ndUnarmed = NULL;

/* initialize to value which marks that the user didn't specify it */
def->targetNode = -1;
@@ -15741,11 +15768,35 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
_("label size must be smaller than NVDIMM size"));
goto cleanup;
}
+
+ if ((ndPrst = virXPathString("string(./persistence)", ctxt))) {
+ if ((val = virDomainMemoryPersistenceTypeFromString(ndPrst)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'persistence': %s"),
+ ndPrst);
+ goto cleanup;
+ }
+ def->nvdimmPersistence = val;
+ }
+ VIR_FREE(ndPrst);
+
+ if ((ndUnarmed = virXPathString("string(./unarmed)", ctxt))) {
+ if ((val = virTristateSwitchTypeFromString(ndUnarmed)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'unarmed': %s"),
+ ndUnarmed);
+ goto cleanup;
+ }
+ def->nvdimmUnarmed = val;
+ }
+ VIR_FREE(ndUnarmed);
}

ret = 0;

cleanup:
+ VIR_FREE(ndPrst);
+ VIR_FREE(ndUnarmed);
ctxt->node = save;
return ret;
}
@@ -22447,13 +22498,49 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
return false;
}

- if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- src->labelsize != dst->labelsize) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Target NVDIMM label size '%llu' doesn't match "
- "source NVDIMM label size '%llu'"),
- src->labelsize, dst->labelsize);
- return false;
+ if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (src->labelsize != dst->labelsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM label size '%llu' doesn't match "
+ "source NVDIMM label size '%llu'"),
+ src->labelsize, dst->labelsize);
+ return false;
+ }
+
+ if (src->alignsize != dst->alignsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM alignment '%llu' doesn't match "
+ "source NVDIMM alignment '%llu'"),
+ src->alignsize, dst->alignsize);
+ return false;
+ }
+
+ if (src->nvdimmPmem != dst->nvdimmPmem) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM pmem flag '%s' doesn't match "
+ "source NVDIMM pmem flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmPmem),
+ virTristateSwitchTypeToString(dst->nvdimmPmem));
+ return false;
+ }
+
+ if (src->nvdimmPersistence != dst->nvdimmPersistence) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM persistence value '%s' doesn't match "
+ "source NVDIMM persistence value '%s'"),
+ virDomainMemoryPersistenceTypeToString(src->nvdimmPersistence),
+ virDomainMemoryPersistenceTypeToString(dst->nvdimmPersistence));
+ return false;
+ }
+
+ if (src->nvdimmUnarmed != dst->nvdimmUnarmed) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM unarmed flag '%s' doesn't match "
+ "source NVDIMM unarmed flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmUnarmed),
+ virTristateSwitchTypeToString(dst->nvdimmUnarmed));
+ return false;
+ }
}

return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
@@ -25939,6 +26026,14 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,

case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath);
+
+ if (def->alignsize)
+ virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n",
+ def->alignsize);
+
+ if (def->nvdimmPmem)
+ virBufferEscapeString(buf, "<pmem>%s</pmem>\n",
+ virTristateSwitchTypeToString(def->nvdimmPmem));
break;

case VIR_DOMAIN_MEMORY_MODEL_NONE:
@@ -25974,6 +26069,12 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</label>\n");
}
+ if (def->nvdimmPersistence)
+ virBufferEscapeString(buf, "<persistence>%s</persistence>\n",
+ virDomainMemoryPersistenceTypeToString(def->nvdimmPersistence));
+ if (def->nvdimmUnarmed)
+ virBufferEscapeString(buf, "<unarmed>%s</unarmed>\n",
+ virTristateSwitchTypeToString(def->nvdimmUnarmed));

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</target>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e30a4b2..057aaea 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2133,6 +2133,14 @@ typedef enum {
VIR_DOMAIN_MEMORY_MODEL_LAST
} virDomainMemoryModel;

+typedef enum {
+ VIR_DOMAIN_MEMORY_PERSISTENCE_NONE,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_MEMCTRL,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_CPU,
+
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+} virDomainMemoryPersistence;
+
struct _virDomainMemoryDef {
virDomainMemoryAccess access;
virTristateBool discard;
@@ -2141,12 +2149,17 @@ struct _virDomainMemoryDef {
virBitmapPtr sourceNodes;
unsigned long long pagesize; /* kibibytes */
char *nvdimmPath;
+ unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPmem; /* enum virTristateSwitch; valid only for NVDIMM */

/* target */
int model; /* virDomainMemoryModel */
int targetNode;
unsigned long long size; /* kibibytes */
unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPersistence; /* enum virDomainMemoryPersistence;
+ valid only for NVDIMM*/
+ int nvdimmUnarmed; /* enum virTristateSwitch; valid only for NVDIMM */

virDomainDeviceInfo info;
};
@@ -3448,6 +3461,7 @@ VIR_ENUM_DECL(virDomainTPMVersion)
VIR_ENUM_DECL(virDomainMemoryModel)
VIR_ENUM_DECL(virDomainMemoryBackingModel)
VIR_ENUM_DECL(virDomainMemorySource)
+VIR_ENUM_DECL(virDomainMemoryPersistence)
VIR_ENUM_DECL(virDomainMemoryAllocation)
VIR_ENUM_DECL(virDomainIOMMUModel)
VIR_ENUM_DECL(virDomainVsockModel)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9236391..e925f7b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -436,6 +436,8 @@ virDomainMemoryFindByDef;
virDomainMemoryFindInactiveByDef;
virDomainMemoryInsert;
virDomainMemoryModelTypeToString;
+virDomainMemoryPersistenceTypeFromString;
+virDomainMemoryPersistenceTypeToString;
virDomainMemoryRemove;
virDomainMemorySourceTypeFromString;
virDomainMemorySourceTypeToString;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
new file mode 120000
index 0000000..9fc6001
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
new file mode 120000
index 0000000..0c0de1b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
new file mode 120000
index 0000000..3e57c1e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 120000
index 0000000..1793347
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 89640f6..4a931f2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1089,6 +1089,10 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm", NONE);
DO_TEST("memory-hotplug-nvdimm-access", NONE);
DO_TEST("memory-hotplug-nvdimm-label", NONE);
+ DO_TEST("memory-hotplug-nvdimm-align", NONE);
+ DO_TEST("memory-hotplug-nvdimm-pmem", NONE);
+ DO_TEST("memory-hotplug-nvdimm-persistence", NONE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed", NONE);
DO_TEST("net-udp", NONE);

DO_TEST("video-virtio-gpu-device", NONE);

[John Ferlan]

NB: I had to remove "***@redhat.com" from the CC line since it failed
to send.

Post by Luyao Zhong
Four new parameters were introduced into libvirt xml, including
'align', 'pmem', 'persistence' and 'unarmed', which are related to
NVDIMM memory device. So we need parse and format the xml to save
these configurations.Besides, more testcases related to these
parameters were added to verify the xml2xml process.
---
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
8 files changed, 132 insertions(+), 7 deletions(-)
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml

This patch causes failures for "make check", specifically virschematest
and qemuxml2xmltest... Seems you may have forgotten the input XML for
this patch, but included it in the next one. This one needs the input data.

Similar to patch1 comments - you'll need to extract things out a bit,
essentially creating 3 patches from this one - although those would be
included with the 3 patches for each part of patch1.

Post by Luyao Zhong
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56..1326116 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -932,6 +932,12 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
"dimm",
"nvdimm")
+VIR_ENUM_IMPL(virDomainMemoryPersistence,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+ "",
+ "mem-ctrl",
+ "cpu")
+
VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
"ivshmem",
"ivshmem-plain",
@@ -15656,7 +15662,9 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
virDomainMemoryDefPtr def)
{
int ret = -1;
+ int val;
char *nodemask = NULL;
+ char *ndPmem = NULL;
xmlNodePtr save = ctxt->node;
ctxt->node = node;
@@ -15685,6 +15693,21 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
_("path is required for model 'nvdimm'"));
goto cleanup;
}
+
+ &def->alignsize, false, false) < 0)
+ goto cleanup;
+
+ if ((ndPmem = virXPathString("string(./pmem)", ctxt))) {

This is a much simpler check following nosharepages' model.

Post by Luyao Zhong
+ if ((val = virTristateSwitchTypeFromString(ndPmem)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'pmem': %s"),
+ ndPmem);
+ goto cleanup;
+ }
+ def->nvdimmPmem = val;
+ }
+ VIR_FREE(ndPmem);
break;
@@ -15696,6 +15719,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
VIR_FREE(nodemask);
+ VIR_FREE(ndPmem);
ctxt->node = save;
return ret;
}
@@ -15710,6 +15734,9 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
xmlNodePtr save = ctxt->node;
ctxt->node = node;
int rv;
+ int val;
+ char *ndPrst = NULL;
+ char *ndUnarmed = NULL;
/* initialize to value which marks that the user didn't specify it */
def->targetNode = -1;
@@ -15741,11 +15768,35 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
_("label size must be smaller than NVDIMM size"));
goto cleanup;
}
+
+ if ((ndPrst = virXPathString("string(./persistence)", ctxt))) {
+ if ((val = virDomainMemoryPersistenceTypeFromString(ndPrst)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'persistence': %s"),
+ ndPrst);
+ goto cleanup;
+ }
+ def->nvdimmPersistence = val;
+ }
+ VIR_FREE(ndPrst);

This one seems strange to place on a target since it gets added to a
machine command line. I would think it needs to be w/ machine, because
it's not like one nvdimm can have it and other not have it, right? That
is it's not specific to a single entry and since there can be multiple
nvdimm's once it's defined it's there for all.

The above was written before I read patch3 and went back to patch1 to
complain about placement. But the context still is true - it doesn't
belong as a device it seems since it ends up on the <machine> command line.

Post by Luyao Zhong
+
+ if ((ndUnarmed = virXPathString("string(./unarmed)", ctxt))) {
+ if ((val = virTristateSwitchTypeFromString(ndUnarmed)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'unarmed': %s"),
+ ndUnarmed);
+ goto cleanup;
+ }
+ def->nvdimmUnarmed = val;
+ }
+ VIR_FREE(ndUnarmed);

And again unarmed is much simpler like nosharepages.

Post by Luyao Zhong
}
ret = 0;
+ VIR_FREE(ndPrst);
+ VIR_FREE(ndUnarmed);
ctxt->node = save;
return ret;
}
@@ -22447,13 +22498,49 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
return false;
}
- if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- src->labelsize != dst->labelsize) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Target NVDIMM label size '%llu' doesn't match "
- "source NVDIMM label size '%llu'"),
- src->labelsize, dst->labelsize);
- return false;
+ if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (src->labelsize != dst->labelsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM label size '%llu' doesn't match "
+ "source NVDIMM label size '%llu'"),
+ src->labelsize, dst->labelsize);
+ return false;
+ }
+
+ if (src->alignsize != dst->alignsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM alignment '%llu' doesn't match "
+ "source NVDIMM alignment '%llu'"),
+ src->alignsize, dst->alignsize);
+ return false;
+ }
+
+ if (src->nvdimmPmem != dst->nvdimmPmem) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM pmem flag '%s' doesn't match "
+ "source NVDIMM pmem flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmPmem),
+ virTristateSwitchTypeToString(dst->nvdimmPmem));
+ return false;
+ }

Follow nosharepages and not tristate

Post by Luyao Zhong
+
+ if (src->nvdimmPersistence != dst->nvdimmPersistence) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM persistence value '%s' doesn't match "
+ "source NVDIMM persistence value '%s'"),
+ virDomainMemoryPersistenceTypeToString(src->nvdimmPersistence),
+ virDomainMemoryPersistenceTypeToString(dst->nvdimmPersistence));
+ return false;
+ }
+
+ if (src->nvdimmUnarmed != dst->nvdimmUnarmed) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM unarmed flag '%s' doesn't match "
+ "source NVDIMM unarmed flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmUnarmed),
+ virTristateSwitchTypeToString(dst->nvdimmUnarmed));
+ return false;
+ }

Similar follow nosharepages and not be tristate's

Post by Luyao Zhong
}
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
@@ -25939,6 +26026,14 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath);
+
+ if (def->alignsize)
+ virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n",
+ def->alignsize);
+
+ if (def->nvdimmPmem)
+ virBufferEscapeString(buf, "<pmem>%s</pmem>\n",
+ virTristateSwitchTypeToString(def->nvdimmPmem));

Much more simple following nosharepages.

Post by Luyao Zhong
break;
@@ -25974,6 +26069,12 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</label>\n");
}
+ if (def->nvdimmPersistence)
+ virBufferEscapeString(buf, "<persistence>%s</persistence>\n",
+ virDomainMemoryPersistenceTypeToString(def->nvdimmPersistence));
+ if (def->nvdimmUnarmed)
+ virBufferEscapeString(buf, "<unarmed>%s</unarmed>\n",
+ virTristateSwitchTypeToString(def->nvdimmUnarmed));

Again.

Post by Luyao Zhong
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</target>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e30a4b2..057aaea 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2133,6 +2133,14 @@ typedef enum {
VIR_DOMAIN_MEMORY_MODEL_LAST
} virDomainMemoryModel;
+typedef enum {
+ VIR_DOMAIN_MEMORY_PERSISTENCE_NONE,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_MEMCTRL,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_CPU,
+
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+} virDomainMemoryPersistence;
+
struct _virDomainMemoryDef {
virDomainMemoryAccess access;
virTristateBool discard;
@@ -2141,12 +2149,17 @@ struct _virDomainMemoryDef {
virBitmapPtr sourceNodes;
unsigned long long pagesize; /* kibibytes */
char *nvdimmPath;
+ unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPmem; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimPmem

Post by Luyao Zhong
/* target */
int model; /* virDomainMemoryModel */
int targetNode;
unsigned long long size; /* kibibytes */
unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPersistence; /* enum virDomainMemoryPersistence;
+ valid only for NVDIMM*/
+ int nvdimmUnarmed; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimmUnarmed

Post by Luyao Zhong
virDomainDeviceInfo info;
};
@@ -3448,6 +3461,7 @@ VIR_ENUM_DECL(virDomainTPMVersion)
VIR_ENUM_DECL(virDomainMemoryModel)
VIR_ENUM_DECL(virDomainMemoryBackingModel)
VIR_ENUM_DECL(virDomainMemorySource)
+VIR_ENUM_DECL(virDomainMemoryPersistence)
VIR_ENUM_DECL(virDomainMemoryAllocation)
VIR_ENUM_DECL(virDomainIOMMUModel)
VIR_ENUM_DECL(virDomainVsockModel)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9236391..e925f7b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -436,6 +436,8 @@ virDomainMemoryFindByDef;
virDomainMemoryFindInactiveByDef;
virDomainMemoryInsert;
virDomainMemoryModelTypeToString;
+virDomainMemoryPersistenceTypeFromString;
+virDomainMemoryPersistenceTypeToString;
virDomainMemoryRemove;
virDomainMemorySourceTypeFromString;
virDomainMemorySourceTypeToString;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
new file mode 120000
index 0000000..9fc6001
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
new file mode 120000
index 0000000..0c0de1b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
new file mode 120000
index 0000000..3e57c1e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 120000
index 0000000..1793347
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 89640f6..4a931f2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1089,6 +1089,10 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm", NONE);
DO_TEST("memory-hotplug-nvdimm-access", NONE);
DO_TEST("memory-hotplug-nvdimm-label", NONE);
+ DO_TEST("memory-hotplug-nvdimm-align", NONE);
+ DO_TEST("memory-hotplug-nvdimm-pmem", NONE);

The above two should be combined...

John

Post by Luyao Zhong
+ DO_TEST("memory-hotplug-nvdimm-persistence", NONE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed", NONE);
DO_TEST("net-udp", NONE);
DO_TEST("video-virtio-gpu-device", NONE);

[Zhong, Luyao]

Post by John Ferlan
to send.

Post by Luyao Zhong
Four new parameters were introduced into libvirt xml, including
'align', 'pmem', 'persistence' and 'unarmed', which are related to
NVDIMM memory device. So we need parse and format the xml to save
these configurations.Besides, more testcases related to these
parameters were added to verify the xml2xml process.
---
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
8 files changed, 132 insertions(+), 7 deletions(-)
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml

This patch causes failures for "make check", specifically virschematest
and qemuxml2xmltest... Seems you may have forgotten the input XML for
this patch, but included it in the next one. This one needs the input data.

Got it.

Post by John Ferlan
Similar to patch1 comments - you'll need to extract things out a bit,
essentially creating 3 patches from this one - although those would be
included with the 3 patches for each part of patch1.

Got it.

Post by John Ferlan

Post by Luyao Zhong
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56..1326116 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -932,6 +932,12 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
"dimm",
"nvdimm")
+VIR_ENUM_IMPL(virDomainMemoryPersistence,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+ "",
+ "mem-ctrl",
+ "cpu")
+
VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
"ivshmem",
"ivshmem-plain",
@@ -15656,7 +15662,9 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
virDomainMemoryDefPtr def)
{
int ret = -1;
+ int val;
char *nodemask = NULL;
+ char *ndPmem = NULL;
xmlNodePtr save = ctxt->node;
ctxt->node = node;
@@ -15685,6 +15693,21 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
_("path is required for model 'nvdimm'"));
goto cleanup;
}
+
+ &def->alignsize, false, false) < 0)
+ goto cleanup;
+
+ if ((ndPmem = virXPathString("string(./pmem)", ctxt))) {

This is a much simpler check following nosharepages' model.

Post by Luyao Zhong
+ if ((val = virTristateSwitchTypeFromString(ndPmem)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'pmem': %s"),
+ ndPmem);
+ goto cleanup;
+ }
+ def->nvdimmPmem = val;
+ }
+ VIR_FREE(ndPmem);
break;
@@ -15696,6 +15719,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
VIR_FREE(nodemask);
+ VIR_FREE(ndPmem);
ctxt->node = save;
return ret;
}
@@ -15710,6 +15734,9 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
xmlNodePtr save = ctxt->node;
ctxt->node = node;
int rv;
+ int val;
+ char *ndPrst = NULL;
+ char *ndUnarmed = NULL;
/* initialize to value which marks that the user didn't specify it */
def->targetNode = -1;
@@ -15741,11 +15768,35 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
_("label size must be smaller than NVDIMM size"));
goto cleanup;
}
+
+ if ((ndPrst = virXPathString("string(./persistence)", ctxt))) {
+ if ((val = virDomainMemoryPersistenceTypeFromString(ndPrst)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'persistence': %s"),
+ ndPrst);
+ goto cleanup;
+ }
+ def->nvdimmPersistence = val;
+ }
+ VIR_FREE(ndPrst);

This one seems strange to place on a target since it gets added to a
machine command line. I would think it needs to be w/ machine, because
it's not like one nvdimm can have it and other not have it, right? That
is it's not specific to a single entry and since there can be multiple
nvdimm's once it's defined it's there for all.

You are right, I misunderstand your comment in patch1. I will redesign
the 'persistence' part. Maybe it's better to kick it out from this patch
set and put it into another patch.

Post by John Ferlan
The above was written before I read patch3 and went back to patch1 to
complain about placement. But the context still is true - it doesn't
belong as a device it seems since it ends up on the <machine> command line.

Post by Luyao Zhong
+
+ if ((ndUnarmed = virXPathString("string(./unarmed)", ctxt))) {
+ if ((val = virTristateSwitchTypeFromString(ndUnarmed)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'unarmed': %s"),
+ ndUnarmed);
+ goto cleanup;
+ }
+ def->nvdimmUnarmed = val;
+ }
+ VIR_FREE(ndUnarmed);

And again unarmed is much simpler like nosharepages.

Got it, I will check.

Post by John Ferlan

Post by Luyao Zhong
}
ret = 0;
+ VIR_FREE(ndPrst);
+ VIR_FREE(ndUnarmed);
ctxt->node = save;
return ret;
}
@@ -22447,13 +22498,49 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
return false;
}
- if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- src->labelsize != dst->labelsize) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Target NVDIMM label size '%llu' doesn't match "
- "source NVDIMM label size '%llu'"),
- src->labelsize, dst->labelsize);
- return false;
+ if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (src->labelsize != dst->labelsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM label size '%llu' doesn't match "
+ "source NVDIMM label size '%llu'"),
+ src->labelsize, dst->labelsize);
+ return false;
+ }
+
+ if (src->alignsize != dst->alignsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM alignment '%llu' doesn't match "
+ "source NVDIMM alignment '%llu'"),
+ src->alignsize, dst->alignsize);
+ return false;
+ }
+
+ if (src->nvdimmPmem != dst->nvdimmPmem) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM pmem flag '%s' doesn't match "
+ "source NVDIMM pmem flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmPmem),
+ virTristateSwitchTypeToString(dst->nvdimmPmem));
+ return false;
+ }

Follow nosharepages and not tristate

Got it, I will check

Post by John Ferlan

Post by Luyao Zhong
+
+ if (src->nvdimmPersistence != dst->nvdimmPersistence) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM persistence value '%s' doesn't match "
+ "source NVDIMM persistence value '%s'"),
+ virDomainMemoryPersistenceTypeToString(src->nvdimmPersistence),
+ virDomainMemoryPersistenceTypeToString(dst->nvdimmPersistence));
+ return false;
+ }
+
+ if (src->nvdimmUnarmed != dst->nvdimmUnarmed) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM unarmed flag '%s' doesn't match "
+ "source NVDIMM unarmed flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmUnarmed),
+ virTristateSwitchTypeToString(dst->nvdimmUnarmed));
+ return false;
+ }

Similar follow nosharepages and not be tristate's

Got it.

Post by John Ferlan

Post by Luyao Zhong
}
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
@@ -25939,6 +26026,14 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath);
+
+ if (def->alignsize)
+ virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n",
+ def->alignsize);
+
+ if (def->nvdimmPmem)
+ virBufferEscapeString(buf, "<pmem>%s</pmem>\n",
+ virTristateSwitchTypeToString(def->nvdimmPmem));

Much more simple following nosharepages.

Got it.

Post by John Ferlan

Post by Luyao Zhong
break;
@@ -25974,6 +26069,12 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</label>\n");
}
+ if (def->nvdimmPersistence)
+ virBufferEscapeString(buf, "<persistence>%s</persistence>\n",
+ virDomainMemoryPersistenceTypeToString(def->nvdimmPersistence));
+ if (def->nvdimmUnarmed)
+ virBufferEscapeString(buf, "<unarmed>%s</unarmed>\n",
+ virTristateSwitchTypeToString(def->nvdimmUnarmed));

Again.

Got it.

Post by John Ferlan

Post by Luyao Zhong
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</target>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e30a4b2..057aaea 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2133,6 +2133,14 @@ typedef enum {
VIR_DOMAIN_MEMORY_MODEL_LAST
} virDomainMemoryModel;
+typedef enum {
+ VIR_DOMAIN_MEMORY_PERSISTENCE_NONE,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_MEMCTRL,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_CPU,
+
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+} virDomainMemoryPersistence;
+
struct _virDomainMemoryDef {
virDomainMemoryAccess access;
virTristateBool discard;
@@ -2141,12 +2149,17 @@ struct _virDomainMemoryDef {
virBitmapPtr sourceNodes;
unsigned long long pagesize; /* kibibytes */
char *nvdimmPath;
+ unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPmem; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimPmem

Got it.

Post by John Ferlan

Post by Luyao Zhong
/* target */
int model; /* virDomainMemoryModel */
int targetNode;
unsigned long long size; /* kibibytes */
unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPersistence; /* enum virDomainMemoryPersistence;
+ valid only for NVDIMM*/
+ int nvdimmUnarmed; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimmUnarmed

Got it.

Post by John Ferlan

Post by Luyao Zhong
virDomainDeviceInfo info;
};
@@ -3448,6 +3461,7 @@ VIR_ENUM_DECL(virDomainTPMVersion)
VIR_ENUM_DECL(virDomainMemoryModel)
VIR_ENUM_DECL(virDomainMemoryBackingModel)
VIR_ENUM_DECL(virDomainMemorySource)
+VIR_ENUM_DECL(virDomainMemoryPersistence)
VIR_ENUM_DECL(virDomainMemoryAllocation)
VIR_ENUM_DECL(virDomainIOMMUModel)
VIR_ENUM_DECL(virDomainVsockModel)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9236391..e925f7b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -436,6 +436,8 @@ virDomainMemoryFindByDef;
virDomainMemoryFindInactiveByDef;
virDomainMemoryInsert;
virDomainMemoryModelTypeToString;
+virDomainMemoryPersistenceTypeFromString;
+virDomainMemoryPersistenceTypeToString;
virDomainMemoryRemove;
virDomainMemorySourceTypeFromString;
virDomainMemorySourceTypeToString;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
new file mode 120000
index 0000000..9fc6001
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
new file mode 120000
index 0000000..0c0de1b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
new file mode 120000
index 0000000..3e57c1e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 120000
index 0000000..1793347
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 89640f6..4a931f2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1089,6 +1089,10 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm", NONE);
DO_TEST("memory-hotplug-nvdimm-access", NONE);
DO_TEST("memory-hotplug-nvdimm-label", NONE);
+ DO_TEST("memory-hotplug-nvdimm-align", NONE);
+ DO_TEST("memory-hotplug-nvdimm-pmem", NONE);

The above two should be combined...

Got it.

Post by John Ferlan
John

Thanks a lot for your review.
Luyao

Post by John Ferlan

Post by Luyao Zhong
+ DO_TEST("memory-hotplug-nvdimm-persistence", NONE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed", NONE);
DO_TEST("net-udp", NONE);
DO_TEST("video-virtio-gpu-device", NONE);

[Zhong, Luyao]

Post by John Ferlan
to send.

Post by Luyao Zhong
Four new parameters were introduced into libvirt xml, including
'align', 'pmem', 'persistence' and 'unarmed', which are related to
NVDIMM memory device. So we need parse and format the xml to save
these configurations.Besides, more testcases related to these
parameters were added to verify the xml2xml process.
---
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
8 files changed, 132 insertions(+), 7 deletions(-)
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml

This patch causes failures for "make check", specifically virschematest
and qemuxml2xmltest... Seems you may have forgotten the input XML for
this patch, but included it in the next one. This one needs the input data.

Got it.

Post by John Ferlan
Similar to patch1 comments - you'll need to extract things out a bit,
essentially creating 3 patches from this one - although those would be
included with the 3 patches for each part of patch1.

Got it.

Post by John Ferlan

Post by Luyao Zhong
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56..1326116 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -932,6 +932,12 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
"dimm",
"nvdimm")
+VIR_ENUM_IMPL(virDomainMemoryPersistence,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+ "",
+ "mem-ctrl",
+ "cpu")
+
VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
"ivshmem",
"ivshmem-plain",
@@ -15656,7 +15662,9 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
virDomainMemoryDefPtr def)
{
int ret = -1;
+ int val;
char *nodemask = NULL;
+ char *ndPmem = NULL;
xmlNodePtr save = ctxt->node;
ctxt->node = node;
@@ -15685,6 +15693,21 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
_("path is required for model 'nvdimm'"));
goto cleanup;
}
+
+ &def->alignsize, false, false) < 0)
+ goto cleanup;
+
+ if ((ndPmem = virXPathString("string(./pmem)", ctxt))) {

This is a much simpler check following nosharepages' model.

I will check nosharepages first.

Post by John Ferlan

Post by Luyao Zhong
+ if ((val = virTristateSwitchTypeFromString(ndPmem)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'pmem': %s"),
+ ndPmem);
+ goto cleanup;
+ }
+ def->nvdimmPmem = val;
+ }
+ VIR_FREE(ndPmem);
break;
@@ -15696,6 +15719,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
VIR_FREE(nodemask);
+ VIR_FREE(ndPmem);
ctxt->node = save;
return ret;
}
@@ -15710,6 +15734,9 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
xmlNodePtr save = ctxt->node;
ctxt->node = node;
int rv;
+ int val;
+ char *ndPrst = NULL;
+ char *ndUnarmed = NULL;
/* initialize to value which marks that the user didn't specify it */
def->targetNode = -1;
@@ -15741,11 +15768,35 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
_("label size must be smaller than NVDIMM size"));
goto cleanup;
}
+
+ if ((ndPrst = virXPathString("string(./persistence)", ctxt))) {
+ if ((val = virDomainMemoryPersistenceTypeFromString(ndPrst)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'persistence': %s"),
+ ndPrst);
+ goto cleanup;
+ }
+ def->nvdimmPersistence = val;
+ }
+ VIR_FREE(ndPrst);

This one seems strange to place on a target since it gets added to a
machine command line. I would think it needs to be w/ machine, because
it's not like one nvdimm can have it and other not have it, right? That
is it's not specific to a single entry and since there can be multiple
nvdimm's once it's defined it's there for all.
The above was written before I read patch3 and went back to patch1 to
complain about placement. But the context still is true - it doesn't
belong as a device it seems since it ends up on the <machine> command line.

yes, you are right, the 'persistence' is different from the other
options I want to introduced, I have not noticed that before.

Post by John Ferlan

Post by Luyao Zhong
+
+ if ((ndUnarmed = virXPathString("string(./unarmed)", ctxt))) {
+ if ((val = virTristateSwitchTypeFromString(ndUnarmed)) < 0) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("Invalid value of nvdimm 'unarmed': %s"),
+ ndUnarmed);
+ goto cleanup;
+ }
+ def->nvdimmUnarmed = val;
+ }
+ VIR_FREE(ndUnarmed);

And again unarmed is much simpler like nosharepages.

Got it, I will check.

Post by John Ferlan

Post by Luyao Zhong
}
ret = 0;
+ VIR_FREE(ndPrst);
+ VIR_FREE(ndUnarmed);
ctxt->node = save;
return ret;
}
@@ -22447,13 +22498,49 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
return false;
}
- if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
- src->labelsize != dst->labelsize) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Target NVDIMM label size '%llu' doesn't match "
- "source NVDIMM label size '%llu'"),
- src->labelsize, dst->labelsize);
- return false;
+ if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+ if (src->labelsize != dst->labelsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM label size '%llu' doesn't match "
+ "source NVDIMM label size '%llu'"),
+ src->labelsize, dst->labelsize);
+ return false;
+ }
+
+ if (src->alignsize != dst->alignsize) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM alignment '%llu' doesn't match "
+ "source NVDIMM alignment '%llu'"),
+ src->alignsize, dst->alignsize);
+ return false;
+ }
+
+ if (src->nvdimmPmem != dst->nvdimmPmem) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM pmem flag '%s' doesn't match "
+ "source NVDIMM pmem flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmPmem),
+ virTristateSwitchTypeToString(dst->nvdimmPmem));
+ return false;
+ }

Follow nosharepages and not tristate

Got it, I will check.

Post by John Ferlan

Post by Luyao Zhong
+
+ if (src->nvdimmPersistence != dst->nvdimmPersistence) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM persistence value '%s' doesn't match "
+ "source NVDIMM persistence value '%s'"),
+ virDomainMemoryPersistenceTypeToString(src->nvdimmPersistence),
+ virDomainMemoryPersistenceTypeToString(dst->nvdimmPersistence));
+ return false;
+ }
+
+ if (src->nvdimmUnarmed != dst->nvdimmUnarmed) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Target NVDIMM unarmed flag '%s' doesn't match "
+ "source NVDIMM unarmed flag '%s'"),
+ virTristateSwitchTypeToString(src->nvdimmUnarmed),
+ virTristateSwitchTypeToString(dst->nvdimmUnarmed));
+ return false;
+ }

Similar follow nosharepages and not be tristate's

Got it, I will check.

Post by John Ferlan

Post by Luyao Zhong
}
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
@@ -25939,6 +26026,14 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath);
+
+ if (def->alignsize)
+ virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n",
+ def->alignsize);
+
+ if (def->nvdimmPmem)
+ virBufferEscapeString(buf, "<pmem>%s</pmem>\n",
+ virTristateSwitchTypeToString(def->nvdimmPmem));

Much more simple following nosharepages.

Got it, I will check.

Post by John Ferlan

Post by Luyao Zhong
break;
@@ -25974,6 +26069,12 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</label>\n");
}
+ if (def->nvdimmPersistence)
+ virBufferEscapeString(buf, "<persistence>%s</persistence>\n",
+ virDomainMemoryPersistenceTypeToString(def->nvdimmPersistence));
+ if (def->nvdimmUnarmed)
+ virBufferEscapeString(buf, "<unarmed>%s</unarmed>\n",
+ virTristateSwitchTypeToString(def->nvdimmUnarmed));

Again.

Got it.

Post by John Ferlan

Post by Luyao Zhong
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</target>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e30a4b2..057aaea 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2133,6 +2133,14 @@ typedef enum {
VIR_DOMAIN_MEMORY_MODEL_LAST
} virDomainMemoryModel;
+typedef enum {
+ VIR_DOMAIN_MEMORY_PERSISTENCE_NONE,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_MEMCTRL,
+ VIR_DOMAIN_MEMORY_PERSISTENCE_CPU,
+
+ VIR_DOMAIN_MEMORY_PERSISTENCE_LAST,
+} virDomainMemoryPersistence;
+
struct _virDomainMemoryDef {
virDomainMemoryAccess access;
virTristateBool discard;
@@ -2141,12 +2149,17 @@ struct _virDomainMemoryDef {
virBitmapPtr sourceNodes;
unsigned long long pagesize; /* kibibytes */
char *nvdimmPath;
+ unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPmem; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimPmem

Got it.

Post by John Ferlan

Post by Luyao Zhong
/* target */
int model; /* virDomainMemoryModel */
int targetNode;
unsigned long long size; /* kibibytes */
unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
+ int nvdimmPersistence; /* enum virDomainMemoryPersistence;
+ valid only for NVDIMM*/
+ int nvdimmUnarmed; /* enum virTristateSwitch; valid only for NVDIMM */

bool nvdimmUnarmed

Got it.

Post by John Ferlan

Post by Luyao Zhong
virDomainDeviceInfo info;
};
@@ -3448,6 +3461,7 @@ VIR_ENUM_DECL(virDomainTPMVersion)
VIR_ENUM_DECL(virDomainMemoryModel)
VIR_ENUM_DECL(virDomainMemoryBackingModel)
VIR_ENUM_DECL(virDomainMemorySource)
+VIR_ENUM_DECL(virDomainMemoryPersistence)
VIR_ENUM_DECL(virDomainMemoryAllocation)
VIR_ENUM_DECL(virDomainIOMMUModel)
VIR_ENUM_DECL(virDomainVsockModel)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9236391..e925f7b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -436,6 +436,8 @@ virDomainMemoryFindByDef;
virDomainMemoryFindInactiveByDef;
virDomainMemoryInsert;
virDomainMemoryModelTypeToString;
+virDomainMemoryPersistenceTypeFromString;
+virDomainMemoryPersistenceTypeToString;
virDomainMemoryRemove;
virDomainMemorySourceTypeFromString;
virDomainMemorySourceTypeToString;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
new file mode 120000
index 0000000..9fc6001
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
new file mode 120000
index 0000000..0c0de1b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
new file mode 120000
index 0000000..3e57c1e
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 120000
index 0000000..1793347
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 89640f6..4a931f2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1089,6 +1089,10 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm", NONE);
DO_TEST("memory-hotplug-nvdimm-access", NONE);
DO_TEST("memory-hotplug-nvdimm-label", NONE);
+ DO_TEST("memory-hotplug-nvdimm-align", NONE);
+ DO_TEST("memory-hotplug-nvdimm-pmem", NONE);

The above two should be combined...
Got it.
John

Thanks a lot for your review.
Luyao

Post by John Ferlan

Post by Luyao Zhong
+ DO_TEST("memory-hotplug-nvdimm-persistence", NONE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed", NONE);
DO_TEST("net-udp", NONE);
DO_TEST("video-virtio-gpu-device", NONE);

[Luyao Zhong]

According to the result parsing from xml, add corresponding properties
into QEMU command line, including 'align', 'pmem', 'persistence' and
'nvdimm-persistence'. And add testcases related to these properties.

Signed-off-by: Zhong,Luyao <***@intel.com>
---
src/qemu/qemu_command.c | 25 ++++++++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-align.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 12 +++++
10 files changed, 393 insertions(+)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 887947d..466a466 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3319,6 +3319,22 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
goto cleanup;

+ if (mem->alignsize) {
+ if (virJSONValueObjectAdd(props,
+ "U:align",
+ mem->alignsize * 1024,
+ NULL) < 0)
+ goto cleanup;
+ }
+
+ if (mem->nvdimmPmem) {
+ if (virJSONValueObjectAdd(props,
+ "s:pmem",
+ virTristateSwitchTypeToString(mem->nvdimmPmem),
+ NULL) < 0)
+ goto cleanup;
+ }
+
if (mem->sourceNodes) {
nodemask = mem->sourceNodes;
} else {
@@ -3480,6 +3496,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
if (mem->labelsize)
virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);

+ if (mem->nvdimmUnarmed)
+ virBufferAsprintf(&buf, "unarmed=%s,",
+ virTristateSwitchTypeToString(mem->nvdimmUnarmed));
+
virBufferAsprintf(&buf, "memdev=mem%s,id=%s",
mem->info.alias, mem->info.alias);

@@ -7256,6 +7276,11 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
goto cleanup;
}
virBufferAddLit(&buf, ",nvdimm=on");
+
+ if (def->mems[i]->nvdimmPersistence) {
+ virBufferAsprintf(&buf, ",nvdimm-persistence=%s",
+ virDomainMemoryPersistenceTypeToString(def->mems[i]->nvdimmPersistence));
+ }
break;
}
}
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
new file mode 100644
index 0000000..432f622
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,align=2097152 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
new file mode 100644
index 0000000..a8c5198
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <alignsize unit='KiB'>2048</alignsize>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
new file mode 100644
index 0000000..8ff03e3
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,nvdimm-persistence=mem-ctrl \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
new file mode 100644
index 0000000..28f2b7f
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <persistence>mem-ctrl</persistence>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
new file mode 100644
index 0000000..d34ac5b
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,pmem=on \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
new file mode 100644
index 0000000..b221ddb
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <pmem>on</pmem>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
new file mode 100644
index 0000000..64dcc5a
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,unarmed=on,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 100644
index 0000000..f60abbf
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <unarmed>on</unarmed>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a7cde3e..05ffca5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2717,6 +2717,18 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm-label",
QEMU_CAPS_DEVICE_NVDIMM,
QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-align",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-pmem",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-persistence",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);

DO_TEST("machine-aeskeywrap-on-caps",
QEMU_CAPS_AES_KEY_WRAP,

[John Ferlan]

Post by Luyao Zhong
According to the result parsing from xml, add corresponding properties
into QEMU command line, including 'align', 'pmem', 'persistence' and
'nvdimm-persistence'. And add testcases related to these properties.
---
src/qemu/qemu_command.c | 25 ++++++++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-align.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 12 +++++
10 files changed, 393 insertions(+)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml

As I noted in patch2, the .xml files from xml2xmlargvdata belong there...

You are missing qemu_capabilites.{c,h} and friends changes. Many other
examples, but they essentially determine which version the attribute was
added to QEMU and only allow/use it for those versions and beyond.

When adding capabilities, we tend to make them singular patches, but
you'd only need 1 as I assume all the new switches were added for the
same qemu release. You can note that in the commit message too.

Running make check make-check would have told you:

--- tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
2018-11-07 12:55:59.165965066 -0500
+++ - 2018-11-07 12:56:21.555957343 -0500
@@ -7,7 +7,8 @@
/usr/bin/qemu-system-i686 \
-name QEMUGuest1 \
-S \
--machine
pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,nvdimm-persistence=mem-ctrl
\
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,\
+nvdimm-persistence=mem-ctrl \
-m size=219136k,slots=16,maxmem=1099511627776k \
-smp 2,sockets=2,cores=1,threads=1 \
-numa node,nodeid=0,cpus=0-1,mem=214 \
Incorrect line wrapping in
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
Use test-wrap-argv.pl to wrap test data files
make: *** [cfg.mk:1139: test-wrap-argv] Error 1


You're also missing a patch to update docs/news.xml to describe your new
feature. So your simple 3 patches have probably blossomed into 12 or so.
Don't feel the need to post all at the same time. Work through the
<source> changes first, post those, get feedback, maybe get them pushed,
and then go from there.

Post by Luyao Zhong
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 887947d..466a466 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3319,6 +3319,22 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
goto cleanup;
+ if (mem->alignsize) {

mem->alignsize > 0

It's one of my crusades ;-)

Post by Luyao Zhong
+ if (virJSONValueObjectAdd(props,
+ "U:align",
+ mem->alignsize * 1024,
+ NULL) < 0)
+ goto cleanup;
+ }

Please multiple arguments on a line like "U:size" above - just don't go
beyond 80 char wide display...

Post by Luyao Zhong
+
+ if (mem->nvdimmPmem) {

This would be bool, so OK.

Post by Luyao Zhong
+ if (virJSONValueObjectAdd(props,
+ "s:pmem",
+ virTristateSwitchTypeToString(mem->nvdimmPmem),
+ NULL) < 0)

if (virJSONValueObjectAdd(props, "s:pmem", "on", NULL) < 0)

Post by Luyao Zhong
+ goto cleanup;
+ }
+
if (mem->sourceNodes) {
nodemask = mem->sourceNodes;
} else {
@@ -3480,6 +3496,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
if (mem->labelsize)
virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
+ if (mem->nvdimmUnarmed)
+ virBufferAsprintf(&buf, "unarmed=%s,",
+ virTristateSwitchTypeToString(mem->nvdimmUnarmed));

use virBufferAddLit here. There's other examples.

Post by Luyao Zhong
+
virBufferAsprintf(&buf, "memdev=mem%s,id=%s",
mem->info.alias, mem->info.alias);
@@ -7256,6 +7276,11 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
goto cleanup;
}
virBufferAddLit(&buf, ",nvdimm=on");
+
+ if (def->mems[i]->nvdimmPersistence) {

So if more than one entry had this persistence set, then this gets added
multiple times for each... What happens when they're different? And why
would we want to add multiple of the same?

Post by Luyao Zhong
+ virBufferAsprintf(&buf, ",nvdimm-persistence=%s",
+ virDomainMemoryPersistenceTypeToString(def->mems[i]->nvdimmPersistence));
+ }
break;
}
}
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
new file mode 100644
index 0000000..432f622
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,align=2097152 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
new file mode 100644
index 0000000..a8c5198
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <alignsize unit='KiB'>2048</alignsize>> + </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>

Please try to trim all these XML files to not include stuff you don't
need... such as idmap, cpu, numa, etc. I would add the <pmem/> here and
not have a separate test. You can validate both at the same time. In the
long run there's 3 new tests... Each series of patches would add them.

All the separation will be painful, but it is useful especially as it
relates to bisecting where a problem may have begun.

Post by Luyao Zhong
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
new file mode 100644
index 0000000..8ff03e3
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,nvdimm-persistence=mem-ctrl \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
new file mode 100644
index 0000000..28f2b7f
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <persistence>mem-ctrl</persistence>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
new file mode 100644
index 0000000..d34ac5b
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,pmem=on \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
new file mode 100644
index 0000000..b221ddb
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <pmem>on</pmem>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
new file mode 100644
index 0000000..64dcc5a
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,unarmed=on,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 100644
index 0000000..f60abbf
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <unarmed>on</unarmed>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a7cde3e..05ffca5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2717,6 +2717,18 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm-label",
QEMU_CAPS_DEVICE_NVDIMM,
QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-align",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-pmem",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-persistence",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);

These should be DO_TEST_CAPS_LATEST...

John

Post by Luyao Zhong
DO_TEST("machine-aeskeywrap-on-caps",
QEMU_CAPS_AES_KEY_WRAP,

[Zhong, Luyao]

Post by John Ferlan

Post by Luyao Zhong
According to the result parsing from xml, add corresponding properties
into QEMU command line, including 'align', 'pmem', 'persistence' and
'nvdimm-persistence'. And add testcases related to these properties.
---
src/qemu/qemu_command.c | 25 ++++++++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-align.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 ++++++++++++++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 12 +++++
10 files changed, 393 insertions(+)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml

As I noted in patch2, the .xml files from xml2xmlargvdata belong there...
You are missing qemu_capabilites.{c,h} and friends changes. Many other
examples, but they essentially determine which version the attribute was
added to QEMU and only allow/use it for those versions and beyond.

Got it , I will add this part in next version RFC.

Post by John Ferlan
When adding capabilities, we tend to make them singular patches, but
you'd only need 1 as I assume all the new switches were added for the
same qemu release. You can note that in the commit message too.

Thank you for giving these details.

Post by John Ferlan
--- tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
2018-11-07 12:55:59.165965066 -0500
+++ - 2018-11-07 12:56:21.555957343 -0500
@@ -7,7 +7,8 @@
/usr/bin/qemu-system-i686 \
-name QEMUGuest1 \
-S \
--machine
pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,nvdimm-persistence=mem-ctrl
\
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,\
+nvdimm-persistence=mem-ctrl \
-m size=219136k,slots=16,maxmem=1099511627776k \
-smp 2,sockets=2,cores=1,threads=1 \
-numa node,nodeid=0,cpus=0-1,mem=214 \
Incorrect line wrapping in
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
Use test-wrap-argv.pl to wrap test data files
make: *** [cfg.mk:1139: test-wrap-argv] Error 1

I will check this.

Post by John Ferlan
You're also missing a patch to update docs/news.xml to describe your new
feature. So your simple 3 patches have probably blossomed into 12 or so.
Don't feel the need to post all at the same time. Work through the
<source> changes first, post those, get feedback, maybe get them pushed,
and then go from there.

Got it. Thanks a lot.

Post by John Ferlan

Post by Luyao Zhong
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 887947d..466a466 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3319,6 +3319,22 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
goto cleanup;
+ if (mem->alignsize) {

mem->alignsize > 0

Got it.

Post by John Ferlan
It's one of my crusades ;-)

Post by Luyao Zhong
+ if (virJSONValueObjectAdd(props,
+ "U:align",
+ mem->alignsize * 1024,
+ NULL) < 0)
+ goto cleanup;
+ }

Please multiple arguments on a line like "U:size" above - just don't go
beyond 80 char wide display...

Got it.

Post by John Ferlan

Post by Luyao Zhong
+
+ if (mem->nvdimmPmem) {

This would be bool, so OK.

Post by Luyao Zhong
+ if (virJSONValueObjectAdd(props,
+ "s:pmem",
+ virTristateSwitchTypeToString(mem->nvdimmPmem),
+ NULL) < 0)

if (virJSONValueObjectAdd(props, "s:pmem", "on", NULL) < 0)

Post by Luyao Zhong
+ goto cleanup;
+ }
+
if (mem->sourceNodes) {
nodemask = mem->sourceNodes;
} else {
@@ -3480,6 +3496,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
if (mem->labelsize)
virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
+ if (mem->nvdimmUnarmed)
+ virBufferAsprintf(&buf, "unarmed=%s,",
+ virTristateSwitchTypeToString(mem->nvdimmUnarmed));

use virBufferAddLit here. There's other examples.

Got it, I will try.

Post by John Ferlan

Post by Luyao Zhong
+
virBufferAsprintf(&buf, "memdev=mem%s,id=%s",
mem->info.alias, mem->info.alias);
@@ -7256,6 +7276,11 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
goto cleanup;
}
virBufferAddLit(&buf, ",nvdimm=on");
+
+ if (def->mems[i]->nvdimmPersistence) {

So if more than one entry had this persistence set, then this gets added
multiple times for each... What happens when they're different? And why
would we want to add multiple of the same?

I have not noticed this situation. It should be the same, but I have not
test this case by qemu. I will test more and try to solve this problem.

Post by John Ferlan

Post by Luyao Zhong
+ virBufferAsprintf(&buf, ",nvdimm-persistence=%s",
+ virDomainMemoryPersistenceTypeToString(def->mems[i]->nvdimmPersistence));
+ }
break;
}
}
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
new file mode 100644
index 0000000..432f622
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,align=2097152 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
new file mode 100644
index 0000000..a8c5198
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <alignsize unit='KiB'>2048</alignsize>> + </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>

Please try to trim all these XML files to not include stuff you don't
need... such as idmap, cpu, numa, etc.

The 'nvdimm' need numa support.Actually this test xml is based on the
existing file tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml.

Post by John Ferlan
I would add the <pmem/> here and
not have a separate test. You can validate both at the same time. In the
long run there's 3 new tests... Each series of patches would add them.Got it, it's fine to put them together. I will take this into consider

when I update the pathes.

Post by John Ferlan
All the separation will be painful, but it is useful especially as it
relates to bisecting where a problem may have begun.

Got it. :)

Post by John Ferlan

Post by Luyao Zhong
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
new file mode 100644
index 0000000..8ff03e3
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on,nvdimm-persistence=mem-ctrl \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
new file mode 100644
index 0000000..28f2b7f
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <persistence>mem-ctrl</persistence>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
new file mode 100644
index 0000000..d34ac5b
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912,pmem=on \
+-device nvdimm,node=0,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
new file mode 100644
index 0000000..b221ddb
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ <pmem>on</pmem>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
new file mode 100644
index 0000000..64dcc5a
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+share=no,size=536870912 \
+-device nvdimm,node=0,unarmed=on,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 \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
new file mode 100644
index 0000000..f60abbf
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
@@ -0,0 +1,58 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+ <memory unit='KiB'>1267710</memory>
+ <currentMemory unit='KiB'>1267710</currentMemory>
+ <vcpu placement='static' cpuset='0-1'>2</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <idmap>
+ <uid start='0' target='1000' count='10'/>
+ <gid start='0' target='1000' count='10'/>
+ </idmap>
+ <cpu>
+ <topology sockets='2' cores='1' threads='1'/>
+ <numa>
+ <cell id='0' cpus='0-1' memory='219136' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ <memory model='nvdimm' access='private'>
+ <source>
+ <path>/tmp/nvdimm</path>
+ </source>
+ <target>
+ <size unit='KiB'>523264</size>
+ <node>0</node>
+ <unarmed>on</unarmed>
+ </target>
+ <address type='dimm' slot='0'/>
+ </memory>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a7cde3e..05ffca5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2717,6 +2717,18 @@ mymain(void)
DO_TEST("memory-hotplug-nvdimm-label",
QEMU_CAPS_DEVICE_NVDIMM,
QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-align",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-pmem",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-persistence",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
+ DO_TEST("memory-hotplug-nvdimm-unarmed",
+ QEMU_CAPS_DEVICE_NVDIMM,
+ QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);

These should be DO_TEST_CAPS_LATEST...

Got it.

Post by John Ferlan
John

Thanks a lot for your review. I need more rewords and recode work.
Luyao

Post by John Ferlan

Post by Luyao Zhong
DO_TEST("machine-aeskeywrap-on-caps",
QEMU_CAPS_AES_KEY_WRAP,

[Luyao Zhong]

In order to align with QEMU ,four more parameters about NVDIMM will
be introduced into Libvirt xml.

1.alignsize
The 'alignsize' option allows users to specify the proper alignment. When
mmap(2) the backend files, QEMU uses the host page size by default as
the alignment of mapping address. However, some backends may require
alignments different from the pagesize. For example, mmap a device DAX
on NVDIMM maybe 2M-aligned.

2.pmem
The 'pmem' option allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory that can be accessed in
SNIA NVM Programming Model. If the vNVDIMM backend is in host persistent
memory , and 'pmem' is 'on' and QEMU is built with libpmem, qemu will
guarantee the persistence of its own writes to the vNVDIMM backend.

3.persistence
The 'persistence' option allows users to set platform-supported features
about NVDIMM data persistence of a guest. It has two values, 'mem-ctrl'
means the platform supports flushing dirty data from the memory controller
to the NVDIMMs in the event of power loss, 'cpu' means The platform supports
flushing dirty data from the CPU cache to the NVDIMMs in the event of power
loss, which contains what 'mem-ctrl' means.

4.unarmed
The 'unarmed' option allows users to mark vNVDIMM read-only. Only one type
of vNVDIMM backends can guarantee the guest write persistence, which is the
device DAX on the real NVDIMM device (e.g., /dev/dax0.0). When using other
types of backends, it's suggested to set 'unarmed' option to 'on', so the
guest Linux NVDIMM driver will mark such vNVDIMM device as read-only.

For more details, see
https://github.com/qemu/qemu/blob/master/docs/nvdimm.txt

Signed-off-by: Zhong,Luyao <***@intel.com>
---
docs/formatdomain.html.in | 98 ++++++++++++++++++++++++++++++++++++-------
docs/schemas/domaincommon.rng | 31 ++++++++++++--
2 files changed, 111 insertions(+), 18 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8189959..9dec984 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8258,6 +8258,8 @@ qemu-kvm -net nic,model=? /dev/null
&lt;memory model='nvdimm'&gt;
&lt;source&gt;
&lt;path&gt;/tmp/nvdimm&lt;/path&gt;
+ &lt;alignsize unit='KiB'&gt;2048&lt;/alignsize&gt;
+ &lt;pmem&gt;on&lt;/pmem&gt;
&lt;/source&gt;
&lt;target&gt;
&lt;size unit='KiB'&gt;524288&lt;/size&gt;
@@ -8265,6 +8267,8 @@ qemu-kvm -net nic,model=? /dev/null
&lt;label&gt;
&lt;size unit='KiB'&gt;128&lt;/size&gt;
&lt;/label&gt;
+ &lt;persistence&gt;cpu&lt;/persistence&gt;
+ &lt;unarmed&gt;on&lt;/unarmed&gt;
&lt;/target&gt;
&lt;/memory&gt;
&lt;/devices&gt;
@@ -8339,10 +8343,38 @@ qemu-kvm -net nic,model=? /dev/null
</dl>

<p>
- For model <code>nvdimm</code> this element is mandatory and has a
- single child element <code>path</code> that represents a path
- in the host that backs the nvdimm module in the guest.
+ For model <code>nvdimm</code> this element is mandatory. The
+ mandatory child element <code>path</code> represents a path in
+ the host that backs the nvdimm module in the guest. If
+ <code>nvdimm</code> is provided, then the following optional
+ elements can be provided as well:
</p>
+
+ <dl>
+ <dt><code>alignsize</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify a proper alignment.
+ When mmap(2) the backend files, QEMU uses the host page
+ size by default as the alignment of mapping address. However,
+ some backends may require alignments different from the page.
+ For example, mmap a device DAX on NVDIMM maybe 2M-aligned.
+ </p>
+ </dd>
+
+ <dt><code>pmem</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify whether the backend storage
+ of memory-backend-file is a real persistent memory that can be
+ accessed in SNIA NVM Programming Model. If the backend is a real
+ persistence memory and <code>pmem</code> is set to 'on', QEMU will
+ guarantee the persistence of its own writes to the vNVDIMM backend,
+ but which can work well with libpmem support (QEMU configured with
+ --enable-libpmem).
+ </p>
+ </dd>
+ </dl>
</dd>

<dt><code>target</code></dt>
@@ -8361,19 +8393,55 @@ qemu-kvm -net nic,model=? /dev/null
NUMA nodes configured.
</p>
<p>
- For NVDIMM type devices one can optionally use
- <code>label</code> and its subelement <code>size</code>
- to configure the size of namespaces label storage
- within the NVDIMM module. The <code>size</code> element
- has usual meaning described
- <a href="#elementsMemoryAllocation">here</a>.
- For QEMU domains the following restrictions apply:
+ Besides, the following optional elements can be provided as well for
+ NVDIMM type devices:
</p>
- <ol>
- <li>the minimum label size is 128KiB,</li>
- <li>the remaining size (total-size - label-size) has to be aligned to
- 4KiB</li>
- </ol>
+
+ <dl>
+ <dt><code>label</code></dt>
+ <dd>
+ <p>
+ For NVDIMM type devices one can optionally use
+ <code>label</code> and its subelement <code>size</code>
+ to configure the size of namespaces label storage
+ within the NVDIMM module. The <code>size</code> element
+ has usual meaning described
+ <a href="#elementsMemoryAllocation">here</a>.
+ For QEMU domains the following restrictions apply:
+ </p>
+ <ol>
+ <li>the minimum label size is 128KiB,</li>
+ <li>the remaining size (total-size - label-size) will be aligned to
+ 4KiB as default.</li>
+ </ol>
+ </dd>
+
+ <dt><code>persistence</code></dt>
+ <dd>
+ <p>
+ The <code>persistence</code> element can be set to "mem-ctl" or "cpu",
+ which indicate platform-supported features about NVDIMM data persistence.
+ 'mem-ctrl' means the platform supports flushing dirty data from the memory
+ controller to the NVDIMMs in the event of power loss, 'cpu' means The platform
+ supports flushing dirty data from the CPU cache to the NVDIMMs in the event
+ of power loss, which contains what 'mem-ctrl' means.
+ ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
+ in NFIT, so the guest ACPI NFIT will be filled in according to the persistence
+ option.
+ </p>
+ </dd>
+
+ <dt><code>unarmed</code></dt>
+ <dd>
+ <p>
+ The <code>unarmed</code> element can be used to mark vNVDIMM read-only
+ through setting unarmed flag in guest NFIT.Currently the only vNVDIMM backend
+ can guarantee the guest write persistence is device DAX on Linux, so it's
+ suggested to set <code>unarmed</code> to 'on' when using other types of
+ backends.
+ </p>
+ </dd>
+ </dl>
</dd>
</dl>

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 099a949..cca7869 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5353,9 +5353,21 @@
</interleave>
</group>
<group>
- <element name="path">
- <ref name="absFilePath"/>
- </element>
+ <interleave>
+ <element name="path">
+ <ref name="absFilePath"/>
+ </element>
+ <optional>
+ <element name="alignsize">
+ <ref name="scaledInteger"/>
+ </element>
+ </optional>
+ <optional>
+ <element name="pmem">
+ <ref name="virOnOff"/>
+ </element>
+ </optional>
+ </interleave>
</group>
</choice>
</element>
@@ -5379,6 +5391,19 @@
</element>
</element>
</optional>
+ <optional>
+ <element name="persistence">
+ <choice>
+ <value>mem-ctrl</value>
+ <value>cpu</value>
+ </choice>
+ </element>
+ </optional>
+ <optional>
+ <element name="unarmed">
+ <ref name="virOnOff"/>
+ </element>
+ </optional>
</interleave>
</element>
</define>

[John Ferlan]

Post by Luyao Zhong
In order to align with QEMU ,four more parameters about NVDIMM will
be introduced into Libvirt xml.
1.alignsize
The 'alignsize' option allows users to specify the proper alignment. When
mmap(2) the backend files, QEMU uses the host page size by default as
the alignment of mapping address. However, some backends may require
alignments different from the pagesize. For example, mmap a device DAX
on NVDIMM maybe 2M-aligned.
2.pmem
The 'pmem' option allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory that can be accessed in
SNIA NVM Programming Model. If the vNVDIMM backend is in host persistent
memory , and 'pmem' is 'on' and QEMU is built with libpmem, qemu will
guarantee the persistence of its own writes to the vNVDIMM backend.
3.persistence
The 'persistence' option allows users to set platform-supported features
about NVDIMM data persistence of a guest. It has two values, 'mem-ctrl'
means the platform supports flushing dirty data from the memory controller
to the NVDIMMs in the event of power loss, 'cpu' means The platform supports
flushing dirty data from the CPU cache to the NVDIMMs in the event of power
loss, which contains what 'mem-ctrl' means.
4.unarmed
The 'unarmed' option allows users to mark vNVDIMM read-only. Only one type
of vNVDIMM backends can guarantee the guest write persistence, which is the
device DAX on the real NVDIMM device (e.g., /dev/dax0.0). When using other
types of backends, it's suggested to set 'unarmed' option to 'on', so the
guest Linux NVDIMM driver will mark such vNVDIMM device as read-only.

caveat: I didn't research too deeply into each of these options, but
I'll give you some feedback from the aspect of how you should formulate
your patches.

Rather than essentially replicate what was added in formatdomain provide
the examples here and a summary of what the proposed XML would "look
like", e.g.

Since 'alignsize' and 'pmem' seem to be related specifically to memory
mapped files:

<memory model='nvdimm'>
<source>
<path>/tmp/nvdimm</path>
<alignsize unit='KiB'>2048</alignsize>
<pmem>on</pmem>
</source>
...
</memory>

The <alignsize> would seem to similar in description to the existing
<pagesize> - although yes for a different model type.....

The <pmem> seems to have two states "on" or "off", thus having just
<pmem/> similar to perhaps how nosharepages is handled.

Since they're both memory mapped related changes, keep them together
from conf, xml, rng, etc. Then the "next" patch would be to do the qemu
and capability changes. With the last being any virsh changes to allow
modification on the command line (if possible).

The "persistence" seems to be less of a <memory> option and more of a
<machine> option, true/false? It would thus need to be separated from
the others. Still similar to the first one, separate patch for conf,
xml, rng, html... Then patch for qemu...

The "unarmed" is a <target> option since it's being added in patch2
after node and label are processed, so it should be separate from the
<source> and <machine> option patches. If it's just going to have 2
states, then it doesn't need <unarmed>on</unarmed> - instead it could
just be <unarmed/>.

Post by Luyao Zhong
For more details, see
https://github.com/qemu/qemu/blob/master/docs/nvdimm.txt

Not a commit message worthy... Place it under the ---...

Post by Luyao Zhong
---
docs/formatdomain.html.in | 98 ++++++++++++++++++++++++++++++++++++-------
docs/schemas/domaincommon.rng | 31 ++++++++++++--
2 files changed, 111 insertions(+), 18 deletions(-)

As noted above, but perhaps difficult to determine - the html.in, .rng,
domain_conf, xml2xml tests, etc. should be in one patch. Then another
patch for the qemu, capabilities, xml2argv tests, etc. in the next
patch. Then if you're going to add virsh options a separate patch for
that (although probably doesn't apply here).

After each patch, running make check syntax-check needs to pass.

Post by Luyao Zhong
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8189959..9dec984 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8258,6 +8258,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8265,6 +8267,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8339,10 +8343,38 @@ qemu-kvm -net nic,model=? /dev/null
</dl>
<p>
- For model <code>nvdimm</code> this element is mandatory and has a
- single child element <code>path</code> that represents a path
- in the host that backs the nvdimm module in the guest.
+ For model <code>nvdimm</code> this element is mandatory. The
+ mandatory child element <code>path</code> represents a path in
+ the host that backs the nvdimm module in the guest. If
+ <code>nvdimm</code> is provided, then the following optional

You'd need to add a since tags here for the "new" work...e.g.:

<span class='since'>since 4.10.0</span>

Whenever something new is added for a specific version, that's what's
necessary.

Post by Luyao Zhong
</p>
+
+ <dl>
+ <dt><code>alignsize</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify a proper alignment.
+ When mmap(2) the backend files, QEMU uses the host page
+ size by default as the alignment of mapping address. However,
+ some backends may require alignments different from the page.
+ For example, mmap a device DAX on NVDIMM maybe 2M-aligned.
+ </p>
+ </dd>
+
+ <dt><code>pmem</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify whether the backend storage
+ of memory-backend-file is a real persistent memory that can be
+ accessed in SNIA NVM Programming Model. If the backend is a real

^
there's two spaces between backend and is

Post by Luyao Zhong
+ persistence memory and <code>pmem</code> is set to 'on', QEMU will
+ guarantee the persistence of its own writes to the vNVDIMM backend,
+ but which can work well with libpmem support (QEMU configured with
+ --enable-libpmem).
+ </p>
+ </dd>
+ </dl>
</dd>

The probably should be reworded in syntax more suitable for libvirt's
usage without including QEMU details.

The pmem one in particular may be hard to describe... The fact that it
doesn't work without libpmem does raise a couple of warning flags. I
would just say that if <pmem/> is there, then it's on if it's not, then
it's off. Internally that then isn't a tristate, it just an on/off.

Post by Luyao Zhong
<dt><code>target</code></dt>
@@ -8361,19 +8393,55 @@ qemu-kvm -net nic,model=? /dev/null
NUMA nodes configured.
</p>
<p>
- For NVDIMM type devices one can optionally use
- <code>label</code> and its subelement <code>size</code>
- to configure the size of namespaces label storage
- within the NVDIMM module. The <code>size</code> element
- has usual meaning described
- <a href="#elementsMemoryAllocation">here</a>.
+ Besides, the following optional elements can be provided as well for
</p>
- <ol>
- <li>the minimum label size is 128KiB,</li>
- <li>the remaining size (total-size - label-size) has to be aligned to
- 4KiB</li>
- </ol>
+
+ <dl>
+ <dt><code>label</code></dt>
+ <dd>
+ <p>
+ For NVDIMM type devices one can optionally use
+ <code>label</code> and its subelement <code>size</code>
+ to configure the size of namespaces label storage
+ within the NVDIMM module. The <code>size</code> element
+ has usual meaning described
+ <a href="#elementsMemoryAllocation">here</a>.
+ </p>
+ <ol>
+ <li>the minimum label size is 128KiB,</li>
+ <li>the remaining size (total-size - label-size) will be aligned to
+ 4KiB as default.</li>
+ </ol>
+ </dd>
+
+ <dt><code>persistence</code></dt>
+ <dd>
+ <p>
+ The <code>persistence</code> element can be set to "mem-ctl" or "cpu",

"mem-ctrl"

Post by Luyao Zhong
+ which indicate platform-supported features about NVDIMM data persistence.
+ 'mem-ctrl' means the platform supports flushing dirty data from the memory
+ controller to the NVDIMMs in the event of power loss, 'cpu' means The platform

means the platform

Post by Luyao Zhong
+ supports flushing dirty data from the CPU cache to the NVDIMMs in the event
+ of power loss, which contains what 'mem-ctrl' means.
+ ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
+ in NFIT, so the guest ACPI NFIT will be filled in according to the persistence
+ option.
+ </p>
+ </dd>

That's a hard read - someone has to know about ACPI 6.2 Errata A and
whatever it is and where to find it?

Also, I'm not sure this belongs on the NVDIMM line... Allowing it means
more than one could use this and they could use separate values, each
then being placed on the command line and then you don't know what goes
with what.

That is what happens if we have

<memory model='nvdimm'>
...
<target>
...
<persistence>mem-ctrl</persistence>
...
</memory>
<memory model='nvdimm'>
...
<target>
...
<persistence>cpu</persistence>
...
</memory>

Post by Luyao Zhong
+
+ <dt><code>unarmed</code></dt>
+ <dd>
+ <p>
+ The <code>unarmed</code> element can be used to mark vNVDIMM read-only
+ through setting unarmed flag in guest NFIT.Currently the only vNVDIMM backend

What is NFIT?

Add a space before Currently

Post by Luyao Zhong
+ can guarantee the guest write persistence is device DAX on Linux, so it's

What is DAX?

Post by Luyao Zhong
+ suggested to set <code>unarmed</code> to 'on' when using other types of
+ backends.
+ </p>
+ </dd>
+ </dl>

unarmed will be similar to pmem...

Once these are properly separated it'll be easier to review the text in
more detail. But again, like I noted earlier try to use more generic
terms and avoid specifically calling out QEMU. Although that may be
difficult - you have to consider the audience - they just want to know
what the feature does in general and how it will help them or make
things better for them. The details of what we rename it to under the
covers hides the complexity.

Post by Luyao Zhong
</dd>
</dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 099a949..cca7869 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5353,9 +5353,21 @@
</interleave>
</group>
<group>
- <element name="path">
- <ref name="absFilePath"/>
- </element>
+ <interleave>
+ <element name="path">
+ <ref name="absFilePath"/>
+ </element>
+ <optional>
+ <element name="alignsize">
+ <ref name="scaledInteger"/>
+ </element>
+ </optional>
+ <optional>
+ <element name="pmem">
+ <ref name="virOnOff"/>

Again, I'd follow the nosharepages example here.

Post by Luyao Zhong
+ </element>
+ </optional>
+ </interleave>
</group>
</choice>
</element>
@@ -5379,6 +5391,19 @@
</element>
</element>
</optional>
+ <optional>
+ <element name="persistence">
+ <choice>
+ <value>mem-ctrl</value>
+ <value>cpu</value>
+ </choice>
+ </element>
+ </optional>

While the data is correct, the placement isn't...

Post by Luyao Zhong
+ <optional>
+ <element name="unarmed">
+ <ref name="virOnOff"/>

Similar here to be like nosharepages.

John

Post by Luyao Zhong
+ </element>
+ </optional>
</interleave>
</element>
</define>

[Zhong, Luyao]

Post by John Ferlan

Post by Luyao Zhong
In order to align with QEMU ,four more parameters about NVDIMM will
be introduced into Libvirt xml.
1.alignsize
The 'alignsize' option allows users to specify the proper alignment. When
mmap(2) the backend files, QEMU uses the host page size by default as
the alignment of mapping address. However, some backends may require
alignments different from the pagesize. For example, mmap a device DAX
on NVDIMM maybe 2M-aligned.
2.pmem
The 'pmem' option allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory that can be accessed in
SNIA NVM Programming Model. If the vNVDIMM backend is in host persistent
memory , and 'pmem' is 'on' and QEMU is built with libpmem, qemu will
guarantee the persistence of its own writes to the vNVDIMM backend.
3.persistence
The 'persistence' option allows users to set platform-supported features
about NVDIMM data persistence of a guest. It has two values, 'mem-ctrl'
means the platform supports flushing dirty data from the memory controller
to the NVDIMMs in the event of power loss, 'cpu' means The platform supports
flushing dirty data from the CPU cache to the NVDIMMs in the event of power
loss, which contains what 'mem-ctrl' means.
4.unarmed
The 'unarmed' option allows users to mark vNVDIMM read-only. Only one type
of vNVDIMM backends can guarantee the guest write persistence, which is the
device DAX on the real NVDIMM device (e.g., /dev/dax0.0). When using other
types of backends, it's suggested to set 'unarmed' option to 'on', so the
guest Linux NVDIMM driver will mark such vNVDIMM device as read-only.

caveat: I didn't research too deeply into each of these options, but
I'll give you some feedback from the aspect of how you should formulate
your patches.
Rather than essentially replicate what was added in formatdomain provide
the examples here and a summary of what the proposed XML would "look
like", e.g.
Since 'alignsize' and 'pmem' seem to be related specifically to memory
<memory model='nvdimm'>
<source>
<path>/tmp/nvdimm</path>
<alignsize unit='KiB'>2048</alignsize>
<pmem>on</pmem>
</source>
...
</memory>

Got it.

Post by John Ferlan
The <alignsize> would seem to similar in description to the existing
<pagesize> - although yes for a different model type.....

No, the 'alignsize' is totally another conception.It means our data
structure must aligned with a fixed size.

Post by John Ferlan
The <pmem> seems to have two states "on" or "off", thus having just
<pmem/> similar to perhaps how nosharepages is handled.

The 'pmem' has nothing to do with the 'nosharepages'. Actually, the
'nvdimm' model can only support 'shared' access mode now. The 'pmem'
just tell the QEMU if the backend file is a real persistence memory
device, so QEMU will know if itself should guarantee the write persistent.

Post by John Ferlan
Since they're both memory mapped related changes, keep them together
from conf, xml, rng, etc. Then the "next" patch would be to do the qemu
and capability changes. With the last being any virsh changes to allow
modification on the command line (if possible).

Got it.

Post by John Ferlan
The "persistence" seems to be less of a <memory> option and more of a
<machine> option, true/false? It would thus need to be separated from
the others. Still similar to the first one, separate patch for conf,
xml, rng, html... Then patch for qemu...

It really show the platform features, but it's memory-related. So my
opinion is keep this along with other options in <source>, which cover
all configurations about backend NVDIMM on host.

Post by John Ferlan
The "unarmed" is a <target> option since it's being added in patch2
after node and label are processed, so it should be separate from the
<source> and <machine> option patches. If it's just going to have 2
states, then it doesn't need <unarmed>on</unarmed> - instead it could
just be <unarmed/>.

Got it. My opinion is keep it in <target>, which cover all
configurations about guest vNVDIMM.

Post by John Ferlan

Post by Luyao Zhong
For more details, see
https://github.com/qemu/qemu/blob/master/docs/nvdimm.txt

Not a commit message worthy... Place it under the ---...

Post by Luyao Zhong
---
docs/formatdomain.html.in | 98 ++++++++++++++++++++++++++++++++++++-------
docs/schemas/domaincommon.rng | 31 ++++++++++++--
2 files changed, 111 insertions(+), 18 deletions(-)

As noted above, but perhaps difficult to determine - the html.in, .rng,
domain_conf, xml2xml tests, etc. should be in one patch. Then another
patch for the qemu, capabilities, xml2argv tests, etc. in the next
patch. Then if you're going to add virsh options a separate patch for
that (although probably doesn't apply here).

Got it.

Post by John Ferlan
After each patch, running make check syntax-check needs to pass.

Got it.

Post by John Ferlan

Post by Luyao Zhong
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8189959..9dec984 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8258,6 +8258,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8265,6 +8267,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8339,10 +8343,38 @@ qemu-kvm -net nic,model=? /dev/null
</dl>
<p>
- For model <code>nvdimm</code> this element is mandatory and has a
- single child element <code>path</code> that represents a path
- in the host that backs the nvdimm module in the guest.
+ For model <code>nvdimm</code> this element is mandatory. The
+ mandatory child element <code>path</code> represents a path in
+ the host that backs the nvdimm module in the guest. If
+ <code>nvdimm</code> is provided, then the following optional

<span class='since'>since 4.10.0</span>
Whenever something new is added for a specific version, that's what's
necessary.

Got it.

Post by John Ferlan

Post by Luyao Zhong
</p>
+
+ <dl>
+ <dt><code>alignsize</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify a proper alignment.
+ When mmap(2) the backend files, QEMU uses the host page
+ size by default as the alignment of mapping address. However,
+ some backends may require alignments different from the page.
+ For example, mmap a device DAX on NVDIMM maybe 2M-aligned.
+ </p>
+ </dd>
+
+ <dt><code>pmem</code></dt>
+ <dd>
+ <p>
+ This element can be used to specify whether the backend storage
+ of memory-backend-file is a real persistent memory that can be
+ accessed in SNIA NVM Programming Model. If the backend is a real

^
there's two spaces between backend and is

Got it. It's my mistake.

Post by John Ferlan

Post by Luyao Zhong
+ persistence memory and <code>pmem</code> is set to 'on', QEMU will
+ guarantee the persistence of its own writes to the vNVDIMM backend,
+ but which can work well with libpmem support (QEMU configured with
+ --enable-libpmem).
+ </p>
+ </dd>
+ </dl>
</dd>

The probably should be reworded in syntax more suitable for libvirt's
usage without including QEMU details.

Got it. I will reword this.

Post by John Ferlan
The pmem one in particular may be hard to describe... The fact that it
doesn't work without libpmem does raise a couple of warning flags. I
would just say that if <pmem/> is there, then it's on if it's not, then
it's off. Internally that then isn't a tristate, it just an on/off.

Post by Luyao Zhong
<dt><code>target</code></dt>
@@ -8361,19 +8393,55 @@ qemu-kvm -net nic,model=? /dev/null
NUMA nodes configured.
</p>
<p>
- For NVDIMM type devices one can optionally use
- <code>label</code> and its subelement <code>size</code>
- to configure the size of namespaces label storage
- within the NVDIMM module. The <code>size</code> element
- has usual meaning described
- <a href="#elementsMemoryAllocation">here</a>.
+ Besides, the following optional elements can be provided as well for
</p>
- <ol>
- <li>the minimum label size is 128KiB,</li>
- <li>the remaining size (total-size - label-size) has to be aligned to
- 4KiB</li>
- </ol>
+
+ <dl>
+ <dt><code>label</code></dt>
+ <dd>
+ <p>
+ For NVDIMM type devices one can optionally use
+ <code>label</code> and its subelement <code>size</code>
+ to configure the size of namespaces label storage
+ within the NVDIMM module. The <code>size</code> element
+ has usual meaning described
+ <a href="#elementsMemoryAllocation">here</a>.
+ </p>
+ <ol>
+ <li>the minimum label size is 128KiB,</li>
+ <li>the remaining size (total-size - label-size) will be aligned to
+ 4KiB as default.</li>
+ </ol>
+ </dd>
+
+ <dt><code>persistence</code></dt>
+ <dd>
+ <p>
+ The <code>persistence</code> element can be set to "mem-ctl" or "cpu",

"mem-ctrl"

Post by Luyao Zhong
+ which indicate platform-supported features about NVDIMM data persistence.
+ 'mem-ctrl' means the platform supports flushing dirty data from the memory
+ controller to the NVDIMMs in the event of power loss, 'cpu' means The platform

means the platform

Post by Luyao Zhong
+ supports flushing dirty data from the CPU cache to the NVDIMMs in the event
+ of power loss, which contains what 'mem-ctrl' means.
+ ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
+ in NFIT, so the guest ACPI NFIT will be filled in according to the persistence
+ option.
+ </p>
+ </dd>

That's a hard read - someone has to know about ACPI 6.2 Errata A and
whatever it is and where to find it?

Sorry, maybe I put too much details here, there is no need to have
knowledge about ACPI. I will delete the ACPI things.

Post by John Ferlan
Also, I'm not sure this belongs on the NVDIMM line... Allowing it means
more than one could use this and they could use separate values, each
then being placed on the command line and then you don't know what goes
with what.
That is what happens if we have
<memory model='nvdimm'>
...
<target>
...
<persistence>mem-ctrl</persistence>
...
</memory>
<memory model='nvdimm'>
...
<target>
...
<persistence>cpu</persistence>
...
</memory>

I have not considered this situation, I need do more tests and find a
solution.

Post by John Ferlan

Post by Luyao Zhong
+
+ <dt><code>unarmed</code></dt>
+ <dd>
+ <p>
+ The <code>unarmed</code> element can be used to mark vNVDIMM read-only
+ through setting unarmed flag in guest NFIT.Currently the only vNVDIMM backend

What is NFIT?

'NVDIMM Firmware Interface Table' defined in ACPI. I will delete this
for its hard to understand and not very necessary.

Post by John Ferlan
Add a space before Currently

Got it.

Post by John Ferlan

Post by Luyao Zhong
+ can guarantee the guest write persistence is device DAX on Linux, so it's

What is DAX?

'DAX' means 'direct access'.

Post by John Ferlan

Post by Luyao Zhong
+ suggested to set <code>unarmed</code> to 'on' when using other types of
+ backends.
+ </p>
+ </dd>
+ </dl>

unarmed will be similar to pmem...
Once these are properly separated it'll be easier to review the text in
more detail. But again, like I noted earlier try to use more generic
terms and avoid specifically calling out QEMU. Although that may be
difficult - you have to consider the audience - they just want to know
what the feature does in general and how it will help them or make
things better for them. The details of what we rename it to under the
covers hides the complexity.

Post by Luyao Zhong
</dd>
</dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 099a949..cca7869 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5353,9 +5353,21 @@
</interleave>
</group>
<group>
- <element name="path">
- <ref name="absFilePath"/>
- </element>
+ <interleave>
+ <element name="path">
+ <ref name="absFilePath"/>
+ </element>
+ <optional>
+ <element name="alignsize">
+ <ref name="scaledInteger"/>
+ </element>
+ </optional>
+ <optional>
+ <element name="pmem">
+ <ref name="virOnOff"/>

Again, I'd follow the nosharepages example here.

See my explain above, NVDIMM model only supports 'shared' access now and
the <pmem> has nothing to do with 'nosharepages'.

Post by John Ferlan

Post by Luyao Zhong
+ </element>
+ </optional>
+ </interleave>
</group>
</choice>
</element>
@@ -5379,6 +5391,19 @@
</element>
</element>
</optional>
+ <optional>
+ <element name="persistence">
+ <choice>
+ <value>mem-ctrl</value>
+ <value>cpu</value>
+ </choice>
+ </element>
+ </optional>

While the data is correct, the placement isn't...

Post by Luyao Zhong
+ <optional>
+ <element name="unarmed">
+ <ref name="virOnOff"/>

Similar here to be like nosharepages.

The <unarmed> has nothing to do with 'nosharepages'.

Post by John Ferlan
John

Thanks for your review and so much detailed comments.
Luyao

Post by John Ferlan

Post by Luyao Zhong
+ </element>
+ </optional>
</interleave>
</element>
</define>

[Zhong, Luyao]

On 11/16/2018 11:35 PM, Zhong, Luyao wrote:

I maybe misunderstand some comments. so sorry for that. I have updated
my reply.

Post by Zhong, Luyao

Post by John Ferlan

Post by Luyao Zhong
In order to align with QEMU ,four more parameters about NVDIMM will
be introduced into Libvirt xml.
1.alignsize
The 'alignsize' option allows users to specify the proper alignment. When
mmap(2) the backend files, QEMU uses the host page size by default as
the alignment of mapping address. However, some backends may require
alignments different from the pagesize. For example, mmap a device DAX
on NVDIMM maybe 2M-aligned.
2.pmem
The 'pmem' option allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory that can be accessed in
SNIA NVM Programming Model. If the vNVDIMM backend is in host persistent
memory , and 'pmem' is 'on' and QEMU is built with libpmem, qemu will
guarantee the persistence of its own writes to the vNVDIMM backend.
3.persistence
The 'persistence' option allows users to set platform-supported features
about NVDIMM data persistence of a guest. It has two values, 'mem-ctrl'
means the platform supports flushing dirty data from the memory controller
to the NVDIMMs in the event of power loss, 'cpu' means The platform supports
flushing dirty data from the CPU cache to the NVDIMMs in the event of power
loss, which contains what 'mem-ctrl' means.
4.unarmed
The 'unarmed' option allows users to mark vNVDIMM read-only. Only one type
of vNVDIMM backends can guarantee the guest write persistence, which is the
device DAX on the real NVDIMM device (e.g., /dev/dax0.0). When using other
types of backends, it's suggested to set 'unarmed' option to 'on', so the
guest Linux NVDIMM driver will mark such vNVDIMM device as read-only.

caveat: I didn't research too deeply into each of these options, but
I'll give you some feedback from the aspect of how you should formulate
your patches.
Rather than essentially replicate what was added in formatdomain provide
the examples here and a summary of what the proposed XML would "look
like", e.g.
Since 'alignsize' and 'pmem' seem to be related specifically to memory
   <memory model='nvdimm'>
     <source>
       <path>/tmp/nvdimm</path>
       <alignsize unit='KiB'>2048</alignsize>
       <pmem>on</pmem>
     </source>
     ...
   </memory>

Got it.

Post by John Ferlan
The <alignsize> would seem to similar in description to the existing
<pagesize> - although yes for a different model type.....

No, the 'alignsize' is totally another conception.It means our data
structure must aligned with a fixed size.

Post by John Ferlan
The <pmem> seems to have two states "on" or "off", thus having just
<pmem/> similar to perhaps how nosharepages is handled.

The 'pmem' has nothing to do with the 'nosharepages'. Actually, the
'nvdimm' model can only support 'shared' access mode now. The 'pmem'
just tell the QEMU if the backend file is a real persistence memory
device, so QEMU will know if itself should guarantee the write persistent.

Ignore the reply above.I will check nosharepages usage first.

Post by Zhong, Luyao

Post by John Ferlan
Since they're both memory mapped related changes, keep them together
from conf, xml, rng, etc. Then the "next" patch would be to do the qemu
and capability changes. With the last being any virsh changes to allow
modification on the command line (if possible).

Got it.

Post by John Ferlan
The "persistence" seems to be less of a <memory> option and more of a
<machine> option, true/false?  It would thus need to be separated from
the others. Still similar to the first one, separate patch for conf,
xml, rng, html... Then patch for qemu...

It really show the platform features, but it's memory-related. So my
opinion is keep this along with other options in <source>, which cover
all configurations about backend NVDIMM on host.

Sorry for misunderstanding your comment. Thank you very much for point
that. It's really a little bit big problem. I think I need redesign this
'persistence' part. My opinion is kicking it out and put it into another
patch set.

Post by Zhong, Luyao

Post by John Ferlan
The "unarmed" is a <target> option since it's being added in patch2
after node and label are processed, so it should be separate from the
<source> and <machine> option patches.  If it's just going to have 2
states, then it doesn't need <unarmed>on</unarmed> - instead it could
just be <unarmed/>.

Got it. My opinion is keep it in <target>, which cover all
configurations about guest vNVDIMM.

Post by John Ferlan

Post by Luyao Zhong
For more details, see
https://github.com/qemu/qemu/blob/master/docs/nvdimm.txt

Not a commit message worthy...  Place it under the ---...

Post by Luyao Zhong
---
  docs/formatdomain.html.in     | 98
++++++++++++++++++++++++++++++++++++-------
  docs/schemas/domaincommon.rng | 31 ++++++++++++--
  2 files changed, 111 insertions(+), 18 deletions(-)

As noted above, but perhaps difficult to determine - the html.in, .rng,
domain_conf, xml2xml tests, etc. should be in one patch.  Then another
patch for the qemu, capabilities, xml2argv tests, etc. in the next
patch. Then if you're going to add virsh options a separate patch for
that (although probably doesn't apply here).

Got it.

Post by John Ferlan
After each patch, running make check syntax-check needs to pass.

Got it.

Post by John Ferlan

Post by Luyao Zhong
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8189959..9dec984 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8258,6 +8258,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8265,6 +8267,8 @@ qemu-kvm -net nic,model=? /dev/null
@@ -8339,10 +8343,38 @@ qemu-kvm -net nic,model=? /dev/null
          </dl>
          <p>
-          For model <code>nvdimm</code> this element is mandatory
and has a
-          single child element <code>path</code> that represents a path
-          in the host that backs the nvdimm module in the guest.
+          For model <code>nvdimm</code> this element is mandatory. The
+          mandatory child element <code>path</code> represents a
path in
+          the host that backs the nvdimm module in the guest. If
+          <code>nvdimm</code> is provided, then the following optional

<span class='since'>since 4.10.0</span>
Whenever something new is added for a specific version, that's what's
necessary.

Got it.

Post by John Ferlan

Post by Luyao Zhong
          </p>
+
+        <dl>
+          <dt><code>alignsize</code></dt>
+          <dd>
+            <p>
+              This element can be used to specify a proper alignment.
+              When mmap(2) the backend files, QEMU uses the host page
+              size by default as the alignment of mapping address.
However,
+              some backends may require alignments different from
the page.
+              For example, mmap a device DAX on NVDIMM maybe
2M-aligned.
+            </p>
+          </dd>
+
+          <dt><code>pmem</code></dt>
+          <dd>
+            <p>
+              This element can be used to specify whether the
backend storage
+              of memory-backend-file is a real persistent memory
that can be
+              accessed in SNIA NVM Programming Model. If the
backend  is a real

                                                                        ^
there's two spaces between backend and is

Got it. It's my mistake.

Post by John Ferlan

Post by Luyao Zhong
+              persistence memory and <code>pmem</code> is set to
'on', QEMU will
+              guarantee the persistence of its own writes to the
vNVDIMM backend,
+              but which can work well with libpmem support (QEMU
configured with
+              --enable-libpmem).
+            </p>
+          </dd>
+        </dl>
        </dd>

The probably should be reworded in syntax more suitable for libvirt's
usage without including QEMU details.

Got it. I will reword this.

Post by John Ferlan
The pmem one in particular may be hard to describe... The fact that it
doesn't work without libpmem does raise a couple of warning flags. I
would just say that if <pmem/> is there, then it's on if it's not, then
it's off. Internally that then isn't a tristate, it just an on/off.

Post by Luyao Zhong
        <dt><code>target</code></dt>
@@ -8361,19 +8393,55 @@ qemu-kvm -net nic,model=? /dev/null
            NUMA nodes configured.
          </p>
          <p>
-          For NVDIMM type devices one can optionally use
-          <code>label</code> and its subelement <code>size</code>
-          to configure the size of namespaces label storage
-          within the NVDIMM module. The <code>size</code> element
-          has usual meaning described
-          <a href="#elementsMemoryAllocation">here</a>.
+          Besides, the following optional elements can be provided
as well for
          </p>
-        <ol>
-          <li>the minimum label size is 128KiB,</li>
-          <li>the remaining size (total-size - label-size) has to be
aligned to
-            4KiB</li>
-        </ol>
+
+        <dl>
+          <dt><code>label</code></dt>
+          <dd>
+            <p>
+              For NVDIMM type devices one can optionally use
+              <code>label</code> and its subelement <code>size</code>
+              to configure the size of namespaces label storage
+              within the NVDIMM module. The <code>size</code> element
+              has usual meaning described
+              <a href="#elementsMemoryAllocation">here</a>.
+            </p>
+            <ol>
+              <li>the minimum label size is 128KiB,</li>
+              <li>the remaining size (total-size - label-size) will
be aligned to
+                4KiB as default.</li>
+            </ol>
+          </dd>
+
+          <dt><code>persistence</code></dt>
+          <dd>
+            <p>
+              The <code>persistence</code> element can be set to
"mem-ctl" or "cpu",

"mem-ctrl"

Post by Luyao Zhong
+              which indicate platform-supported features about
NVDIMM data persistence.
+              'mem-ctrl' means the platform supports flushing dirty
data from the memory
+              controller to the NVDIMMs in the event of power loss,
'cpu' means The platform

means the platform

Post by Luyao Zhong
+              supports flushing dirty data from the CPU cache to the
NVDIMMs in the event
+              of power loss, which contains what 'mem-ctrl' means.
+              ACPI 6.2 Errata A added support for a new Platform
Capabilities Structure
+              in NFIT, so the guest ACPI NFIT will be filled in
according to the persistence
+              option.
+            </p>
+          </dd>

That's a hard read - someone has to know about ACPI 6.2 Errata A and
whatever it is and where to find it?

Sorry, maybe I put too much details here, there is no need to have
knowledge about ACPI. I will delete the ACPI things.

Post by John Ferlan
Also, I'm not sure this belongs on the NVDIMM line... Allowing it means
more than one could use this and they could use separate values, each
then being placed on the command line and then you don't know what goes
with what.
That is what happens if we have
   <memory model='nvdimm'>
   ...
     <target>
     ...
       <persistence>mem-ctrl</persistence>
     ...
   </memory>
   <memory model='nvdimm'>
   ...
     <target>
     ...
       <persistence>cpu</persistence>
     ...
   </memory>

I have not considered this situation, I need do more tests and find a
solution.

Post by John Ferlan

Post by Luyao Zhong
+
+          <dt><code>unarmed</code></dt>
+          <dd>
+            <p>
+              The <code>unarmed</code> element can be used to mark
vNVDIMM read-only
+              through setting unarmed flag in guest NFIT.Currently
the only vNVDIMM backend

What is NFIT?

'NVDIMM Firmware Interface Table' defined in ACPI. I will delete this
for its hard to understand and not very necessary.

Post by John Ferlan
Add a space before Currently

Got it.

Post by John Ferlan

Post by Luyao Zhong
+              can guarantee the guest write persistence is device
DAX on Linux, so it's

What is DAX?

'DAX' means 'direct access'.

Post by John Ferlan

Post by Luyao Zhong
+              suggested to set <code>unarmed</code> to 'on' when
using other types of
+              backends.
+            </p>
+          </dd>
+        </dl>

unarmed will be similar to pmem...
Once these are properly separated it'll be easier to review the text in
more detail. But again, like I noted earlier try to use more generic
terms and avoid specifically calling out QEMU. Although that may be
difficult - you have to consider the audience - they just want to know
what the feature does in general and how it will help them or make
things better for them. The details of what we rename it to under the
covers hides the complexity.

Post by Luyao Zhong
        </dd>
      </dl>
diff --git a/docs/schemas/domaincommon.rng
b/docs/schemas/domaincommon.rng
index 099a949..cca7869 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5353,9 +5353,21 @@
            </interleave>
          </group>
          <group>
-          <element name="path">
-            <ref name="absFilePath"/>
-          </element>
+          <interleave>
+            <element name="path">
+              <ref name="absFilePath"/>
+            </element>
+            <optional>
+              <element name="alignsize">
+                <ref name="scaledInteger"/>
+              </element>
+            </optional>
+            <optional>
+              <element name="pmem">
+                <ref name="virOnOff"/>

Again, I'd follow the nosharepages example here.

See my explain above, NVDIMM model only supports 'shared' access now and
the <pmem> has nothing to do with 'nosharepages'.

Ignore the reply above.I will check nosharepages usage first.

Post by Zhong, Luyao

Post by John Ferlan

Post by Luyao Zhong
+              </element>
+            </optional>
+          </interleave>
          </group>
        </choice>
      </element>
@@ -5379,6 +5391,19 @@
              </element>
            </element>
          </optional>
+        <optional>
+          <element name="persistence">
+            <choice>
+              <value>mem-ctrl</value>
+              <value>cpu</value>
+            </choice>
+          </element>
+        </optional>

While the data is correct, the placement isn't...

Post by Luyao Zhong
+        <optional>
+          <element name="unarmed">
+            <ref name="virOnOff"/>

Similar here to be like nosharepages.

The <unarmed> has nothing to do with 'nosharepages'.

Ignore the reply above.I will check nosharepages usage first.

Post by Zhong, Luyao

Post by John Ferlan
John

Thanks for your review and so much detailed comments.
Luyao

Post by John Ferlan

Post by Luyao Zhong
+          </element>
+        </optional>
        </interleave>
      </element>
    </define>

[Han Han]

Post by Luyao Zhong
Hi libvirt experts,
This is the RFC for updating NVDIMM support in libvirt.
QEMU has supported four more properties which libvirt has not introduced
yet, including 'align', 'pmem', 'nvdimm-persistences' and 'unarmed'.
The 'align' property allows users to specify the proper alignment. The
previous alignment can only be 4K because QEMU use pagesize as alignment.
But some backends may require alignments different from the pagesize.
The 'pmem' property allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory. Then QEMU will know if
it needs to guarrantee the write persistence to the vNVDIMM backend.
The 'nvdimm-persistence' property allows users to set platform-supported
features about NVDIMM data persistence of a guest.
The 'unarmed' property allows users to mark vNVDIMM read-only. Only the
device DAX on the real NVDIMM can guarantee the guest write persistence,
so it's suggested to set 'unarmed' option to 'on' and then vNVDIMM device
will be marked as read-only.
Libvirt introduces 'alignsize', 'pmem', 'persistence' and 'unarmed' config
elements into xml corresponding to 'align', 'pmem', 'nvdimm-persistence'
and 'unarmed' properties in QEMU, and update xml parsing, formating and
qemu command-line generating process for NVDIMM.
Thanks,
Zhong, Luyao
xml: introduce more config elements for NVDIMM memory
xml: update xml parsing and formating about NVDIMM memory
qemu: update qemu command-line generating for NVDIMM memory
docs/formatdomain.html.in | 98
+++++++++++++++---
docs/schemas/domaincommon.rng | 31 +++++-

Please add a patch to update the release news in news.xml.

Post by Luyao Zhong
src/conf/domain_conf.c | 115
+++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
src/qemu/qemu_command.c | 25 +++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++
.../memory-hotplug-nvdimm-align.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 +++++++++++
tests/qemuxml2argvtest.c | 12 +++
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
20 files changed, 636 insertions(+), 25 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
--
2.7.4
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

[Luyao Zhong]

Hi Han,

I'm not sure which release my patches will merge into. How about adding
the patch to update the release news after my last version of these
patches.  Waiting for more reviews and comments.

Regards,
Luyao Zhong

Post by Luyao Zhong
Hi libvirt experts,
This is the RFC for updating NVDIMM support in libvirt.
QEMU has supported four more properties which libvirt has not introduced
yet, including 'align', 'pmem', 'nvdimm-persistences' and 'unarmed'.
The 'align' property allows users to specify the proper alignment. The
previous alignment can only be 4K because QEMU use pagesize as alignment.
But some backends may require alignments different from the pagesize.
The 'pmem' property allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory. Then QEMU will know if
it needs to guarrantee the write persistence to the vNVDIMM backend.
The 'nvdimm-persistence' property allows users to set
platform-supported
features about NVDIMM data persistence of a guest.
The 'unarmed' property allows users to mark vNVDIMM read-only. Only the
device DAX on the real NVDIMM can guarantee the guest write persistence,
so it's suggested to set 'unarmed' option to 'on' and then vNVDIMM device
will be marked as read-only.
Libvirt introduces 'alignsize', 'pmem', 'persistence' and
'unarmed' config
elements into xml corresponding to 'align', 'pmem',
'nvdimm-persistence'
and 'unarmed' properties in QEMU, and update xml parsing,
formating and
qemu command-line generating process for NVDIMM.
Thanks,
Zhong, Luyao
  xml: introduce more config elements for NVDIMM memory
  xml: update xml parsing and formating about NVDIMM memory
  qemu: update qemu command-line generating for NVDIMM memory
 docs/formatdomain.html.in <http://formatdomain.html.in>          
              |  98 +++++++++++++++---
 docs/schemas/domaincommon.rng                      |  31 +++++-
Please add a patch to update the release news in news.xml.
 src/conf/domain_conf.c                             | 115
+++++++++++++++++++--
 src/conf/domain_conf.h                             |  14 +++
 src/libvirt_private.syms                           |   2 +
 src/qemu/qemu_command.c                            |  25 +++++
 .../memory-hotplug-nvdimm-align.args               |  31 ++++++
 .../memory-hotplug-nvdimm-align.xml                |  58 +++++++++++
 .../memory-hotplug-nvdimm-persistence.args         |  31 ++++++
 .../memory-hotplug-nvdimm-persistence.xml          |  58 +++++++++++
 .../memory-hotplug-nvdimm-pmem.args                |  31 ++++++
 .../memory-hotplug-nvdimm-pmem.xml                 |  58 +++++++++++
 .../memory-hotplug-nvdimm-unarmed.args             |  31 ++++++
 .../memory-hotplug-nvdimm-unarmed.xml              |  58 +++++++++++
 tests/qemuxml2argvtest.c                           |  12 +++
 .../memory-hotplug-nvdimm-align.xml                |   1 +
 .../memory-hotplug-nvdimm-persistence.xml          |   1 +
 .../memory-hotplug-nvdimm-pmem.xml                 |   1 +
 .../memory-hotplug-nvdimm-unarmed.xml              |   1 +
 tests/qemuxml2xmltest.c                            |   4 +
 20 files changed, 636 insertions(+), 25 deletions(-)
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
 create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
 create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
 create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
 create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
 create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
--
2.7.4
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Phone: [phone]
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

[Han Han]

Post by Luyao Zhong
Hi Han,
I'm not sure which release my patches will merge into. How about adding
the patch to update the release news after my last version of these
patches. Waiting for more reviews and comments.

Well. That's OK.

Post by Luyao Zhong
Regards,
Luyao Zhong

Post by Luyao Zhong
Hi libvirt experts,
This is the RFC for updating NVDIMM support in libvirt.
QEMU has supported four more properties which libvirt has not introduced
yet, including 'align', 'pmem', 'nvdimm-persistences' and 'unarmed'.
The 'align' property allows users to specify the proper alignment. The
previous alignment can only be 4K because QEMU use pagesize as alignment.
But some backends may require alignments different from the pagesize.
The 'pmem' property allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory. Then QEMU will know if
it needs to guarrantee the write persistence to the vNVDIMM backend.
The 'nvdimm-persistence' property allows users to set platform-supported
features about NVDIMM data persistence of a guest.
The 'unarmed' property allows users to mark vNVDIMM read-only. Only the
device DAX on the real NVDIMM can guarantee the guest write persistence,
so it's suggested to set 'unarmed' option to 'on' and then vNVDIMM device
will be marked as read-only.
Libvirt introduces 'alignsize', 'pmem', 'persistence' and 'unarmed' config
elements into xml corresponding to 'align', 'pmem', 'nvdimm-persistence'
and 'unarmed' properties in QEMU, and update xml parsing, formating and
qemu command-line generating process for NVDIMM.
Thanks,
Zhong, Luyao
xml: introduce more config elements for NVDIMM memory
xml: update xml parsing and formating about NVDIMM memory
qemu: update qemu command-line generating for NVDIMM memory
docs/formatdomain.html.in | 98
+++++++++++++++---
docs/schemas/domaincommon.rng | 31 +++++-

Please add a patch to update the release news in news.xml.

Post by Luyao Zhong
src/conf/domain_conf.c | 115
+++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
src/qemu/qemu_command.c | 25 +++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++
.../memory-hotplug-nvdimm-align.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 +++++++++++
tests/qemuxml2argvtest.c | 12 +++
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
20 files changed, 636 insertions(+), 25 deletions(-)
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml
--
2.7.4
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Phone: [phone]
--

[Luyao Zhong]

polite ping

Post by Luyao Zhong
Hi libvirt experts,
This is the RFC for updating NVDIMM support in libvirt.
QEMU has supported four more properties which libvirt has not introduced
yet, including 'align', 'pmem', 'nvdimm-persistences' and 'unarmed'.
The 'align' property allows users to specify the proper alignment. The
previous alignment can only be 4K because QEMU use pagesize as alignment.
But some backends may require alignments different from the pagesize.
The 'pmem' property allows users to specify whether the backend storage of
memory-backend-file is a real persistent memory. Then QEMU will know if
it needs to guarrantee the write persistence to the vNVDIMM backend.
The 'nvdimm-persistence' property allows users to set platform-supported
features about NVDIMM data persistence of a guest.
The 'unarmed' property allows users to mark vNVDIMM read-only. Only the
device DAX on the real NVDIMM can guarantee the guest write persistence,
so it's suggested to set 'unarmed' option to 'on' and then vNVDIMM device
will be marked as read-only.
Libvirt introduces 'alignsize', 'pmem', 'persistence' and 'unarmed' config
elements into xml corresponding to 'align', 'pmem', 'nvdimm-persistence'
and 'unarmed' properties in QEMU, and update xml parsing, formating and
qemu command-line generating process for NVDIMM.
Thanks,
Zhong, Luyao
xml: introduce more config elements for NVDIMM memory
xml: update xml parsing and formating about NVDIMM memory
qemu: update qemu command-line generating for NVDIMM memory
docs/formatdomain.html.in | 98 +++++++++++++++---
docs/schemas/domaincommon.rng | 31 +++++-
src/conf/domain_conf.c | 115 +++++++++++++++++++--
src/conf/domain_conf.h | 14 +++
src/libvirt_private.syms | 2 +
src/qemu/qemu_command.c | 25 +++++
.../memory-hotplug-nvdimm-align.args | 31 ++++++
.../memory-hotplug-nvdimm-align.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-persistence.args | 31 ++++++
.../memory-hotplug-nvdimm-persistence.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-pmem.args | 31 ++++++
.../memory-hotplug-nvdimm-pmem.xml | 58 +++++++++++
.../memory-hotplug-nvdimm-unarmed.args | 31 ++++++
.../memory-hotplug-nvdimm-unarmed.xml | 58 +++++++++++
tests/qemuxml2argvtest.c | 12 +++
.../memory-hotplug-nvdimm-align.xml | 1 +
.../memory-hotplug-nvdimm-persistence.xml | 1 +
.../memory-hotplug-nvdimm-pmem.xml | 1 +
.../memory-hotplug-nvdimm-unarmed.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
20 files changed, 636 insertions(+), 25 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args
create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-persistence.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml
create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml