[libvirt] [PATCH] conf: Introduce align for hostmem-file
John Ferlan
jferlan at redhat.com
Mon Jun 4 20:04:08 UTC 2018
[removed developers from To since they read libvir-list anyway]
On 05/29/2018 08:38 AM, Jie Wang wrote:
> QEMU has add the 'align' option to 'memory-backend-file'. Expose
> this option to users by new element align.
>
Would perhaps be nice to have a few more details about what this is as
part of the commit message... and the expected usage.
I see that "align" was added in 2.12 by commit id '98376843', so you'll
need to add a capability for the switch; otherwise, supplying align=XXX
to a pre-2.12 emulator would probably fail, correct?
See libvirt commit '72c1770aa0' done for
"memory-backend-file.discard-data" for the methodology, but I expect the
next round to contain:
{ "align", QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN },
in virQEMUCapsObjectPropsMemoryBackendFile along with changes to the
various tests/qemucapabilitiesdata/caps_2.12*.xml files which you can
generate via "VIR_TEST_REGENERATE_OUTPUT=1 tests/qemucapabilitiestest"
once you have compiled the qemu_capabilities.{c,h} changes.
> Signed-off-by: Jie Wang <wangjie88 at huawei.com>
> ---
> docs/formatdomain.html.in | 18 +++++++
> docs/schemas/domaincommon.rng | 7 +++
> src/conf/domain_conf.c | 14 +++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 4 ++
> .../memory-hotplug-nvdimm-align.args | 31 +++++++++++
> .../memory-hotplug-nvdimm-align.xml | 63 ++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 3 ++
> .../memory-hotplug-nvdimm-align.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 10 files changed, 143 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
> create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
> create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
>
Split the patches into domain_conf, formatdomain, xml2xml testing as one
patch and qemu_command, qemuxml2argv tests in another patch. In between
the two patches, sandwich in the qemu_capabilities changes. You'll also
need a patch 4 to adjust docs/news.xml to briefly describe the enhancement.
Before reposting, please be sure to pass the "make [-j N] check
syntax-check" as check fails with:
FAIL: virschematest
FAIL: qemuxml2xmltest
and syntax-check fails with (at least):
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
maint.mk: empty line(s) or no newline at EOF
make: *** [maint.mk:930: sc_prohibit_empty_lines_at_EOF] Error 1
make: *** Waiting for unfinished jobs....
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0d0fd3b..29fe145 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7932,6 +7932,9 @@ qemu-kvm -net nic,model=? /dev/null
> </target>
> </memory>
> <memory model='nvdimm'>
> + <align>
> + <sieze unit='KiB'>2048</size>
s/sieze/size/
> + </align>
[1] see notes regarding formatting...
> <source>
> <path>/tmp/nvdimm</path>
> </source>
> @@ -7983,6 +7986,21 @@ qemu-kvm -net nic,model=? /dev/null
> </p>
> </dd>
>
> + <dt><code>align</code></dt>
> + <dd>
> + <p>
> + For NVDIMM type devices one can optionally use
> + <code>align</code> and its subelement <code>size</code>
> + to configure the size of alignment within the NVDIMM module.
> + The <code>size</code> element has usual meaning described
> + <a href="#elementsMemoryAllocation">here</a>.
I see you copied the NVDIMM target size description, but I think it's
better described using something like:
"The size of the XXXX. The value by default is in kilobytes, but the
unit attribute can be used to scale the value."
Search around the formatdomain.html.in for other uses of size for examples.
Beyond that, the description in/from the qemu commit regarding how this
is used, might be good to at least paraphrase in the libvirt description.
> + For QEMU domains the following restrictions apply:
> + </p>
> + <ol>
> + <li>the alignment must be multiples of page size 4KiB,</li>
> + </ol>
Strange to just have 1 element in a list - in that case, just indicate
that for QEMU domains the alignment size must be...
BTW: I would have expected in some qemu_domain post parse function to
see a VIR_ROUND_UP for this or a comparison forcing failure because the
value isn't aligned properly. Hint, qemuDomainGetMemorySizeAlignment
> + </dd>
> +
> <dt><code>source</code></dt>
> <dd>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 71ac3d0..9e994b1 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5145,6 +5145,13 @@
> <ref name="virYesNo"/>
> </attribute>
> </optional>
> + <optional>
> + <element name="align">
> + <element name="size">
> + <ref name="scaledInteger"/>
> + </element>
> + </element>
> + </optional>
[1]
This should just follow the "pagesize" value. There's no need for:
<align>
<size unit='KiB'>2048</size>
</align>
it should be :
<align unit='KiB'>2048</align>
and it should be a child of <source>, just like <pagesize>
Thus changing:
<group>
<element name="path">
<ref name="absFilePath"/>
</element>
</group>
to be:
<group>
<interleave>
<element name="path">
<ref name="absFilePath"/>
</element>
<optional>
<element name="align">
<ref name="scaledInteger"/>
</element>
</optional>
</interleave>
</group>
That way it's *very obvious* that this align is only for nvdimm
> <interleave>
> <optional>
> <ref name="memorydev-source"/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3689ac0..bf91167 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15739,6 +15739,12 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> VIR_FREE(tmp);
>
> + if ((node = virXPathNode("./align", ctxt))) {
> + if (virDomainParseMemory("./align/size", "./align/size/@unit", ctxt,
> + &def->align, true, false) < 0)
> + goto error;
> + }
> +
This then moves into the source parsing.
> /* source */
> if ((node = virXPathNode("./source", ctxt)) &&
> virDomainMemorySourceDefParseXML(node, ctxt, def) < 0)
> @@ -25334,6 +25340,14 @@ virDomainMemoryDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
>
> + if (def->align) {
I prefer, def->align > 0 - the value isn't a bool or a pointer.
> + virBufferAddLit(buf, "<align>\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->align);
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</align>\n");
> + }
> +
and all of this moves into source def format
> if (virDomainMemorySourceDefFormat(buf, def) < 0)
> return -1;
>
Should virDomainMemoryDefCheckABIStability ensure that align sizes
between @src and @dst match?
Use cscope to search the sources for VIR_DOMAIN_MEMORY_MODEL_NVDIMM to
make sure there's no other places that may need adjustment... I think
you probably need a change to the virDomainMemoryDefValidate to check if
align is supplied for memory types other than nvdimm and then fail, alt
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a78fdee..1155c84 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2107,6 +2107,7 @@ typedef enum {
> struct _virDomainMemoryDef {
> virDomainMemoryAccess access;
> virTristateBool discard;
> + unsigned long long align;
Since this alignment only for NVDIMM, it should be renamed to
nvdimmAlign and put under nvdimmPath.
>
> /* source */
> virBitmapPtr sourceNodes;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c423733..5862457 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3186,6 +3186,10 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
> goto cleanup;
>
> + if (mem->align &&
> + virJSONValueObjectAdd(props, "u:align", mem->align * 1024, NULL) < 0)
If you use "U:align" you don't need the mem->align condition.
> + goto cleanup;
> +
> if (mem->sourceNodes) {
> nodemask = mem->sourceNodes;
> } else {
> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
> new file mode 100644
> index 0000000..e6fcf58
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
> @@ -0,0 +1,31 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-i686 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc,accel=tcg,usb=off,dump-guest-core=off,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,prealloc=yes,mem-path=/tmp/nvdimm,\
> +share=no,size=536870912,align=2097152 \
> +-device nvdimm,node=0,label-size=131072,memdev=memnvdimm0,id=nvdimm0,slot=0 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-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/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
> new file mode 100644
> index 0000000..aa9e99b
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
> @@ -0,0 +1,63 @@
> +<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'>
> + <align>
> + <size unit='KiB'>2048</size>
> + </align>
[1] From above will change this...
> + <source>
> + <path>/tmp/nvdimm</path>
> + </source>
> + <target>
> + <size unit='KiB'>523264</size>
> + <node>0</node>
> + <label>
> + <size unit='KiB'>128</size>
> + </label>
> + </target>
> + <address type='dimm' slot='0'/>
> + </memory>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 07e5ba1..4674ded 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2588,6 +2588,9 @@ mymain(void)
> DO_TEST("memory-hotplug-nvdimm-label",
> QEMU_CAPS_DEVICE_NVDIMM,
> QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> + DO_TEST("memory-hotplug-nvdimm-align",
> + QEMU_CAPS_DEVICE_NVDIMM,
> + QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
Use DO_TEST_CAPS_LATEST
>
> DO_TEST("machine-aeskeywrap-on-caps",
> QEMU_CAPS_AES_KEY_WRAP,
> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml
> new file mode 100644
> 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
This isn't a link to the file - the contents of the file is a link...
That's probably why schematest and xml2xmltest fail
John
No need to CC me on the next series, I follow the list. As time permits
in my schedule, I do reviews unless someone else gets to them before me.
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 7cedc2b..822e98a 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1098,6 +1098,7 @@ 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("net-udp", NONE);
>
> DO_TEST("video-virtio-gpu-device", NONE);
>
More information about the libvir-list
mailing list