[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC 2/3] xml: update xml parsing and formating about NVDIMM memory





On 11/8/2018 5:15 AM, John Ferlan wrote:


NB: I had to remove "Zhong 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 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);



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]