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

Re: [libvirt] [RFC v2 2/3] nvdimm: update qemu command-line generating for NVDIMM memory





On 2018/11/28 下午10:32, Peter Krempa wrote:
On Wed, Nov 28, 2018 at 22:09:01 +0800, Luyao Zhong wrote:
According to the result parsing from xml, add corresponding properties
into QEMU command line, including 'align', 'pmem', 'unarmed' and
'nvdimm-persistence'.

Signed-off-by: Luyao Zhong <luyao zhong intel com>
---
  src/qemu/qemu_capabilities.c                  | 17 +++++++
  src/qemu/qemu_capabilities.h                  |  5 ++
  src/qemu/qemu_command.c                       | 50 ++++++++++++++++++-
  src/qemu/qemu_command.h                       |  3 +-
  src/qemu/qemu_hotplug.c                       |  2 +-
  .../memory-hotplug-nvdimm-align.args          | 31 ++++++++++++
  .../memory-hotplug-nvdimm-persistence.args    | 31 ++++++++++++
  .../memory-hotplug-nvdimm-pmem.args           | 31 ++++++++++++
  .../memory-hotplug-nvdimm-unarmed.args        | 31 ++++++++++++
  tests/qemuxml2argvtest.c                      | 15 ++++++
  10 files changed, 212 insertions(+), 4 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-persistence.args
  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args
  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 20a1a0c201..1bb9ceb888 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -516,6 +516,11 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
                "memory-backend-memfd.hugetlb",
                "iothread.poll-max-ns",
                "machine.pseries.cap-nested-hv",
+              "nvdimm-align",
+              "nvdimm-pmem",
+
+              /* 325 */
+              "nvdimm-unarmed",
      );
@@ -4190,6 +4195,18 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
          virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM))
          virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM);
+ if (qemuCaps->version < 2001200 &&
+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN))
+        virQEMUCapsClear(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN);
+
+    if (qemuCaps->version < 3000000 &&
+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM_UNARMED))
+        virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM_UNARMED);
+
+    if (qemuCaps->version < 3001000 &&
+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM))
+        virQEMUCapsClear(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM);

This is completely broken. This code just clears the capability bits for
older versions but really does never set them anywhere. So they'll never
be present in real caps.

That is visible that in the fact that this patch does not trigger an
update of capability output test files, which it should.

Additionally these really should check the presence of the fields in the
device properties. For properties of object memory-backend-file we
already call the appropriate qom-list-properties, so they will be
trivial to modify.

For the nvdimm property it will be slightly more hassle, but we really
don't want to see version number based capabilities.

Sorry, I'm a very new libvirt developer and not very familiar with libvirt, so I feel a little confused about this. I don't know how to add a new qemu capability correctly, could you give an example as my reference? Thank you in advance.

Besides, I noticed that the one previous patch 'Introduce label-size for NVDIMMs' (See e433546bef6ff5be92aa0cfca7794fff67c80cac) do not touch the qemu capabilities things, Can I do like that?


+
      if (ARCH_IS_X86(qemuCaps->arch) &&
          virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
          virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_CACHE);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b1990b6bb8..8d5fc5e8e5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -500,6 +500,11 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
      QEMU_CAPS_OBJECT_MEMORY_MEMFD_HUGETLB, /* -object memory-backend-memfd.hugetlb */
      QEMU_CAPS_IOTHREAD_POLLING, /* -object iothread.poll-max-ns */
      QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV, /* -machine pseries.cap-nested-hv */
+    QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN, /* -object memory-backend-file supports align property */
+    QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file supports pmem property*/
+
+    /* 325 */
+    QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm supports unarmed property*/
QEMU_CAPS_LAST /* this must always be the last item */
  } virQEMUCapsFlags;

[...]

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index eae2b7edf7..5a1677ec01 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2754,6 +2754,21 @@ 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,
+            QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN);
+    DO_TEST("memory-hotplug-nvdimm-pmem",
+            QEMU_CAPS_DEVICE_NVDIMM,
+            QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE,
+            QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM);
+    DO_TEST("memory-hotplug-nvdimm-unarmed",
+            QEMU_CAPS_DEVICE_NVDIMM,
+            QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE,
+            QEMU_CAPS_DEVICE_NVDIMM_UNARMED);
+    DO_TEST("memory-hotplug-nvdimm-persistence",
+            QEMU_CAPS_DEVICE_NVDIMM,
+            QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);

Please use DO_TEST_CAPS_LATEST. That way it's tested with real
capabilities and not something you've made up.

Here it would be visible, that your capability setting does not work at
all.

Also please separate capability changes from command line generator
modifications.

Got it, but as I mentioned above, why does the 'memory-hotplug-nvdimm-label' test not use the DO_TEST_CAPS_LATEST. Thank you very much.


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