[libvirt] [RFC 2/3] xml: update xml parsing and formating about NVDIMM memory
Zhong, Luyao
luyao.zhong at intel.com
Sat Nov 17 12:20:07 UTC 2018
On 11/8/2018 5:15 AM, John Ferlan wrote:
>
>
> NB: I had to remove "Zhong at redhat.com" from the CC line since it failed
> to send.
>
> On 10/16/18 10:21 PM, Luyao Zhong wrote:
>> 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 <luyao.zhong at 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
>>
>
> 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.
> 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.
>> 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))) {
>
> This is a much simpler check following nosharepages' model.
>
>> + 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);
>
> 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.
> 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.
>
>> +
>> + 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.
>> }
>>
>> 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;
>> + }
>
> Follow nosharepages and not tristate
>
Got it, I will check
>> +
>> + 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.
>> }
>>
>> 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));
>
> Much more simple following nosharepages.
>
Got it.
>
>> 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));
>
> Again.
>
Got it.
>>
>> 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.
>>
>> /* 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.
>>
>> 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
>> + 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);
>>
More information about the libvir-list
mailing list