On 01.09.2016 00:51, John Ferlan wrote:

On 08/11/2016 09:26 AM, Michal Privoznik wrote:

...

NVDIMM is new type of memory introduced in qemu. The idea is that

s/in qemu/into QEMU 2.6/

...

we have a DIMM module that keeps the data persistent across

s/DIMM/Non Volatile memory/

...

domain reboots.

Perhaps a word of two about what is the usefulness of such a beast. I think (from a former project) one usage is to store parameters for firmware such as OVMF

Add extra line between paragraphs...

...

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> </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/

One could also google it ;-)... In any case, the actual details are found the Documents link from that page, subject to move over time of course.

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 26 ++++-- 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 | 1 + .../qemuxml2argv-memory-hotplug-nvdimm.xml | 49 +++++++++++ 7 files changed, 171 insertions(+), 42 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bfbb0f2..657df8f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6740,6 +6740,15 @@ qemu-kvm -net nic,model=? /dev/null <node>1</node> </target> </memory> + <memory model='nvdimm'> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit='KiB'>524287</size> + <node>1</node> + </target> + </memory> </devices> ... </pre> @@ -6747,17 +6756,19 @@ 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. + Select <code>dimm</code> to add a virtual DIMM module to the guest.

Ugh... The 'dimm' Since tag is in the "Memory devices" section... Which just make the following awkward. I think you should grab that 1.2.14 from above and place it here.

Rather than Select go with "Use" or "Provide"

...

+ Alternatively, <code>nvdimm</code> model adds a Non-Volatile DIMM

Use the same format - e.g.

"Use/Provide <code>nvdimm</code> to add a Non-Volatile DIMM module."

...

+ module. <span class="since">Since 2.2.0</span>

Well we won't hit 2.2.0....

...

</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. </p> <p> <code>pagesize</code> can optionally be used to override the default @@ -6770,6 +6781,11 @@ qemu-kvm -net nic,model=? /dev/null <code>nodemask</code> can optionally be used to override the default set of NUMA nodes where the memory would be allocated. </p>

Since pagesize and nodemask are for DIMM only, so they probably need to be converted to some sort of list or in some way indented to ensure the visual cue is there that they are meant for dimm. Perhaps the end of the dimm paragraph needs a "If <code>source</code> is provided, then at least one of the following values would be provided:".

I think the only way to get the right formatting look is :

<dd> <dt><code>pagesize</code></dt> <p> ... </p> <dt><code>nodemask</code></dt> <p> ... </p> </dd>

...

+ <p> + For model <code>nvdimm</code> this element is mandatory and has a + single child element <code>path</code> which value represents a path

s/which value/that/ ?

...

+ in host that back the nvdimm module in the guest.

s/in host that back/in the host that backs/

Should there be any mention that this also requires "<maxMemory slots='#'...>" to be set?

So something that isn't quite clear... For 'dimm', these are 'extra' memory, while for 'nvdimm 'it seems to be one hunk that's really not extra memory - rather it's memory that can be used for a specific purpose. What's not clear to me - is the existing "physical" memory in the guest (e.g. the numa node page) is used to map to the file or does this appear as extra memory that is only accessible for this purpose?

Kernel expose this new memory as a block device. As

Also, does the numa node have to be properly sized? Your example shows a 214Mb numa cell and a 511MB nvdimm. Doesn't seem to me that fits quite right.

This should be okay, It's a new memory that is not accounted as RAM (it's a block device that guest sees).

