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@redhat.com>
---
src/qemu/qemu_alias.c | 10 ++-
src/qemu/qemu_command.c | 76 +++++++++++++++-------
src/qemu/qemu_domain.c | 42 ++++++++----
.../qemuxml2argv-memory-hotplug-nvdimm.args | 26 ++++++++
tests/qemuxml2argvtest.c | 4 +-
5 files changed, 121 insertions(+), 37 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 07178f839..a66ce6645 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3118,7 +3118,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 *memPath = NULL;
+ bool prealloc = false;
virBitmapPtr nodemask = NULL;
int ret = -1;
virJSONValuePtr props = NULL;
@@ -3199,26 +3200,31 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
if (!(props = virJSONValueNewObject()))
return -1;
- if (pagesize || def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
+ if (pagesize || mem->nvdimmPath ||
+ def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
*backendType = "memory-backend-file";
- if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
- /* we can have both pagesize and mem source, then check mem source first */
- if (virJSONValueObjectAdd(props,
- "s:mem-path", cfg->memoryBackingDir,
- NULL) < 0)
+ if (mem->nvdimmPath) {
+ if (VIR_STRDUP(memPath, mem->nvdimmPath) < 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. */
+ if (VIR_STRDUP(memPath, cfg->memoryBackingDir) < 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, &memPath) < 0)
goto cleanup;
+ prealloc = true;
}
+ if (virJSONValueObjectAdd(props,
+ "B:prealloc", prealloc,
+ "s:mem-path", memPath,
+ NULL) < 0)
+ goto cleanup;
+
switch (memAccess) {
case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
@@ -3274,6 +3280,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
/* If none of the following is requested... */
if (!needHugepage && !mem->sourceNodes && !nodeSpecified &&
+ !mem->nvdimmPath &&
memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && !force) {
/* report back that using the new backend is not necessary
@@ -3303,8 +3310,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
cleanup:
virJSONValueFree(props);
- VIR_FREE(mem_path);
-
+ VIR_FREE(memPath);
return ret;
}
@@ -3391,6 +3397,7 @@ char *
qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
+ const char *device;
if (!mem->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3399,8 +3406,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);
@@ -3416,12 +3430,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;
@@ -6972,6 +6980,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
@@ -7008,6 +7017,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];
@@ -7128,6 +7146,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_domain.c b/src/qemu/qemu_domain.c
index 66c0e5911..495242981 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5919,6 +5919,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",
@@ -5951,11 +5952,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;
@@ -5986,6 +5982,8 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
unsigned int nmems = def->nmems;
unsigned long long hotplugSpace;
unsigned long long hotplugMemory = 0;
+ bool needPCDimmCap = false;
+ bool needNvdimmCap = false;
size_t i;
hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def);
@@ -6009,12 +6007,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
@@ -6038,6 +6030,34 @@ 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:
+ needPCDimmCap = true;
+ break;
+
+ case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
+ needNvdimmCap = true;
+ break;
+
+ case VIR_DOMAIN_MEMORY_MODEL_NONE:
+ case VIR_DOMAIN_MEMORY_MODEL_LAST:
+ break;
+ }
+
+ if (needPCDimmCap &&
+ !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 (needNvdimmCap &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("nvdimm isn't supported by this QEMU binary"));
+ return -1;
+ }
+
/* already existing devices don't need to be checked on hotplug */
if (!mem &&
qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 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 d2d267fce..b6fdaaeb3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2329,7 +2329,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);
@@ -2337,6 +2337,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,
--
2.11.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/09/2017 11:06 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@redhat.com>
> ---
> src/qemu/qemu_alias.c | 10 ++-
> src/qemu/qemu_command.c | 76 +++++++++++++++-------
> src/qemu/qemu_domain.c | 42 ++++++++----
> .../qemuxml2argv-memory-hotplug-nvdimm.args | 26 ++++++++
> tests/qemuxml2argvtest.c | 4 +-
> 5 files changed, 121 insertions(+), 37 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args
>
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 07178f839..a66ce6645 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3118,7 +3118,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 *memPath = NULL;
> + bool prealloc = false;
> virBitmapPtr nodemask = NULL;
> int ret = -1;
> virJSONValuePtr props = NULL;
> @@ -3199,26 +3200,31 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> if (!(props = virJSONValueNewObject()))
> return -1;
>
> - if (pagesize || def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> + if (pagesize || mem->nvdimmPath ||
> + def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> *backendType = "memory-backend-file";
>
> - if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
> - /* we can have both pagesize and mem source, then check mem source first */
> - if (virJSONValueObjectAdd(props,
> - "s:mem-path", cfg->memoryBackingDir,
> - NULL) < 0)
> + if (mem->nvdimmPath) {
> + if (VIR_STRDUP(memPath, mem->nvdimmPath) < 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. */
> + if (VIR_STRDUP(memPath, cfg->memoryBackingDir) < 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, &memPath) < 0)
> goto cleanup;
> + prealloc = true;
> }
>
FWIW: In my v2 review I alluded to a double comma thing (e.g. code that
needs virQEMUBuildBufferEscapeComma)... This is what I was thinking
about, but IIRC it's only something for certain command line options and
not JSON object building...
> + if (virJSONValueObjectAdd(props,
> + "B:prealloc", prealloc,
> + "s:mem-path", memPath,
> + NULL) < 0)
> + goto cleanup;
> +
> switch (memAccess) {
> case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
> if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
> @@ -3274,6 +3280,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>
> /* If none of the following is requested... */
> if (!needHugepage && !mem->sourceNodes && !nodeSpecified &&
> + !mem->nvdimmPath &&
> memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
> def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && !force) {
> /* report back that using the new backend is not necessary
> @@ -3303,8 +3310,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>
> cleanup:
> virJSONValueFree(props);
> - VIR_FREE(mem_path);
> -
> + VIR_FREE(memPath);
> return ret;
> }
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 66c0e5911..495242981 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5919,6 +5919,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",
> @@ -5951,11 +5952,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;
> @@ -5986,6 +5982,8 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> unsigned int nmems = def->nmems;
> unsigned long long hotplugSpace;
> unsigned long long hotplugMemory = 0;
> + bool needPCDimmCap = false;
> + bool needNvdimmCap = false;
needNVDimm could be used.... although I see no reason for these bool's
the way the rest is written.
> size_t i;
>
> hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def);
> @@ -6009,12 +6007,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
> @@ -6038,6 +6030,34 @@ 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:
> + needPCDimmCap = true;
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + needNvdimmCap = true;
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
> +
> + if (needPCDimmCap &&
> + !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 (needNvdimmCap &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("nvdimm isn't supported by this QEMU binary"));
> + return -1;
> + }
> +
Still inefficient as virQEMUCapsGet will get called each time through
the for loop as soon as need*Cap = true... Perhaps even moreso since
one or the other will be called both times for a single pass if there
both types of *Dimm defs are in the XML (once one or the other is seen).
I think the bool's should be turned into 'checkedPCDimmCap' and
'checkedNVDimmCap' though and that would be less inefficient.
e.g. within the case:
case VIR_DOMAIN_MEMORY_MODEL_DIMM:
if (!checkedPCDimmCap) {
if (!virQEMUCapsGet())
failure
checkedPCDimmCap = true;
}
> /* already existing devices don't need to be checked on hotplug */
> if (!mem &&
> qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 0)
ACK - I think it would be good to not make a CapsGet on every pass
through though
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/14/2017 02:36 PM, John Ferlan wrote:
>
>
> On 03/09/2017 11:06 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@redhat.com>
>> ---
>> src/qemu/qemu_alias.c | 10 ++-
>> src/qemu/qemu_command.c | 76 +++++++++++++++-------
>> src/qemu/qemu_domain.c | 42 ++++++++----
>> .../qemuxml2argv-memory-hotplug-nvdimm.args | 26 ++++++++
>> tests/qemuxml2argvtest.c | 4 +-
>> 5 files changed, 121 insertions(+), 37 deletions(-)
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args
>>
>
> [...]
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 07178f839..a66ce6645 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3118,7 +3118,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 *memPath = NULL;
>> + bool prealloc = false;
>> virBitmapPtr nodemask = NULL;
>> int ret = -1;
>> virJSONValuePtr props = NULL;
>> @@ -3199,26 +3200,31 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>> if (!(props = virJSONValueNewObject()))
>> return -1;
>>
>> - if (pagesize || def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
>> + if (pagesize || mem->nvdimmPath ||
>> + def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
>> *backendType = "memory-backend-file";
>>
>> - if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
>> - /* we can have both pagesize and mem source, then check mem source first */
>> - if (virJSONValueObjectAdd(props,
>> - "s:mem-path", cfg->memoryBackingDir,
>> - NULL) < 0)
>> + if (mem->nvdimmPath) {
>> + if (VIR_STRDUP(memPath, mem->nvdimmPath) < 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. */
>> + if (VIR_STRDUP(memPath, cfg->memoryBackingDir) < 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, &memPath) < 0)
>> goto cleanup;
>> + prealloc = true;
>> }
>>
>
> FWIW: In my v2 review I alluded to a double comma thing (e.g. code that
> needs virQEMUBuildBufferEscapeComma)... This is what I was thinking
> about, but IIRC it's only something for certain command line options and
> not JSON object building...
It is not needed for JSON build.
>
>
>
>> + if (virJSONValueObjectAdd(props,
>> + "B:prealloc", prealloc,
>> + "s:mem-path", memPath,
>> + NULL) < 0)
>> + goto cleanup;
>> +
>> switch (memAccess) {
>> case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
>> if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
>> @@ -3274,6 +3280,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>>
>> /* If none of the following is requested... */
>> if (!needHugepage && !mem->sourceNodes && !nodeSpecified &&
>> + !mem->nvdimmPath &&
>> memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
>> def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && !force) {
>> /* report back that using the new backend is not necessary
>> @@ -3303,8 +3310,7 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
>>
>> cleanup:
>> virJSONValueFree(props);
>> - VIR_FREE(mem_path);
>> -
>> + VIR_FREE(memPath);
>> return ret;
>> }
>
> [...]
>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 66c0e5911..495242981 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5919,6 +5919,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",
>> @@ -5951,11 +5952,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;
>> @@ -5986,6 +5982,8 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
>> unsigned int nmems = def->nmems;
>> unsigned long long hotplugSpace;
>> unsigned long long hotplugMemory = 0;
>> + bool needPCDimmCap = false;
>> + bool needNvdimmCap = false;
>
> needNVDimm could be used.... although I see no reason for these bool's
> the way the rest is written.
>
>> size_t i;
>>
>> hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def);
>> @@ -6009,12 +6007,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
>> @@ -6038,6 +6030,34 @@ 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:
>> + needPCDimmCap = true;
>> + break;
>> +
>> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
>> + needNvdimmCap = true;
>> + break;
>> +
>> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
>> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
>> + break;
>> + }
>> +
>> + if (needPCDimmCap &&
>> + !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 (needNvdimmCap &&
>> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("nvdimm isn't supported by this QEMU binary"));
>> + return -1;
>> + }
>> +
>
> Still inefficient as virQEMUCapsGet will get called each time through
> the for loop as soon as need*Cap = true... Perhaps even moreso since
> one or the other will be called both times for a single pass if there
> both types of *Dimm defs are in the XML (once one or the other is seen).
Oh, I am a giddy goat. This should have been:
for () {
switch() {
case MODEL_DIM:
needPCDimmCap = true; break;
case MODEL_NVDIMM:
needNvdimmCap = true; break;
}
}
if (needPCDimmCap &&
!virQEMUCapsGet()) error;
if (needNvdimmCap &&
!virQEMUCapsGet()) error;
I am fixing it as such. Thanks.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.