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

[Peter Krempa]

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


> +
>      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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181128/faa7c998/attachment-0001.sig>