BTW: That video you point to in your v1 indicates two types of NVDIMM - "persistent memory namespace" (accessed using loads/stores, mapped to physical memory) and "block mode namespace" (accessed via block operations, atomicity at block level, indirect mapping thru a block window, no mapping entier memory.

In both cases the device is shown as block device, but the "block mode" is not supported even in qemu. And I doubt it will ever be.

So which is KVM? Since libvirt must be the master to more than one driver - should there be some sort of subtype="block|persistent" as well? Or we could just indicate KVM only for now...

If we are ever gonna implement the block mode, we can introduce an attribute for which "persistent memory mode" is the default.

...

+ </p> </dd>

<dt><code>target</code></dt>

...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82876f3..cb5a053 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c

...

@@ -21705,23 +21738,35 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, char *bitmap = NULL; int ret = -1;

- if (!def->pagesize && !def->sourceNodes) + if (!def->pagesize && !def->sourceNodes && !def->path) 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: + virBufferAsprintf(buf, "<path>%s</path>\n", def->path); + 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");

So do you feel there is a need for a check for different path in virDomainMemoryDefCheckABIStability? I know it's not a target thing, but the target/guest uses it by mapping directly...

From my understanding qemu will copy the content of NVDIMM over to the destination, therefore we are safe with backend changing its path.

Should there be checks for order? That is src has dimm, dimm, dimm, nvdim while dst has nvdimm, dimm, dimm, dimm? Would that matter?

No, the order shouldn't matter. See my next answer for explanation.

After going through patch 3, I'm beginning to think it wouldn't be such a good idea to keep nvdimm's and dimm's in the same nmems list. I think they might just be too different. I also recall some algorithm somewhere which made sure addresses and sizes used fit "properly"... Searching on DIMM in cscope brings up a few more references and questions.

So, I've just discussed this with Peter who implemented the DIMM (aka mem hotplug) feature. In this case, libvirt tries to not duplicate code that already lives in qemu. So how this works is: on domain startup libvirt just puts the dimms (and after my patches nvdimms as well) onto the qemu cmd line just in the order as in the XML. Then, when qemu is started its magic algorithm assigns addresses to all the modules (so that they don't overlap) and libvirt just queries this information on the monitor. And it is only in case of migration when we explicitly put the addresses onto qemu's cmd line (to preserve ABI stability). On the next run of domain (I mean hard stop & start => new qemu process), the process repeats itself again. So addresses can be possibly different, but I'd say this is okay. For two reasons: even in real hardware if you switch two devices (e.g. DIMM modules, disks, ...) you'll boot into different environment (e.g. disks gets renamed, what used to be sda is now sdb). Secondly, this NVDIMM is meant to be used as a sync mechanism between host & guest. Therefore I think DIMMs and NVDIMMs are very similar and can be kept in the same internal list. At least for now. If we ever discover great difference, we can put them into separate lists - after all it's an internal list and we can change it whenever we please.

And well - what about migration?

Michal

On 08/11/2016 09:26 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 | 12 ++- src/qemu/qemu_command.c | 90 ++++++++++++++-------- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 28 +++++-- src/qemu/qemu_hotplug.c | 2 +- .../qemuxml2argv-hugepages-numa.args | 5 +- .../qemuxml2argv-hugepages-pages.args | 24 +++--- .../qemuxml2argv-hugepages-pages2.args | 8 +- .../qemuxml2argv-hugepages-pages3.args | 4 +- .../qemuxml2argv-hugepages-shared.args | 22 +++--- .../qemuxml2argv-memory-hotplug-dimm-addr.args | 5 +- .../qemuxml2argv-memory-hotplug-dimm.args | 5 +- .../qemuxml2argv-memory-hotplug-nvdimm.args | 25 ++++++ tests/qemuxml2argvtest.c | 4 +- 14 files changed, 155 insertions(+), 80 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 0102c96..517dca1 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -339,13 +339,19 @@ 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";

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; }

- if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0) + if (virAsprintf(&mem->info.alias, "%s%d", prefix, maxidx) < 0) return -1;

return 0; @@ -445,7 +451,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nmems; i++) { - if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0) + if (qemuAssignDeviceMemoryAlias(def, def->mems[i]) < 0) return -1; }

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a4cc87f..6b83d1c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3061,6 +3061,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, * to, or -1 if NUMA is not used in the guest * @hostNodes: map of host nodes to alloc the memory in, NULL for default * @autoNodeset: fallback nodeset in case of automatic numa placement + * @memPath: request memory-backend-file with specific mem-path * @def: domain definition object * @qemuCaps: qemu capabilities object * @cfg: qemu driver config object @@ -3082,6 +3083,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, int guestNode, virBitmapPtr userNodeset, virBitmapPtr autoNodeset, + const char *memPath, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, @@ -3173,35 +3175,42 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (!(props = virJSONValueNewObject())) return -1;

