Re: [libvirt] [RFC v3 1/4] nvdimm: introduce more config elements into xml for NVDIMM memory

On 12/12/18 7:52 AM, Luyao Zhong wrote: > 1.alignsize > The 'alignsize' option allows users to specify the proper alignment. > > 2.pmem > The 'pmem' option allows users to specify whether the backend storage of > memory-backend-file is a real persistent memory. > > 3.unarmed > The 'unarmed' option allows users to mark vNVDIMM read-only. > > These options can be configured respectively or simultaneously in domain > xml file, here is an example: > <devices> > ... > <memory model='nvdimm' access='shared'> > <source> > <path>/dev/dax0.0</path> > <alignsize unit='MiB'>2</alignsize> > <pmem/> > </source> > <target> > <size unit='MiB'>4094</size> > <node>0</node> > <label> > <size unit='MiB'>2</size> > </label> > <unarmed/> > </target> > </memory> > ... > </devices> > > Signed-off-by: Luyao Zhong <luyao.zhong(a)intel.com&gt; > --- > docs/formatdomain.html.in | 80 ++++++++++++++++++---- > docs/schemas/domaincommon.rng | 23 ++++++- > src/conf/domain_conf.c | 61 +++++++++++++++-- > src/conf/domain_conf.h | 3 + > .../memory-hotplug-nvdimm-align.xml | 58 ++++++++++++++++ > .../memory-hotplug-nvdimm-pmem.xml | 58 ++++++++++++++++ > .../memory-hotplug-nvdimm-unarmed.xml | 58 ++++++++++++++++ > .../memory-hotplug-nvdimm-align.xml | 1 + > .../memory-hotplug-nvdimm-pmem.xml | 1 + > .../memory-hotplug-nvdimm-unarmed.xml | 1 + > tests/qemuxml2xmltest.c | 3 + > 11 files changed, 322 insertions(+), 25 deletions(-) > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml > fails syntax-check

Adding 3 things at one time borders on separation of concepts, but it's also a pain to repeat the same process. Still it's also easier to review and bisect if/when issues arise, separation is preferred.

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 428b0e8..3f813f2 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -8322,6 +8322,8 @@ qemu-kvm -net nic,model=? /dev/null > &lt;memory model='nvdimm'&gt; > &lt;source&gt; > &lt;path&gt;/tmp/nvdimm&lt;/path&gt; > + &lt;alignsize unit='KiB'&gt;2048&lt;/alignsize&gt; > + &lt;pmem/&gt; > &lt;/source&gt; > &lt;target&gt; > &lt;size unit='KiB'&gt;524288&lt;/size&gt; > @@ -8329,6 +8331,7 @@ qemu-kvm -net nic,model=? /dev/null > &lt;label&gt; > &lt;size unit='KiB'&gt;128&lt;/size&gt; > &lt;/label&gt; > + &lt;unarmed/&gt; > &lt;/target&gt; > &lt;/memory&gt; > &lt;/devices&gt; > @@ -8403,10 +8406,37 @@ qemu-kvm -net nic,model=? /dev/null > </dl> > > <p> > - 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. > + For model <code>nvdimm</code> this element is mandatory. The > + mandatory child element <code>path</code> represents a path in > + the host that backs the nvdimm module in the guest. If > + <code>nvdimm</code> is provided, then the following optional > + elements can be provided as well: > </p> > + > + <dl> > + <dt><code>alignsize</code></dt> > + <dd> > + <p> > + This element can be used to specify a proper alignment. > + When mmap(2) the backend files, QEMU uses the host page > + size by default as the alignment of mapping address. However, > + some backends may require alignments different from the page. > + For example, mmap a real NVDIMM device maybe 2M-aligned required. > + <span class="since">Since 4.9.0</span> At the earliest 5.0.0 (the next release)...

> + </p> > + </dd> > + > + <dt><code>pmem</code></dt> > + <dd> > + <p> > + This element can be used to specify whether the backend storage > + of memory-backend-file is a real persistent memory. If the backend "is a real" should that be "is using real" ?? > + is a real persistence memory and <code>pmem</code> is set, QEMU > + will guarantee the persistence of its own writes to the vNVDIMM > + backend.<span class="since">Since 4.9.0</span> Add a space between .<

