Re: [libvirt] [PATCH v2 01/10] Introduce NVDIMM memory model

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(a)redhat.com&gt; > --- > 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 > &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;524287&lt;/size&gt; > + &lt;node&gt;1&lt;/node&gt; > + &lt;/target&gt; > + &lt;/memory&gt; > &lt;/devices&gt; > ... > </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?

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.

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.

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

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

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

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.

And well - what about migration?