- if (pagesize || hugepage) { - if (pagesize) { - /* Now lets see, if the huge page we want to use is even mounted - * and ready to use */ - for (i = 0; i < cfg->nhugetlbfs; i++) { - if (cfg->hugetlbfs[i].size == pagesize) - break; - } + if (memPath || pagesize || hugepage) { + if (pagesize || hugepage) { + if (pagesize) { + /* Now lets see, if the huge page we want to use is even mounted + * and ready to use */ + for (i = 0; i < cfg->nhugetlbfs; i++) { + if (cfg->hugetlbfs[i].size == pagesize) + break; + }

- if (i == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - pagesize); - goto cleanup; - } + if (i == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs mount for %llu KiB"), + pagesize); + goto cleanup; + }

- if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) - goto cleanup; - } else { - if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) - goto cleanup; + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + goto cleanup; + } else { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + goto cleanup; + } }

*backendType = "memory-backend-file";

if (virJSONValueObjectAdd(props, + "s:mem-path", memPath ? memPath : mem_path, + NULL) < 0) + goto cleanup; + + if (!memPath && (pagesize || hugepage) && + virJSONValueObjectAdd(props, "b:prealloc", true, - "s:mem-path", mem_path, NULL) < 0) goto cleanup;

Two trips through virJSONValueObjectAdd... IIRC a prior review of my code noted it's an expensive trip into that code... Besides you're switching the arguments resulting changes to all the dimm tests... Perhaps this should just be an if (memPath) "s:mem-path", memPath else "b:prealloc" "s:mem-path", mem_path In fact rather than the excess of changes with indents, why not keep: if (pagesize || hugepage) { } and replace the closing } with } else if (memPath) { } I think that would be cleaner and more obvious that they're different.

@@ -3253,7 +3262,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, }

/* If none of the following is requested... */ - if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force) { + if (!pagesize && !userNodeset && !memAccess && !nodeSpecified && !force && !memPath) { /* report back that using the new backend is not necessary * to achieve the desired configuration */ ret = 1; @@ -3309,7 +3318,7 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, goto cleanup;

if ((rc = qemuBuildMemoryBackendStr(memsize, 0, cell, NULL, auto_nodeset, - def, qemuCaps, cfg, &backendType, + NULL, def, qemuCaps, cfg, &backendType, &props, false)) < 0) goto cleanup;

@@ -3351,7 +3360,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,

if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, auto_nodeset, - def, qemuCaps, cfg, + mem->path, def, qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup;

@@ -3369,6 +3378,7 @@ char * qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) { virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *device;

if (!mem->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3377,8 +3387,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); @@ -3393,12 +3410,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; @@ -6938,6 +6949,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 @@ -6974,6 +6986,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]; @@ -7070,6 +7091,13 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } }

+ for (i = 0; i < def->nmems; i++) { + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + virBufferAddLit(&buf, ",nvdimm=on"); + break; + } + } +

So I know we check later on when building the NVDIMM command line for the QEMU_CAPS_DEVICE_NVDIMM; however, should we also check here. Mostly for completeness