Same > + </p> > + </dd> > + </dl> > </dd> > > <dt><code>target</code></dt> > @@ -8425,19 +8455,39 @@ qemu-kvm -net nic,model=? /dev/null > NUMA nodes configured. > </p> > <p> > - For NVDIMM type devices one can optionally use > - <code>label</code> and its subelement <code>size</code> > - to configure the size of namespaces label storage > - within the NVDIMM module. The <code>size</code> element > - has usual meaning described > - <a href="#elementsMemoryAllocation">here</a>. > - For QEMU domains the following restrictions apply: > + Besides, the following optional elements can be provided as well for > + NVDIMM type devices: > </p> > - <ol> > - <li>the minimum label size is 128KiB,</li> > - <li>the remaining size (total-size - label-size) has to be aligned to > - 4KiB</li> > - </ol> > + > + <dl> > + <dt><code>label</code></dt> > + <dd> > + <p> > + For NVDIMM type devices one can optionally use > + <code>label</code> and its subelement <code>size</code> > + to configure the size of namespaces label storage > + within the NVDIMM module. The <code>size</code> element > + has usual meaning described > + <a href="#elementsMemoryAllocation">here</a>. > + For QEMU domains the following restrictions apply: > + </p> > + <ol> > + <li>the minimum label size is 128KiB,</li> > + <li>the remaining size (total-size - label-size) will be aligned to > + 4KiB as default.</li> > + </ol> > + </dd> > + > + <dt><code>unarmed</code></dt> > + <dd> > + <p> > + The <code>unarmed</code> element can be used to mark vNVDIMM read-only. > + Currently, only real NVDIMM device backend can guarantee the guest write > + persistence, so please set <code>unarmed</code> when using other types > + of backends.<span class="since">Since 4.9.0</span> Similar w/ ".<" and usage of 5.0.0 instead.

> + </p> > + </dd> > + </dl> > </dd> > </dl> > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 5a6c48f..de098a5 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -5384,9 +5384,21 @@ > </interleave> > </group> > <group> > - <element name="path"> > - <ref name="absFilePath"/> > - </element> > + <interleave> > + <element name="path"> > + <ref name="absFilePath"/> > + </element> > + <optional> > + <element name="alignsize"> > + <ref name="scaledInteger"/> > + </element> > + </optional> > + <optional> > + <element name="pmem"> > + <empty/> > + </element> > + </optional> > + </interleave> > </group> > </choice> > </element> > @@ -5410,6 +5422,11 @@ > </element> > </element> > </optional> > + <optional> > + <element name="unarmed"> > + <empty/> > + </element> > + </optional> > </interleave> > </element> > </define> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index efa0a94..e16262b 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -15757,6 +15757,16 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, > _("path is required for model 'nvdimm'")); > goto cleanup; > } > + > + if (virDomainParseMemory("./alignsize", "./alignsize/@unit", ctxt, > + &def->alignsize, false, false) < 0) > + goto cleanup; > + > + if (virXPathBoolean("boolean(./pmem)", ctxt)) > + def->nvdimmPmem = true; > + else > + def->nvdimmPmem = false; > + The else is unnecessary since the default is false

> break; > > case VIR_DOMAIN_MEMORY_MODEL_NONE: > @@ -15813,6 +15823,11 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, > _("label size must be smaller than NVDIMM size")); > goto cleanup; > } > + > + if (virXPathBoolean("boolean(./unarmed)", ctxt)) > + def->nvdimmUnarmed = true; > + else > + def->nvdimmUnarmed = false; The else is unnecessary since the default is false.

> } > > ret = 0; > @@ -22712,13 +22727,36 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, > return false; > } > > - if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && > - src->labelsize != dst->labelsize) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("Target NVDIMM label size '%llu' doesn't match " > - "source NVDIMM label size '%llu'"), > - src->labelsize, dst->labelsize); > - return false; > + if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { > + if (src->labelsize != dst->labelsize) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Target NVDIMM label size '%llu' doesn't match " > + "source NVDIMM label size '%llu'"), > + src->labelsize, dst->labelsize); > + return false; > + } > + > + if (src->alignsize != dst->alignsize) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Target NVDIMM alignment '%llu' doesn't match " > + "source NVDIMM alignment '%llu'"), > + src->alignsize, dst->alignsize); > + return false; > + } > + > + if (src->nvdimmPmem != dst->nvdimmPmem) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, Since there's nothing to format you need a "%s", as 2nd arg

> + _("Target NVDIMM pmem flag doesn't match " > + "source NVDIMM pmem flag ")); Remove the trailing space after flag

