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

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(a)intel.com&gt; > --- > 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.

> + > 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.