virCommandAddArgBuffer(cmd, &buf); }

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index dcf9ba6..003a5d7 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -118,6 +118,7 @@ int qemuBuildMemoryBackendStr(unsigned long long size, int guestNode, virBitmapPtr userNodeset, virBitmapPtr autoNodeset, + const char *memPath, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 28ee81d..6b049e7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5289,12 +5289,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

BTW: So the "if (nmems > def->mem.memory_slots) {" check is what got me initially thinking about the "how this is used" query I brought up in patch 1 for formatdomain.

@@ -5318,6 +5312,28 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, for (i = 0; i < def->nmems; i++) { hotplugMemory += def->mems[i]->size;

This to is where I started wondering "how" this was implemented and how it looks to the guest. Does the guest believe it has "extra" memory? Unlike a DIMM, I wouldn't expect the memory to be general purpose usage for the guest and thus wonder if the value for NVDIMM should be included. I seem to have this recollection of some other algorithm that makes sure address space(s) used by multiple dimm's don't have overlaps; however, that wouldn't be true for an nvdimm. Perhaps that answer will be in later patches.

+ 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; + } +

Good thing there aren't 1000's of these things... That's a check every time through the loop 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 00e4a75..bf22b0a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1878,7 +1878,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,

if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, NULL, - vm->def, priv->qemuCaps, cfg, + mem->path, vm->def, priv->qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup;

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 2eb006e..dd12751 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -13,9 +13,8 @@ QEMU_AUDIO_DRV=spice \ -mem-prealloc \ -mem-path /dev/hugepages2M/libvirt/qemu \ -numa node,nodeid=0,cpus=0-1,mem=1024 \ --object memory-backend-file,id=memdimm0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,\ -policy=bind \ +-object memory-backend-file,id=memdimm0,mem-path=/dev/hugepages1G/libvirt/qemu,\ +prealloc=yes,size=1073741824,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ -uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ -nodefaults \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args index 9f0e696..2a196ab 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args @@ -10,21 +10,21 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 4096 \ -smp 4,sockets=4,cores=1,threads=1 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +-object memory-backend-file,id=ram-node0,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +-object memory-backend-file,id=ram-node1,\ +mem-path=/dev/hugepages2M/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ --object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +-object memory-backend-file,id=ram-node2,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ --object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ -policy=bind \ +-object memory-backend-file,id=ram-node3,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args index 447bb52..30f87a8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args @@ -10,11 +10,11 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 1024 \ -smp 2,sockets=2,cores=1,threads=1 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=268435456 \ +-object memory-backend-file,id=ram-node0,\ +mem-path=/dev/hugepages2M/libvirt/qemu,prealloc=yes,size=268435456 \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=805306368 \ +-object memory-backend-file,id=ram-node1,\ +mem-path=/dev/hugepages2M/libvirt/qemu,prealloc=yes,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -uuid ef1bdff4-27f3-4e85-a807-5fb4d58463cc \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args index 57dd3fa..92045a0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -12,8 +12,8 @@ QEMU_AUDIO_DRV=none \ -smp 2,sockets=2,cores=1,threads=1 \ -object memory-backend-ram,id=ram-node0,size=268435456 \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=805306368 \ +-object memory-backend-file,id=ram-node1,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -uuid ef1bdff4-27f3-4e85-a807-5fb4d58463cc \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args index f9fc218..aaa9e99 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args @@ -10,21 +10,21 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 4096 \ -smp 4,sockets=4,cores=1,threads=1 \ --object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +-object memory-backend-file,id=ram-node0,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ --object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,share=yes,size=1073741824,\ +-object memory-backend-file,id=ram-node1,\ +mem-path=/dev/hugepages2M/libvirt/qemu,prealloc=yes,share=yes,size=1073741824,\ host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ --object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,share=no,size=1073741824,host-nodes=0-3,\ -policy=bind \ +-object memory-backend-file,id=ram-node2,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,share=no,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ --object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ -policy=bind \ +-object memory-backend-file,id=ram-node3,\ +mem-path=/dev/hugepages1G/libvirt/qemu,prealloc=yes,size=1073741824,\ +host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args index 1c881c6..ea46c82 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args @@ -11,9 +11,8 @@ QEMU_AUDIO_DRV=none \ -m size=219136k,slots=16,maxmem=1099511627776k \ -smp 2,sockets=2,cores=1,threads=1 \ -numa node,nodeid=0,cpus=0-1,mem=214 \ --object memory-backend-file,id=memdimm0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\ -policy=bind \ +-object memory-backend-file,id=memdimm0,mem-path=/dev/hugepages2M/libvirt/qemu,\ +prealloc=yes,size=536870912,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0,addr=4294967296 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args index fa64fcf..dc58614 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args @@ -13,9 +13,8 @@ QEMU_AUDIO_DRV=none \ -numa node,nodeid=0,cpus=0-1,mem=214 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ --object memory-backend-file,id=memdimm1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\ -policy=bind \ +-object memory-backend-file,id=memdimm1,mem-path=/dev/hugepages2M/libvirt/qemu,\ +prealloc=yes,size=536870912,host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm1,id=dimm1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args new file mode 100644 index 0000000..8cda774 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.args @@ -0,0 +1,25 @@ +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=219136k,slots=16,maxmem=1099511627776k \ +-smp 2,sockets=2,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0-1,mem=214 \ +-object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,size=536870912 \ +-device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0 \ +-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 ad0693f..1995ccc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1977,7 +1977,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); @@ -1985,6 +1985,8 @@ mymain(void) 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,

