[libvirt] [PATCH v2 04/14] qemu: Implement NVDIMM
John Ferlan
jferlan at redhat.com
Tue Mar 7 16:01:24 UTC 2017
On 02/27/2017 08:19 AM, Michal Privoznik wrote:
> So, majority of the code is just ready as-is. Well, with one
> slight change: differentiate between dimm and nvdimm in places
> like device alias generation, generating the command line and so
> on.
>
> Speaking of the command line, we also need to append 'nvdimm=on'
> to the '-machine' argument so that the nvdimm feature is
> advertised in the ACPI tables properly.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> src/qemu/qemu_alias.c | 10 ++-
> src/qemu/qemu_command.c | 91 +++++++++++++++-------
> src/qemu/qemu_command.h | 1 +
> src/qemu/qemu_domain.c | 34 +++++---
> src/qemu/qemu_hotplug.c | 3 +-
> .../qemuxml2argv-memory-hotplug-nvdimm.args | 26 +++++++
> tests/qemuxml2argvtest.c | 4 +-
> 7 files changed, 128 insertions(+), 41 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args
>
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 4cccf231e..05e1293ef 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -352,17 +352,23 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
> size_t i;
> int maxidx = 0;
> int idx;
> + const char *prefix;
> +
> + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM)
> + prefix = "dimm";
> + else
> + prefix = "nvdimm";
>
> if (oldAlias) {
> for (i = 0; i < def->nmems; i++) {
> - if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx)
> + if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, prefix)) >= maxidx)
> maxidx = idx + 1;
> }
> } else {
> maxidx = mem->info.addr.dimm.slot;
> }
>
> - if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0)
> + if (virAsprintf(&mem->info.alias, "%s%d", prefix, maxidx) < 0)
> return -1;
>
> return 0;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f628a9929..46a6ca9f0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3089,6 +3089,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> * @userNodeset: user requested map of host nodes to alloc the memory on, NULL
> * for default
> * @autoNodeset: fallback nodeset in case of automatic NUMA placement
> + * @memPathReq: request memory-backend-file with specific mem-path
> * @force: forcibly use one of the backends
> *
> * Creates a configuration object that represents memory backend of given guest
> @@ -3103,6 +3104,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> * Then, if one of the two memory-backend-* should be used, the @qemuCaps is
> * consulted to check if qemu does support it.
> *
> + * If @memPathReq is non-NULL, memory-backend-file is used with passed path.
> + *
I think if you consider my suggestion in patch 1, then this parameter
becomes unnecessary and all that has to happen is the one place where
the 'mem' is built on the stack would need to decide to add the field or
not.
> * Returns: 0 on success,
> * 1 on success and if there's no need to use memory-backend-*
> * -1 on error.
> @@ -3118,6 +3121,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> unsigned long long pagesize,
> virBitmapPtr userNodeset,
> virBitmapPtr autoNodeset,
> + const char *memPathReq,
> bool force)
> {
> virDomainHugePagePtr master_hugepage = NULL;
> @@ -3126,7 +3130,8 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> const long system_page_size = virGetSystemPageSizeKB();
> virDomainMemoryAccess memAccess = VIR_DOMAIN_MEMORY_ACCESS_DEFAULT;
> size_t i;
> - char *mem_path = NULL;
> + char *memPathActual = NULL;
> + bool prealloc = false;
> virBitmapPtr nodemask = NULL;
> int ret = -1;
> virJSONValuePtr props = NULL;
> @@ -3206,27 +3211,36 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> if (!(props = virJSONValueNewObject()))
> return -1;
>
> - if (pagesize || def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> + if (pagesize || memPathReq ||
> + def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> *backendType = "memory-backend-file";
>
> - if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> + if (memPathReq) {
> + if (VIR_STRDUP(memPathActual, memPathReq) < 0)
> + goto cleanup;
> + prealloc = true;
> + } else if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> /* we can have both pagesize and mem source, then check mem source first */
Seeing as you're changing code in here anyway - could the comment span 2
lines so as to not exceed 80 chars?
> + if (VIR_STRDUP(memPathActual, cfg->memoryBackingDir) < 0)
> + goto cleanup;
> force = true;
> - if (virJSONValueObjectAdd(props,
> - "s:mem-path", cfg->memoryBackingDir,
> - NULL) < 0)
> - goto cleanup;
> } else {
> - if (qemuGetDomainHupageMemPath(def, cfg, pagesize, &mem_path) < 0)
> - goto cleanup;
> -
> - if (virJSONValueObjectAdd(props,
> - "b:prealloc", true,
> - "s:mem-path", mem_path,
> - NULL) < 0)
> + if (qemuGetDomainHupageMemPath(def, cfg, pagesize, &memPathActual) < 0)
> goto cleanup;
> + prealloc = true;
> }
>
> + if (prealloc &&
> + virJSONValueObjectAdd(props,
> + "b:prealloc", true,
Assuming the qemu default would be if not present, then false... If you
went with "B:prealloc", prealloc, then virJSONValueObjectAddVArgs only
adds "prealloc" if prealloc is true, thus the code here could be combined...
> + NULL) < 0)
> + goto cleanup;
> +
> + if (virJSONValueObjectAdd(props,
> + "s:mem-path", memPathActual,
> + NULL) < 0)
> + goto cleanup;
> +
> switch (memAccess) {
> case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
> if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
> @@ -3281,7 +3295,8 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> }
>
> /* If none of the following is requested... */
> - if (!needHugepage && !userNodeset && !memAccess && !nodeSpecified && !force) {
> + if (!needHugepage && !userNodeset &&
> + !memAccess && !nodeSpecified && !force && !memPathReq) {
This is the kind of "logic" that could be replaced if 'force' (or some
more suitably named replacement) was made to be an output as well...
> /* report back that using the new backend is not necessary
> * to achieve the desired configuration */
> ret = 1;
> @@ -3309,8 +3324,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>
> cleanup:
> virJSONValueFree(props);
> - VIR_FREE(mem_path);
> -
> + VIR_FREE(memPathActual);
> return ret;
> }
>
> @@ -3338,7 +3352,7 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
>
> if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps,
> def, cell, memsize, 0, NULL,
> - auto_nodeset, false)) < 0)
> + auto_nodeset, NULL, false)) < 0)
> goto cleanup;
>
> if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType,
> @@ -3379,7 +3393,8 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
>
> if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps, def,
> mem->targetNode, mem->size, mem->pagesize,
> - mem->sourceNodes, auto_nodeset, true) < 0)
> + mem->sourceNodes, auto_nodeset, mem->path,
> + true) < 0)
> goto cleanup;
>
> ret = virQEMUBuildObjectCommandlineFromJSON(backendType, alias, props);
> @@ -3396,6 +3411,7 @@ char *
> qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> + const char *device;
>
> if (!mem->info.alias) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3404,8 +3420,15 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
> }
>
> switch ((virDomainMemoryModel) mem->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> - virBufferAddLit(&buf, "pc-dimm,");
> +
> + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM)
> + device = "pc-dimm";
> + else
> + device = "nvdimm";
> +
> + virBufferAsprintf(&buf, "%s,", device);
>
> if (mem->targetNode >= 0)
> virBufferAsprintf(&buf, "node=%d,", mem->targetNode);
> @@ -3421,12 +3444,6 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
>
> break;
>
> - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> - virReportError(VIR_ERR_NO_SUPPORT, "%s",
> - _("nvdimm not supported yet"));
> - return NULL;
> - break;
> -
> case VIR_DOMAIN_MEMORY_MODEL_NONE:
> case VIR_DOMAIN_MEMORY_MODEL_LAST:
> break;
> @@ -6976,6 +6993,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> bool obsoleteAccel = false;
> + size_t i;
> int ret = -1;
>
> /* This should *never* be NULL, since we always provide
> @@ -7012,6 +7030,15 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> "with this QEMU binary"));
> return -1;
> }
> +
> + for (i = 0; i < def->nmems; i++) {
> + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("nvdimm not is not available "
> + "with this QEMU binary"));
> + return -1;
> + }
> + }
> } else {
> virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
> virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
> @@ -7132,6 +7159,18 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> }
> }
>
> + for (i = 0; i < def->nmems; i++) {
> + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("nvdimm isn't supported by this QEMU binary"));
> + goto cleanup;
> + }
> + virBufferAddLit(&buf, ",nvdimm=on");
> + break;
> + }
> + }
> +
> virCommandAddArgBuffer(cmd, &buf);
> }
>
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index efdad77f3..e23930255 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -134,6 +134,7 @@ int qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> unsigned long long pagesize,
> virBitmapPtr userNodeset,
> virBitmapPtr autoNodeset,
> + const char *memPath,
> bool force);
>
> char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5ec610564..f585e6f25 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5878,6 +5878,7 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
> {
> switch ((virDomainMemoryModel) mem->model) {
> case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
> mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -5910,11 +5911,6 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
> }
> break;
>
> - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("nvdimm hotplug not supported yet"));
> - return -1;
> -
> case VIR_DOMAIN_MEMORY_MODEL_NONE:
> case VIR_DOMAIN_MEMORY_MODEL_LAST:
> return -1;
> @@ -5968,12 +5964,6 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> return 0;
> }
>
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("memory hotplug isn't supported by this QEMU binary"));
> - return -1;
> - }
> -
> if (!ARCH_IS_PPC64(def->os.arch)) {
> /* due to guest support, qemu would silently enable NUMA with one node
> * once the memory hotplug backend is enabled. To avoid possible
> @@ -5997,6 +5987,28 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> for (i = 0; i < def->nmems; i++) {
> hotplugMemory += def->mems[i]->size;
>
> + switch ((virDomainMemoryModel) def->mems[i]->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("memory hotplug isn't supported by this QEMU binary"));
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("nvdimm isn't supported by this QEMU binary"));
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
> +
This could be inefficient if there are "many" [NV]DIMM's as it's making
a check for each defined rather than some signifying we already checked
one type (and the other obviously).
Beyond that - this is ACK'able with a couple of adjustments.
John
> /* already existing devices don't need to be checked on hotplug */
> if (!mem &&
> qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 0)
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c7b8074d6..51b87804d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2218,7 +2218,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>
> if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, priv->qemuCaps,
> vm->def, mem->targetNode, mem->size,
> - mem->pagesize, mem->sourceNodes, NULL, true) < 0)
> + mem->pagesize, mem->sourceNodes, NULL,
> + mem->path, true) < 0)
> goto cleanup;
>
> if (virDomainMemoryInsert(vm->def, mem) < 0) {
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args
> new file mode 100644
> index 000000000..907bcbeda
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args
> @@ -0,0 +1,26 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc,accel=tcg,nvdimm=on \
> +-m size=1048576k,slots=16,maxmem=1099511627776k \
> +-smp 2,sockets=2,cores=1,threads=1 \
> +-numa node,nodeid=0,cpus=0-1,mem=1024 \
> +-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
> +size=536870912 \
> +-device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0,slot=0 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index ad9ce8e2d..05a51a389 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2325,7 +2325,7 @@ mymain(void)
>
> DO_TEST_FAILURE("memory-align-fail", NONE);
> DO_TEST_FAILURE("memory-hotplug-nonuma", QEMU_CAPS_DEVICE_PC_DIMM);
> - DO_TEST_FAILURE("memory-hotplug", NONE);
> + DO_TEST("memory-hotplug", NONE);
> DO_TEST("memory-hotplug", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA);
> DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
> QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> @@ -2333,6 +2333,8 @@ mymain(void)
> QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> DO_TEST("memory-hotplug-ppc64-nonuma", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
> QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> + DO_TEST("memory-hotplug-nvdimm", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DEVICE_NVDIMM,
> + QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
>
> DO_TEST("machine-aeskeywrap-on-caps",
> QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_AES_KEY_WRAP,
>
More information about the libvir-list
mailing list