> + return false; > + } > + > + if (src->nvdimmUnarmed != dst->nvdimmUnarmed) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Target NVDIMM unarmed flag doesn't match " > + "source NVDIMM unarmed flag ")); same for this one too. > + return false; > + } > } > > return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); > @@ -26255,6 +26293,13 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, > > case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: > virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath); > + > + if (def->alignsize) > + virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n", > + def->alignsize); > + > + if (def->nvdimmPmem) > + virBufferAddLit(buf, "<pmem/>\n"); > break; > > case VIR_DOMAIN_MEMORY_MODEL_NONE: > @@ -26290,6 +26335,8 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf, > virBufferAdjustIndent(buf, -2); > virBufferAddLit(buf, "</label>\n"); > } > + if (def->nvdimmUnarmed) > + virBufferAddLit(buf, "<unarmed/>\n"); > > virBufferAdjustIndent(buf, -2); > virBufferAddLit(buf, "</target>\n"); > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index b24e6ec..e921f00 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2149,12 +2149,15 @@ struct _virDomainMemoryDef { > virBitmapPtr sourceNodes; > unsigned long long pagesize; /* kibibytes */ > char *nvdimmPath; > + unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */ > + bool nvdimmPmem; /* valid only for NVDIMM */ > > /* target */ > int model; /* virDomainMemoryModel */ > int targetNode; > unsigned long long size; /* kibibytes */ > unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */ > + bool nvdimmUnarmed; /* valid only for NVDIMM */ > > virDomainDeviceInfo info; > }; > diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml > new file mode 100644 > index 0000000..a8c5198 > --- /dev/null > +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml > @@ -0,0 +1,58 @@ > +<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> I know thiese are copies of memory-hotplug-nvdimm-access.xml, but rather than just be copies can "trim" out the excess stuff. I would think idmap is unnecessary. Same for other .xml additions.

John

> + <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-system-i686</emulator> > + <disk type='block' device='disk'> > + <driver name='qemu' type='raw'/> > + <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' access='private'> > + <source> > + <path>/tmp/nvdimm</path> > + <alignsize unit='KiB'>2048</alignsize> > + </source> > + <target> > + <size unit='KiB'>523264</size> > + <node>0</node> > + </target> > + <address type='dimm' slot='0'/> > + </memory> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml > new file mode 100644 > index 0000000..060d75c > --- /dev/null > +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml > @@ -0,0 +1,58 @@ > +<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-system-i686</emulator> > + <disk type='block' device='disk'> > + <driver name='qemu' type='raw'/> > + <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' access='private'> > + <source> > + <path>/tmp/nvdimm</path> > + <pmem/> > + </source> > + <target> > + <size unit='KiB'>523264</size> > + <node>0</node> > + </target> > + <address type='dimm' slot='0'/> > + </memory> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml > new file mode 100644 > index 0000000..7ddbb01 > --- /dev/null > +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml > @@ -0,0 +1,58 @@ > +<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-system-i686</emulator> > + <disk type='block' device='disk'> > + <driver name='qemu' type='raw'/> > + <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' access='private'> > + <source> > + <path>/tmp/nvdimm</path> > + </source> > + <target> > + <size unit='KiB'>523264</size> > + <node>0</node> > + <unarmed/> > + </target> > + <address type='dimm' slot='0'/> > + </memory> > + </devices> > +</domain> > diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml > new file mode 120000 > index 0000000..9fc6001 > --- /dev/null > +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml > @@ -0,0 +1 @@ > +../qemuxml2argvdata/memory-hotplug-nvdimm-align.xml > \ No newline at end of file > diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml > new file mode 120000 > index 0000000..3e57c1e > --- /dev/null > +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml > @@ -0,0 +1 @@ > +../qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml > \ No newline at end of file > diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml > new file mode 120000 > index 0000000..1793347 > --- /dev/null > +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml > @@ -0,0 +1 @@ > +../qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml > \ No newline at end of file > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c > index c98b957..01e3730 100644 > --- a/tests/qemuxml2xmltest.c > +++ b/tests/qemuxml2xmltest.c > @@ -1112,6 +1112,9 @@ mymain(void) > DO_TEST("memory-hotplug-nvdimm", NONE); > DO_TEST("memory-hotplug-nvdimm-access", NONE); > DO_TEST("memory-hotplug-nvdimm-label", NONE); > + DO_TEST("memory-hotplug-nvdimm-align", NONE); > + DO_TEST("memory-hotplug-nvdimm-pmem", NONE); > + DO_TEST("memory-hotplug-nvdimm-unarmed", NONE); > DO_TEST("net-udp", NONE); > > DO_TEST("video-virtio-gpu-device", NONE); >