On 08/11/2016 09:26 AM, Michal Privoznik wrote:

Now that NVDIMM has found its way into libvirt, users might want to fine tune some settings for each module separately. One such setting is 'share=on|off' for the memory-backend-file object. This setting - just like its name suggest already - enables sharing the nvdimm module with other applications. Under the hood it controls whether qemu mmaps() the file as MAP_PRIVATE or MAP_SHARED.

Yet again, we have such config knob in domain XML, but it's just an attribute to numa <cell/>. This does not give fine enough tuning on per-memdevice basis so we need to have the attribute for each device too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 15 ++++++- docs/schemas/domaincommon.rng | 19 ++++++--- src/conf/domain_conf.c | 15 ++++++- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 2 + ...emuxml2argv-memory-hotplug-nvdimm-memAccess.xml | 49 ++++++++++++++++++++++ 6 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.xml

After reading this patch and the next one, I'm not sure I understand the need. For a 'dimm' it already takes a value from whatever numa has for the node. Adding this would seemingly allow someone to overwrite that value. So what would happen if the numa node was private and the *dimm node shared? The setting and usage does not restrict to nvdimm only. The rest of this is my thoughts upon first read... I'm still hung up on whether dimm and nvdimm should share nmems, but still figured I'd look at more patches to provide more thoughts.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 657df8f..06fe50d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1356,7 +1356,7 @@ <span class='since'>Since 1.2.9</span> the optional attribute <code>memAccess</code> can control whether the memory is to be mapped as "shared" or "private". This is valid only for - hugepages-backed memory. + hugepages-backed memory and nvdimm modules. </p>

<p> @@ -6724,7 +6724,7 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> - <memory model='dimm'> + <memory model='dimm' memAccess='private'>

'dimm'?? Shouldn't this be on the 'nvdimm'? The way I read the commit msg it's for nvdimm only. It would seem this would allow the dimm I would think there'd be concerns over nefarious uses of this hole in the guest...

<target> <size unit='KiB'>524287</size> <node>0</node> @@ -6762,6 +6762,17 @@ qemu-kvm -net nic,model=? /dev/null </p> </dd>

+ <dt><code>memAccess</code></dt> + <dd> + <p> + Then there is optional attribute <code>memAccess</code> + (<span class="since">Since 2.2.0</span>) that allows

2.3.0 at the earliest.

+ uses to fine tune mapping of the memory on per module + basis. Values are the same as for numa <cell/>: + <code>shared</code> and <code>private</code>. + </p> + </dd> +

Placement. This is an attribute of <memory...> but currently "tied to" nvdimm. Making this a <dt> at the same level of <source> gives me the wrong impression. I think the text of the message belongs in the previous paragraph. E.g. "... a Non-Volatile DIMM module. For NVDIMM modules, an optional attribute <code>memAccess</code> can be provided. This allows users to fine tune mapping of the memory on a per module basis. Values are the same as..." BTW: It took me a few searches to find the shared/private NUMA discussion in the "#elementsCPU"... Makes me wonder if we should create another label "#numaTopology" that allows us to link directly to that discussion from right here.

