This is an input argument. We should not overwrite it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6545a9325..f145cf9d8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3203,7 +3203,6 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) { /* we can have both pagesize and mem source, then check mem source first */ - force = true; if (virJSONValueObjectAdd(props, "s:mem-path", cfg->memoryBackingDir, NULL) < 0) @@ -3273,7 +3272,8 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* If none of the following is requested... */ - if (!needHugepage && !userNodeset && !memAccess && !nodeSpecified && !force) { + if (!needHugepage && !userNodeset && !memAccess && !nodeSpecified && + def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && !force) { /* report back that using the new backend is not necessary * to achieve the desired configuration */ ret = 1; -- 2.11.0

Even though this variable contains just values from an enum where zero has the usual meaning, it's enum after all and we should check it as such. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f145cf9d8..62d3a0a4e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3272,7 +3272,8 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* If none of the following is requested... */ - if (!needHugepage && !userNodeset && !memAccess && !nodeSpecified && + if (!needHugepage && !userNodeset && !nodeSpecified && + 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 * to achieve the desired configuration */ -- 2.11.0

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 49 ++++++++++++++++++++++--------------------------- src/qemu/qemu_command.h | 5 +---- src/qemu/qemu_hotplug.c | 6 ++---- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 62d3a0a4e..cbe3e3492 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3078,11 +3078,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, /** * qemuBuildMemoryBackendStr: - * @size: size of the memory device in kibibytes - * @pagesize: size of the requested memory page in KiB, 0 for default - * @guestNode: NUMA node in the guest that the memory object will be attached - * to, or -1 if NUMA is not used in the guest - * @hostNodes: map of host nodes to alloc the memory in, NULL for default + * @mem: memory definition object * @autoNodeset: fallback nodeset in case of automatic numa placement * @def: domain definition object * @qemuCaps: qemu capabilities object @@ -3100,10 +3096,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, * -1 in case of an error. */ int -qemuBuildMemoryBackendStr(unsigned long long size, - unsigned long long pagesize, - int guestNode, - virBitmapPtr userNodeset, +qemuBuildMemoryBackendStr(virDomainMemoryDefPtr mem, virBitmapPtr autoNodeset, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -3122,26 +3115,27 @@ qemuBuildMemoryBackendStr(unsigned long long size, virBitmapPtr nodemask = NULL; int ret = -1; virJSONValuePtr props = NULL; - bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, guestNode); + bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, mem->targetNode); + unsigned long long pagesize = mem->pagesize; bool needHugepage = !!pagesize; *backendProps = NULL; *backendType = NULL; - if (guestNode >= 0) { + if (mem->targetNode >= 0) { /* memory devices could provide a invalid guest node */ - if (guestNode >= virDomainNumaGetNodeCount(def->numa)) { + if (mem->targetNode >= virDomainNumaGetNodeCount(def->numa)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("can't add memory backend for guest node '%d' as " "the guest has only '%zu' NUMA nodes configured"), - guestNode, virDomainNumaGetNodeCount(def->numa)); + mem->targetNode, virDomainNumaGetNodeCount(def->numa)); return -1; } - memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode); + memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, mem->targetNode); } - if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 && + if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 && virDomainNumatuneGetMode(def->numa, -1, &mode) < 0) mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; @@ -3158,10 +3152,10 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* just find the master hugepage in case we don't use NUMA */ - if (guestNode < 0) + if (mem->targetNode < 0) continue; - if (virBitmapGetBit(hugepage->nodemask, guestNode, + if (virBitmapGetBit(hugepage->nodemask, mem->targetNode, &thisHugepage) < 0) { /* Ignore this error. It's not an error after all. Well, * the nodemask for this <page/> can contain lower NUMA @@ -3250,14 +3244,14 @@ qemuBuildMemoryBackendStr(unsigned long long size, *backendType = "memory-backend-ram"; } - if (virJSONValueObjectAdd(props, "U:size", size * 1024, NULL) < 0) + if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0) goto cleanup; - if (userNodeset) { - nodemask = userNodeset; + if (mem->sourceNodes) { + nodemask = mem->sourceNodes; } else { if (virDomainNumatuneMaybeGetNodeset(def->numa, autoNodeset, - &nodemask, guestNode) < 0) + &nodemask, mem->targetNode) < 0) goto cleanup; } @@ -3272,7 +3266,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, } /* If none of the following is requested... */ - if (!needHugepage && !userNodeset && !nodeSpecified && + if (!needHugepage && !mem->sourceNodes && !nodeSpecified && 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 @@ -3321,17 +3315,19 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, const char *backendType; int ret = -1; int rc; + virDomainMemoryDef mem = { 0 }; unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa, cell); *backendStr = NULL; + mem.size = memsize; + mem.targetNode = cell; if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; - if ((rc = qemuBuildMemoryBackendStr(memsize, 0, cell, NULL, auto_nodeset, - def, qemuCaps, cfg, &backendType, - &props, false)) < 0) + if ((rc = qemuBuildMemoryBackendStr(&mem, auto_nodeset, def, qemuCaps, + cfg, &backendType, &props, false)) < 0) goto cleanup; if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType, @@ -3370,8 +3366,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, if (virAsprintf(&alias, "mem%s", mem->info.alias) < 0) goto cleanup; - if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, - mem->targetNode, mem->sourceNodes, auto_nodeset, + if (qemuBuildMemoryBackendStr(mem, auto_nodeset, def, qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 69fe84613..9b2b81f55 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -124,10 +124,7 @@ char *qemuBuildControllerDevStr(const virDomainDef *domainDef, virQEMUCapsPtr qemuCaps, int *nusbcontroller); -int qemuBuildMemoryBackendStr(unsigned long long size, - unsigned long long pagesize, - int guestNode, - virBitmapPtr userNodeset, +int qemuBuildMemoryBackendStr(virDomainMemoryDefPtr mem, virBitmapPtr autoNodeset, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0e4af830f..39710db19 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2240,10 +2240,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (!(devstr = qemuBuildMemoryDeviceStr(mem))) goto cleanup; - if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, - mem->targetNode, mem->sourceNodes, NULL, - vm->def, priv->qemuCaps, cfg, - &backendType, &props, true) < 0) + if (qemuBuildMemoryBackendStr(mem, NULL, vm->def, priv->qemuCaps, + cfg, &backendType, &props, true) < 0) goto cleanup; if (virDomainMemoryInsert(vm->def, mem) < 0) { -- 2.11.0

Frankly, this function is one big mess. A lot of arguments, complicated behaviour. It's really surprising that arguments were in random order (input and output arguments were mixed together), the documentation was outdated, the description of return values was bogus. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 54 +++++++++++++++++++++++++++---------------------- src/qemu/qemu_command.h | 12 +++++------ src/qemu/qemu_hotplug.c | 4 ++-- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cbe3e3492..0cca99567 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3078,31 +3078,38 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, /** * qemuBuildMemoryBackendStr: - * @mem: memory definition object - * @autoNodeset: fallback nodeset in case of automatic numa placement - * @def: domain definition object - * @qemuCaps: qemu capabilities object + * @backendProps: [out] constructed object + * @backendType: [out] type of the backennd used * @cfg: qemu driver config object - * @aliasPrefix: prefix string of the alias (to allow for multiple frontents) - * @id: index of the device (to construct the alias) - * @backendStr: returns the object string + * @qemuCaps: qemu capabilities object + * @def: domain definition object + * @mem: memory definition object + * @autoNodeset: fallback nodeset in case of automatic NUMA placement + * @force: forcibly use one of the backends * - * Formats the configuration string for the memory device backend according - * to the configuration. @pagesize and @hostNodes can be used to override the - * default source configuration, both are optional. + * Creates a configuration object that represents memory backend of given guest + * NUMA node (domain @def and @mem). Use @autoNodeset to fine tune the + * placement of the memory on the host NUMA nodes. * - * Returns 0 on success, 1 if only the implicit memory-device-ram with no - * other configuration was used (to detect legacy configurations). Returns - * -1 in case of an error. + * By default, if no memory-backend-* object is necessary to fulfil the guest + * configuration value of 1 is returned. This behaviour can be suppressed by + * setting @force to true in which case 0 would be returned. + * + * Then, if one of the two memory-backend-* should be used, the @qemuCaps is + * consulted to check if qemu does support it. + * + * Returns: 0 on success, + * 1 on success and if there's no need to use memory-backend-* + * -1 on error. */ int -qemuBuildMemoryBackendStr(virDomainMemoryDefPtr mem, - virBitmapPtr autoNodeset, - virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virQEMUDriverConfigPtr cfg, +qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps, const char **backendType, - virJSONValuePtr *backendProps, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps, + virDomainDefPtr def, + virDomainMemoryDefPtr mem, + virBitmapPtr autoNodeset, bool force) { virDomainHugePagePtr master_hugepage = NULL; @@ -3326,8 +3333,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; - if ((rc = qemuBuildMemoryBackendStr(&mem, auto_nodeset, def, qemuCaps, - cfg, &backendType, &props, false)) < 0) + if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps, + def, &mem, auto_nodeset, false)) < 0) goto cleanup; if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType, @@ -3366,9 +3373,8 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, if (virAsprintf(&alias, "mem%s", mem->info.alias) < 0) goto cleanup; - if (qemuBuildMemoryBackendStr(mem, auto_nodeset, - def, qemuCaps, cfg, - &backendType, &props, true) < 0) + if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps, + def, mem, auto_nodeset, true) < 0) goto cleanup; ret = virQEMUBuildObjectCommandlineFromJSON(backendType, alias, props); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 9b2b81f55..f3ed9e7e4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -124,13 +124,13 @@ char *qemuBuildControllerDevStr(const virDomainDef *domainDef, virQEMUCapsPtr qemuCaps, int *nusbcontroller); -int qemuBuildMemoryBackendStr(virDomainMemoryDefPtr mem, - virBitmapPtr autoNodeset, - virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virQEMUDriverConfigPtr cfg, +int qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps, const char **backendType, - virJSONValuePtr *backendProps, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps, + virDomainDefPtr def, + virDomainMemoryDefPtr mem, + virBitmapPtr autoNodeset, bool force); char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 39710db19..4e416b12e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2240,8 +2240,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (!(devstr = qemuBuildMemoryDeviceStr(mem))) goto cleanup; - if (qemuBuildMemoryBackendStr(mem, NULL, vm->def, priv->qemuCaps, - cfg, &backendType, &props, true) < 0) + if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, + priv->qemuCaps, vm->def, mem, NULL, true) < 0) goto cleanup; if (virDomainMemoryInsert(vm->def, mem) < 0) { -- 2.11.0

NVDIMM is new type of memory introduced into QEMU 2.6. The idea is that we have a Non-Volatile memory module that keeps the data persistent across domain reboots. At the domain XML level, we already have some representation of 'dimm' modules. Long story short, we have <memory/> element that lives under <devices/>. Now, the element even has @model attribute which we can use to introduce new memory type: <memory model='nvdimm'> <source> <path>/tmp/nvdimm</path> </source> <target> <size unit='KiB'>523264</size> <node>0</node> </target> <address type='dimm' slot='0'/> </memory> So far, this is just a XML parser/formatter extension. QEMU driver implementation is in the next commit. For more info on NVDIMM visit the following web page: http://pmem.io/ Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- docs/formatdomain.html.in | 56 +++++++++---- docs/schemas/domaincommon.rng | 32 ++++--- src/conf/domain_conf.c | 97 ++++++++++++++++------ src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 6 ++ src/qemu/qemu_domain.c | 5 ++ .../qemuxml2argv-memory-hotplug-nvdimm.xml | 56 +++++++++++++ .../qemuxml2xmlout-memory-hotplug-nvdimm.xml | 1 + tests/qemuxml2xmltest.c | 1 + 9 files changed, 205 insertions(+), 51 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3095111c4..6e89bfe3a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7091,7 +7091,6 @@ qemu-kvm -net nic,model=? /dev/null guests' memory resource needs. Some hypervisors may require NUMA configured for the guest. - <span class="since">Since 1.2.14</span> </p> <p> @@ -7116,6 +7115,15 @@ qemu-kvm -net nic,model=? /dev/null &lt;node&gt;1&lt;/node&gt; &lt;/target&gt; &lt;/memory&gt; + &lt;memory model='nvdimm'&gt; + &lt;source&gt; + &lt;path&gt;/tmp/nvdimm&lt;/path&gt; + &lt;/source&gt; + &lt;target&gt; + &lt;size unit='KiB'&gt;524288&lt;/size&gt; + &lt;node&gt;1&lt;/node&gt; + &lt;/target&gt; + &lt;/memory&gt; &lt;/devices&gt; ... </pre> @@ -7123,28 +7131,48 @@ qemu-kvm -net nic,model=? /dev/null <dt><code>model</code></dt> <dd> <p> - Currently only the <code>dimm</code> model is supported in order to - add a virtual DIMM module to the guest. + Provide <code>dimm</code> to add a virtual DIMM module to the guest. + <span class="since">Since 1.2.14</span> + Provide <code>nvdimm</code> model adds a Non-Volatile DIMM + module. <span class="since">Since 3.2.0</span> </p> </dd> <dt><code>source</code></dt> <dd> <p> - The optional source element allows to fine tune the source of the - memory used for the given memory device. If the element is not - provided defaults configured via <code>numatune</code> are used. + For model <code>dimm</code> this element is optional and allows to + fine tune the source of the memory used for the given memory device. + If the element is not provided defaults configured via + <code>numatune</code> are used. If <code>dimm</code> is provided, + then the following optional elements can be provided as well: </p> - <p> - <code>pagesize</code> can optionally be used to override the default - host page size used for backing the memory device. - The configured value must correspond to a page size supported by the - host. - </p> + <dl> + <dt><code>pagesize</code></dt> + <dd> + <p> + This element can be used to override the default + host page size used for backing the memory device. + The configured value must correspond to a page size + supported by the host. + </p> + </dd> + + <dt><code>nodemask</code></dt> + <dd> + <p> + This element can be used to override the default + set of NUMA nodes where the memory would be + allocated. + </p> + </dd> + </dl> + <p> - <code>nodemask</code> can optionally be used to override the default - set of NUMA nodes where the memory would be allocated. + For model <code>nvdimm</code> this element is mandatory and has a + single child element <code>path</code> that represents a path + in the host that backs the nvdimm module in the guest. </p> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1860f9075..07b3c52ad 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4746,6 +4746,7 @@ <attribute name="model"> <choice> <value>dimm</value> + <value>nvdimm</value> </choice> </attribute> <interleave> @@ -4765,18 +4766,27 @@ <define name="memorydev-source"> <element name="source"> - <interleave> - <optional> - <element name="pagesize"> - <ref name="scaledInteger"/> + <choice> + <group> + <interleave> + <optional> + <element name="pagesize"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="nodemask"> + <ref name="cpuset"/> + </element> + </optional> + </interleave> + </group> + <group> + <element name="path"> + <ref name="absFilePath"/> </element> - </optional> - <optional> - <element name="nodemask"> - <ref name="cpuset"/> - </element> - </optional> - </interleave> + </group> + </choice> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a58f99762..e23ce0015 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -858,8 +858,11 @@ VIR_ENUM_DECL(virDomainBlockJob) VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, "", "", "copy", "", "active-commit") -VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, - "", "dimm") +VIR_ENUM_IMPL(virDomainMemoryModel, + VIR_DOMAIN_MEMORY_MODEL_LAST, + "", + "dimm", + "nvdimm") VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, "ivshmem", @@ -2418,6 +2421,7 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def) if (!def) return; + VIR_FREE(def->nvdimmPath); virBitmapFree(def->sourceNodes); virDomainDeviceInfoClear(&def->info); VIR_FREE(def); @@ -13769,20 +13773,36 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, xmlNodePtr save = ctxt->node; ctxt->node = node; - if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, - &def->pagesize, false, false) < 0) - goto cleanup; - - if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { - if (virBitmapParse(nodemask, &def->sourceNodes, - VIR_DOMAIN_CPUMASK_LEN) < 0) + switch ((virDomainMemoryModel) def->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, + &def->pagesize, false, false) < 0) goto cleanup; - if (virBitmapIsAllClear(def->sourceNodes)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid value of 'nodemask': %s"), nodemask); + if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { + if (virBitmapParse(nodemask, &def->sourceNodes, + VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (virBitmapIsAllClear(def->sourceNodes)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodemask': %s"), nodemask); + goto cleanup; + } + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (!(def->nvdimmPath = virXPathString("string(./path)", ctxt))) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("path is required for model nvdimm'")); goto cleanup; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } ret = 0; @@ -15187,12 +15207,25 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, tmp->size != mem->size) continue; - /* source stuff -> match with device */ - if (tmp->pagesize != mem->pagesize) - continue; + switch ((virDomainMemoryModel) mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + /* source stuff -> match with device */ + if (tmp->pagesize != mem->pagesize) + continue; - if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes)) - continue; + if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes)) + continue; + break; + + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (STRNEQ(tmp->nvdimmPath, mem->nvdimmPath)) + continue; + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } break; } @@ -22585,23 +22618,35 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, char *bitmap = NULL; int ret = -1; - if (!def->pagesize && !def->sourceNodes) + if (!def->pagesize && !def->sourceNodes && !def->nvdimmPath) return 0; virBufferAddLit(buf, "<source>\n"); virBufferAdjustIndent(buf, 2); - if (def->sourceNodes) { - if (!(bitmap = virBitmapFormat(def->sourceNodes))) - goto cleanup; + switch ((virDomainMemoryModel) def->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + if (def->sourceNodes) { + if (!(bitmap = virBitmapFormat(def->sourceNodes))) + goto cleanup; - virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap); + virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap); + } + + if (def->pagesize) + virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n", + def->pagesize); + break; + + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath); + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } - if (def->pagesize) - virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n", - def->pagesize); - virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e53cc328..823582253 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1996,6 +1996,7 @@ struct _virDomainRNGDef { typedef enum { VIR_DOMAIN_MEMORY_MODEL_NONE, VIR_DOMAIN_MEMORY_MODEL_DIMM, /* dimm hotpluggable memory device */ + VIR_DOMAIN_MEMORY_MODEL_NVDIMM, /* nvdimm memory device */ VIR_DOMAIN_MEMORY_MODEL_LAST } virDomainMemoryModel; @@ -2004,6 +2005,7 @@ struct _virDomainMemoryDef { /* source */ virBitmapPtr sourceNodes; unsigned long long pagesize; /* kibibytes */ + char *nvdimmPath; /* target */ int model; /* virDomainMemoryModel */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0cca99567..07178f839 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3416,6 +3416,12 @@ 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; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1a42fcf1b..66c0e5911 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5951,6 +5951,11 @@ 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; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml new file mode 100644 index 000000000..1578db453 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml @@ -0,0 +1,56 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>1267710</memory> + <currentMemory unit='KiB'>1267710</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <idmap> + <uid start='0' target='1000' count='10'/> + <gid start='0' target='1000' count='10'/> + </idmap> + <cpu> + <topology sockets='2' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + <memory model='nvdimm'> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit='KiB'>523264</size> + <node>0</node> + </target> + <address type='dimm' slot='0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml new file mode 120000 index 000000000..4cac477a9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4353ad245..e1c341dd5 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1078,6 +1078,7 @@ mymain(void) DO_TEST("memory-hotplug", NONE); DO_TEST("memory-hotplug-nonuma", NONE); DO_TEST("memory-hotplug-dimm", NONE); + DO_TEST("memory-hotplug-nvdimm", NONE); DO_TEST("net-udp", NONE); DO_TEST("video-virtio-gpu-device", NONE); -- 2.11.0

Introduce a qemu capability for -device nvdimm. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 9 +++++++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5a3b4ac50..2f459d128 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -359,6 +359,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-cpu-model-expansion", /* 245 */ "virtio-net.host_mtu", "spice-rendernode", + "nvdimm", ); @@ -1626,6 +1627,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "ivshmem-plain", QEMU_CAPS_DEVICE_IVSHMEM_PLAIN }, { "ivshmem-doorbell", QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL }, { "vhost-scsi", QEMU_CAPS_DEVICE_VHOST_SCSI }, + { "nvdimm", QEMU_CAPS_DEVICE_NVDIMM }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { @@ -4545,6 +4547,13 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, virQEMUCapsProbeQMPGICCapabilities(qemuCaps, mon) < 0) goto cleanup; + /* Prealloc on NVDIMMs is broken on older QEMUs leading to + * user data corruption. If we are dealing with such version + * of QEMU pretend we don't know how to NVDIMM. */ + if (qemuCaps->version < 2009000 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) + virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM); + ret = 0; cleanup: return ret; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index cc9f46e65..1b4bcfb2f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -394,6 +394,7 @@ typedef enum { QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */ QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */ QEMU_CAPS_SPICE_RENDERNODE, /* -spice rendernode */ + QEMU_CAPS_DEVICE_NVDIMM, /* -device nvdimm */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.11.0

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

On 03/14/2017 02:36 PM, John Ferlan wrote: It is not needed for JSON build. 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

On 03/09/2017 11:06 AM, Michal Privoznik wrote: This patch fails make check for me because your qemuxml2xmloutdata has the wrong softlink set up. I don't have "/home/zippy/work/...", but I would have e.g. from the tests/qemuxml2xmloutdata directory use: ln -sf ../qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-access.xml qemuxml2xmlout-memory-hotplug-nvdimm-access.xml here that the access setting in/for nvdimm would override any memoryBacking setting?' If so, then would it be furthermore interesting to modify the XML here to set the memoryBacking as private and override that, but it's not required... ACK w/ the fixed link to the output file... Your call on the doc change John

On 03/09/2017 11:06 AM, Michal Privoznik wrote: Not that it matters in the grand scheme of things, but @access was split across 2 patches, while @label-size is all in one. The tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm-label.xml has the same problem as patch 8 as make check fails for me... Required a similar adjustment from qemuxml2xmloutdata directory: ln -sf ../qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-label.xml qemuxml2xmlout-memory-hotplug-nvdimm-label.xml FWIW: In re-reading my v2 review - not quite sure what was tripping me up about label-size. Perhaps when I saw that comparison to 128 I started overthinking things and missed the "if (def->labelsize..." Should there be a summary of what was written in: https://www.redhat.com/archives/libvir-list/2016-August/msg00650.html e.g. By default no Namespace Label area is reserved in the file. If the user specifies label-size then the memory at the end of the file is used as the Namespace Label area. For QEMU, there are two restrictions: 1. Minimum label size is 128K 2. Remaining size (total - labelsize) is aligned with 4k for nomral backend or hugepage-size for hugetblfs cannot be 0. (Or reworded sufficiently ;-)... Would be "nice" not lose that and something someone may trip across... ACK w/ the link fixed - bonus points for documentation adjustments. Better to do it now while it's still fresh in your mind rather than months down the road when someone asks and you're in the middle of something else! John [...] !!

On 03/09/2017 11:06 AM, Michal Privoznik wrote: Since it doesn't matter for DIMM, should this go in the NVDIMM label? Although I do see this follow a couple of the other Set*Label functions when there's only one of the switch case statements that uses the seclabel. I guess for consistency it can stay as is, although I wouldn't object to altering code for those single switch/case conditions Also I note that the security_dac code looks at the "->relabel" when making a decision, but that's not done here. Ironically you did change this one to be different... Similar comment regarding the relabel Conditional ACK of course depending on the relabel thing - you could explain or just provide something that you'll squash in.... John

On 03/09/2017 11:06 AM, Michal Privoznik wrote: @props will be leaked (found by Coverity) If the code would follow other models, then virJSONValueFree(props) would be in the cleanup label. Since props = NULL is done after the AddObject that should be safe. Of course that means a couple of existing virJSONValueFree(props) would be removed and just goto cleanup left. Whether those are done inline here or as a patch stuffed in before this one - doesn't matter to me... Given this requirement for 'mem' to still be valid one wonders if rather than goto cleanup, maybe the goto should be some label below removedef and above goto audit... Furthermore whether that should also be wrapped in a virSaveLastError... restoredevice: if (!mem) { VIR_WARN("Unable to restore host device la goto audit; } orig_err = virSaveLastError(); if (teardownlabel && qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0) VIR_WARN("Unable to restore security label on memdev"); virSetError(orig_err); virFreeError(orig_err); I think cgroup and namespace could use the same label - whether or not you have a VIR_WARN for when if (!mem) is true is your call. Using the label leaves out the ugliness of (mem && ret < 0) from the cleanup path. ACK w/ adjustments... John

Some users might want to pass a blockdev or a chardev as a backend for NVDIMM. In fact, this is expected to be the mostly used configuration. Therefore libvirt should allow the device in devices CGroup then. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_cgroup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_cgroup.h | 4 ++++ src/qemu/qemu_hotplug.c | 10 ++++++++++ 3 files changed, 63 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 42a47a798..36762d4f6 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -348,6 +348,50 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, } +int +qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm, + virDomainMemoryDefPtr mem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + return 0; + + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + VIR_DEBUG("Setting devices Cgroup for NVDIMM device: %s", mem->nvdimmPath); + rv = virCgroupAllowDevicePath(priv->cgroup, mem->nvdimmPath, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", + mem->nvdimmPath, "rw", rv == 0); + + return rv; +} + + +int +qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm, + virDomainMemoryDefPtr mem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + return 0; + + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + rv = virCgroupDenyDevicePath(priv->cgroup, mem->nvdimmPath, + VIR_CGROUP_DEVICE_RWM, false); + virDomainAuditCgroupPath(vm, priv->cgroup, + "deny", mem->nvdimmPath, "rwm", rv == 0); + return rv; +} + + static int qemuSetupGraphicsCgroup(virDomainObjPtr vm, virDomainGraphicsDefPtr gfx) @@ -647,6 +691,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } + for (i = 0; i < vm->def->nmems; i++) { + if (qemuSetupMemoryDevicesCgroup(vm, vm->def->mems[i]) < 0) + goto cleanup; + } + for (i = 0; i < vm->def->ngraphics; i++) { if (qemuSetupGraphicsCgroup(vm, vm->def->graphics[i]) < 0) goto cleanup; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 8ae4a72ab..d016ce29d 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -43,6 +43,10 @@ int qemuSetupHostdevCgroup(virDomainObjPtr vm, int qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm, + virDomainMemoryDefPtr mem); +int qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm, + virDomainMemoryDefPtr mem); int qemuSetupRNGCgroup(virDomainObjPtr vm, virDomainRNGDefPtr rng); int qemuTeardownRNGCgroup(virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7e19d2f82..829b40258 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2216,6 +2216,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, const char *backendType; bool objAdded = false; bool teardownlabel = false; + bool teardowncgroup = false; virJSONValuePtr props = NULL; virObjectEventPtr event; int id; @@ -2245,6 +2246,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, priv->qemuCaps, vm->def, mem, NULL, true) < 0) goto cleanup; + if (qemuSetupMemoryDevicesCgroup(vm, mem) < 0) + goto cleanup; + teardowncgroup = true; + if (qemuSecuritySetMemoryLabel(driver, vm, mem) < 0) goto cleanup; teardownlabel = true; @@ -2294,6 +2299,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0); cleanup: if (mem && ret < 0) { + if (teardowncgroup && qemuTeardownMemoryDevicesCgroup(vm, mem) < 0) + VIR_WARN("Unable to remove memory device cgroup ACL on hotplug fail"); if (teardownlabel && qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0) VIR_WARN("Unable to restore security label on memdev"); } @@ -3761,6 +3768,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, if (qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0) VIR_WARN("Unable to restore security label on memdev"); + if (qemuTeardownMemoryDevicesCgroup(vm, mem) < 0) + VIR_WARN("Unable to remove memory device cgroup ACL"); + virDomainMemoryDefFree(mem); /* fix the balloon size */ -- 2.11.0

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 8 ++++++ src/qemu/qemu_hotplug.c | 11 +++++++ 3 files changed, 95 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 495242981..552523617 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7526,6 +7526,37 @@ qemuDomainSetupAllHostdevs(virQEMUDriverConfigPtr cfg, } +static int +qemuDomainSetupMemory(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, + virDomainMemoryDefPtr mem, + const char *devPath) +{ + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + return 0; + + return qemuDomainCreateDevice(mem->nvdimmPath, devPath, false); +} + + +static int +qemuDomainSetupAllMemories(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + const char *devPath) +{ + size_t i; + + VIR_DEBUG("Setting up memories"); + for (i = 0; i < vm->def->nmems; i++) { + if (qemuDomainSetupMemory(cfg, + vm->def->mems[i], + devPath) < 0) + return -1; + } + VIR_DEBUG("Setup all memories"); + return 0; +} + + static int qemuDomainSetupChardev(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainChrDefPtr dev, @@ -7770,6 +7801,9 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, if (qemuDomainSetupAllHostdevs(cfg, vm, devPath) < 0) goto cleanup; + if (qemuDomainSetupAllMemories(cfg, vm, devPath) < 0) + goto cleanup; + if (qemuDomainSetupAllChardevs(cfg, vm, devPath) < 0) goto cleanup; @@ -8228,6 +8262,48 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver, } +int +qemuDomainNamespaceSetupMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr mem) +{ + int ret = -1; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + return 0; + + if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + return 0; + + if (qemuDomainAttachDeviceMknod(driver, vm, mem->nvdimmPath) < 0) + goto cleanup; + ret = 0; + cleanup: + return ret; +} + + +int +qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr mem) +{ + int ret = -1; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + return 0; + + if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + return 0; + + if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath) < 0) + goto cleanup; + ret = 0; + cleanup: + return ret; +} + + int qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7fa717390..f5b8d3fe7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -834,6 +834,14 @@ int qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev); +int qemuDomainNamespaceSetupMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr memory); + +int qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr memory); + int qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 829b40258..392b2fd02 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2217,6 +2217,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, bool objAdded = false; bool teardownlabel = false; bool teardowncgroup = false; + bool teardowndevice = false; virJSONValuePtr props = NULL; virObjectEventPtr event; int id; @@ -2246,6 +2247,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, priv->qemuCaps, vm->def, mem, NULL, true) < 0) goto cleanup; + if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0) + goto removedef; + teardowndevice = true; + if (qemuSetupMemoryDevicesCgroup(vm, mem) < 0) goto cleanup; teardowncgroup = true; @@ -2303,6 +2308,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, VIR_WARN("Unable to remove memory device cgroup ACL on hotplug fail"); if (teardownlabel && qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0) VIR_WARN("Unable to restore security label on memdev"); + if (teardowndevice && + qemuDomainNamespaceTeardownMemory(driver, vm, mem) < 0) + VIR_WARN("Unable to remove memory device from /dev"); } virObjectUnref(cfg); @@ -3771,6 +3779,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, if (qemuTeardownMemoryDevicesCgroup(vm, mem) < 0) VIR_WARN("Unable to remove memory device cgroup ACL"); + if (qemuDomainNamespaceTeardownMemory(driver, vm, mem) < 0) + VIR_WARN("Unable to remove memory device from /dev"); + virDomainMemoryDefFree(mem); /* fix the balloon size */ -- 2.11.0