<dt><code>source</code></dt> <dd> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6ad452..3e9d342 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4467,6 +4467,15 @@ </element> </define>

+ <define name="memAccess"> + <attribute name="memAccess"> + <choice> + <value>shared</value> + <value>private</value> + </choice> + </attribute> + </define> + <define name="numaCell"> <element name="cell"> <optional> @@ -4486,12 +4495,7 @@ </attribute> </optional> <optional> - <attribute name="memAccess"> - <choice> - <value>shared</value> - <value>private</value> - </choice> - </attribute> + <ref name="memAccess"/> </optional> </element> </define> @@ -4725,6 +4729,9 @@ <value>nvdimm</value> </choice> </attribute> + <optional> + <ref name="memAccess"/> + </optional> <interleave> <optional> <ref name="memorydev-source"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cb5a053..84f76dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13245,6 +13245,15 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, } VIR_FREE(tmp);

+ tmp = virXMLPropString(memdevNode, "memAccess"); + if (tmp && + (def->memAccess = virNumaMemAccessTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid memAccess mode '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); +

An no check for NVDIMM usage only... What if someone does add this to their DIMM? In the next patch, we'll take whatever this is set to and change the memAccess based on that. I would also think that if we did support this that we'd *have* to have some ABI check to make sure src and dst used the same memAccess mode...

/* source */ if ((node = virXPathNode("./source", ctxt)) && virDomainMemorySourceDefParseXML(node, ctxt, def) < 0) @@ -21800,7 +21809,11 @@ virDomainMemoryDefFormat(virBufferPtr buf, { const char *model = virDomainMemoryModelTypeToString(def->model);

- virBufferAsprintf(buf, "<memory model='%s'>\n", model); + virBufferAsprintf(buf, "<memory model='%s'", model); + if (def->memAccess) + virBufferAsprintf(buf, " memAccess='%s'", + virNumaMemAccessTypeToString(def->memAccess)); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);

if (virDomainMemorySourceDefFormat(buf, def) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4e6a9bf..213768d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1945,6 +1945,8 @@ typedef enum { } virDomainMemoryModel;

struct _virDomainMemoryDef { + virNumaMemAccess memAccess; + /* source */ virBitmapPtr sourceNodes; unsigned long long pagesize; /* kibibytes */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 419c33d..024c3e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2058,6 +2058,8 @@ virNumaGetNodeMemory; virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; +virNumaMemAccessTypeFromString; +virNumaMemAccessTypeToString; virNumaNodeIsAvailable; virNumaNodesetIsAvailable; virNumaSetPagePoolSize; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.xml new file mode 100644 index 0000000..4ebea01 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.xml @@ -0,0 +1,49 @@ +<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='219136' 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'/> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + <memory model='nvdimm' memAccess='private'> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit='KiB'>523264</size> + <node>0</node> + </target> + </memory> + </devices> +</domain>

And again no xml2xml tests... So in this case, we overwrite whatever the numa default is. If the numa default were private, how would it be possible to have the nvdimm allow shared? John

On 08/11/2016 09:26 AM, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 11 +++++++-- src/qemu/qemu_command.h | 1 + src/qemu/qemu_hotplug.c | 3 ++- ...muxml2argv-memory-hotplug-nvdimm-memAccess.args | 26 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6b83d1c..f888de3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3062,6 +3062,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, * @hostNodes: map of host nodes to alloc the memory in, NULL for default * @autoNodeset: fallback nodeset in case of automatic numa placement * @memPath: request memory-backend-file with specific mem-path + * @memAccessReq: specifically requested memAccess mode * @def: domain definition object * @qemuCaps: qemu capabilities object * @cfg: qemu driver config object @@ -3084,6 +3085,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virBitmapPtr userNodeset, virBitmapPtr autoNodeset, const char *memPath, + virNumaMemAccess memAccessReq, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, @@ -3119,6 +3121,9 @@ qemuBuildMemoryBackendStr(unsigned long long size, memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode); }

+ if (memAccessReq) + memAccess = memAccessReq; +

This would overwrite the value for dimm as well, which I'm still not sure is what you expected. John

if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 && virDomainNumatuneGetMode(def->numa, -1, &mode) < 0) mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; @@ -3318,7 +3323,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, goto cleanup;

if ((rc = qemuBuildMemoryBackendStr(memsize, 0, cell, NULL, auto_nodeset, - NULL, def, qemuCaps, cfg, &backendType, + NULL, VIR_NUMA_MEM_ACCESS_DEFAULT, + def, qemuCaps, cfg, &backendType, &props, false)) < 0) goto cleanup;

@@ -3360,7 +3366,8 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,

if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, auto_nodeset, - mem->path, def, qemuCaps, cfg, + mem->path, mem->memAccess, + def, qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup;

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 003a5d7..29c0f58 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -119,6 +119,7 @@ int qemuBuildMemoryBackendStr(unsigned long long size, virBitmapPtr userNodeset, virBitmapPtr autoNodeset, const char *memPath, + virNumaMemAccess memAccessReq, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virQEMUDriverConfigPtr cfg, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bf22b0a..6ba0b8e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1878,7 +1878,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,

if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize, mem->targetNode, mem->sourceNodes, NULL, - mem->path, vm->def, priv->qemuCaps, cfg, + mem->path, mem->memAccess, vm->def, + priv->qemuCaps, cfg, &backendType, &props, true) < 0) goto cleanup;

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.args new file mode 100644 index 0000000..9446259 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.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=219136k,slots=16,maxmem=1099511627776k \ +-smp 2,sockets=2,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0-1,mem=214 \ +-object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,share=no,\ +size=536870912 \ +-device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0 \ +-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 1995ccc..985412c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1987,6 +1987,8 @@ mymain(void) 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("memory-hotplug-nvdimm-memAccess", 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,

On 08/11/2016 09:26 AM, Michal Privoznik wrote:

Now that we have APIs for relabel memdevs on hotplug, fill in the missing implementation in qemu hotplug code.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

Note: Patches 6-9 have an implicit ACK - they seem to be fairly standard. Although what about apparmour?

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ba0b8e..afabbda 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1861,6 +1861,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, int id; int ret = -1; int rv; + bool restoreLabel = false;

qemuDomainMemoryDeviceAlignSize(vm->def, mem);

@@ -1893,6 +1894,11 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, goto removedef; }

+ if (virSecurityManagerSetMemoryLabel(driver->securityManager, + vm->def, mem) < 0) + goto cleanup; + restoreLabel = true; + qemuDomainObjEnterMonitor(driver, vm); rv = qemuMonitorAddObject(priv->mon, backendType, objalias, props); props = NULL; /* qemuMonitorAddObject consumes */ @@ -1945,6 +1951,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, mem = NULL; goto audit; } + if (mem && restoreLabel &&

Coverity notes that checking for mem here is unnecessary. It dereffed at the top and there is no way to get to the exit_monitor label after the mem = NULL.

+ virSecurityManagerRestoreMemoryLabel(driver->securityManager, + vm->def, mem) < 0) + VIR_WARN("Unable to restore security label on memdev");

In any case, if this does stay within this label, I think it should move to inside the 'orig_err' code... The question becomes, if the qemuDomainObjExitMonitor fails, should the Restore be called as well. Part of me says yes, but then it's noted in the failure to ExitMonitor that we cannot touch mem, so we're SOL. John

removedef: if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0) @@ -3141,6 +3151,10 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0) virDomainMemoryRemove(vm->def, idx);

+ if (virSecurityManagerRestoreMemoryLabel(driver->securityManager, + vm->def, mem) < 0) + VIR_WARN("Unable to restore security label on memdev"); + virDomainMemoryDefFree(mem);

/* fix the balloon size */