[PATCH 0/3] A couple of virtio-mem/virtio-pmem address improvements
*** BLURB HERE *** Michal Prívozník (3): domain_validate: Validate VIRTIO_PMEM address alignment virDomainMemoryDefValidate: Fix VIRTIO_MEM alignment check virDomainMemoryDefValidate: Check for overlapping memory devices src/conf/domain_validate.c | 59 ++++++++++++++++++- ...rtio-mem-overlap-address.x86_64-latest.err | 1 + ...ory-hotplug-virtio-mem-overlap-address.xml | 50 ++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml -- 2.41.0
QEMU mandates the VIRTIO_PMEM address is aligned to a pagesize. This is a very reasonable requirement. So much so, that it deserves to be in hypervisor agnostic validation code (virDomainMemoryDefValidate()). Not that any other hypervisor would support VIRTIO_PMEM yet. But even if they did, this would surely be still valid. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index ac32fa1477..e423383e22 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2221,6 +2221,7 @@ static int virDomainMemoryDefValidate(const virDomainMemoryDef *mem, const virDomainDef *def) { + const long pagesize = virGetSystemPageSize(); unsigned long long thpSize; /* Guest NUMA nodes are continuous and indexed from zero. */ @@ -2295,6 +2296,14 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, _("virtio-pmem does not support NUMA nodes")); return -1; } + + if (pagesize > 0 && + mem->target.virtio_pmem.address % pagesize != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory address must be aligned to %1$ld bytes"), + pagesize); + return -1; + } break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: -- 2.41.0
Inside of virDomainMemoryDefValidate() there's a check that address where a virtio-mem memory device is mapped to is a multiple of its block size. But this check is off by a couple of bits, because the memory address is in bytes while the block size is in kibibytes. Therefore, when checking whether address is a multiple of the block size, the latter has to be multiplied by a factor of 1024. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index e423383e22..d14559cd73 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2341,7 +2341,8 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, return -1; } - if (mem->target.virtio_mem.address % mem->target.virtio_mem.blocksize != 0) { + /* blocksize is stored in KiB while address is in bytes */ + if (mem->target.virtio_mem.address % (mem->target.virtio_mem.blocksize * 1024) != 0) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("memory device address must be aligned to blocksize")); return -1; -- 2.41.0
As of v9.4.0-rc2~5 it is possible to specify guest address where a virtio-mem/virtio-pmem memory device is mapped to. What that commit forgot to introduce was a check for overlaps. And yes, this is technically an O(n^2) algorithm, as virDomainMemoryDefValidate() is called over each memory device and after this, virDomainMemoryDefValidate() also iterates over each memory device. But given there's usually only a handful of such devices, and this runs only when parsing domain XML I guess code readability wins over some less obvious solution. Resolves: https://issues.redhat.com/browse/RHEL-4452 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 47 +++++++++++++++++ ...rtio-mem-overlap-address.x86_64-latest.err | 1 + ...ory-hotplug-virtio-mem-overlap-address.xml | 50 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index d14559cd73..6962fe76bf 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2223,6 +2223,9 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, { const long pagesize = virGetSystemPageSize(); unsigned long long thpSize; + unsigned long long thisStart = 0; + unsigned long long thisEnd = 0; + size_t i; /* Guest NUMA nodes are continuous and indexed from zero. */ if (mem->targetNode != -1) { @@ -2304,6 +2307,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, pagesize); return -1; } + thisStart = mem->target.virtio_pmem.address; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: @@ -2347,6 +2351,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, _("memory device address must be aligned to blocksize")); return -1; } + thisStart = mem->target.virtio_mem.address; break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -2368,6 +2373,48 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, return -1; } + if (thisStart == 0) { + return 0; + } + + /* thisStart and thisEnd are in bytes, mem->size in kibibytes */ + thisEnd = thisStart + mem->size * 1024; + + for (i = 0; i < def->nmems; i++) { + const virDomainMemoryDef *other = def->mems[i]; + unsigned long long otherStart = 0; + + if (other == mem) + continue; + + switch (other->model) { + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + continue; + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: + otherStart = other->target.virtio_pmem.address; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: + otherStart = other->target.virtio_mem.address; + break; + } + + if (otherStart == 0) + continue; + + if (thisStart <= otherStart && thisEnd > otherStart) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device address [0x%1$llx:0x%2$llx] overlaps with other memory device (0x%3$llx)"), + thisStart, thisEnd, otherStart); + return -1; + } + } + return 0; } diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err new file mode 100644 index 0000000000..36d5b8a6e6 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: memory device address [0x140000000:0x180000000] overlaps with other memory device (0x170000000) diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml new file mode 100644 index 0000000000..65999ccd99 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml @@ -0,0 +1,50 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>8388608</memory> + <currentMemory unit='KiB'>8388608</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + <topology sockets='2' dies='1' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='2095104' 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-x86_64</emulator> + <memory model='virtio-mem'> + <target> + <size unit='KiB'>1048576</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>524288</requested> + <address base='0x140000000'/> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memory> + <memory model='virtio-mem'> + <source> + <nodemask>1-3</nodemask> + <pagesize unit='KiB'>2048</pagesize> + </source> + <target> + <size unit='KiB'>2097152</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>1048576</requested> + <address base='0x170000000'/> + </target> + <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2195d8efa3..a454dcb205 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2114,6 +2114,7 @@ mymain(void) DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem"); DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem"); DO_TEST_CAPS_LATEST("memory-hotplug-multiple"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("memory-hotplug-virtio-mem-overlap-address"); DO_TEST_CAPS_ARCH_LATEST("machine-aeskeywrap-on-caps", "s390x"); DO_TEST_CAPS_ARCH_LATEST("machine-aeskeywrap-on-cap", "s390x"); -- 2.41.0
On a Friday in 2023, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (3): domain_validate: Validate VIRTIO_PMEM address alignment virDomainMemoryDefValidate: Fix VIRTIO_MEM alignment check virDomainMemoryDefValidate: Check for overlapping memory devices
src/conf/domain_validate.c | 59 ++++++++++++++++++- ...rtio-mem-overlap-address.x86_64-latest.err | 1 + ...ory-hotplug-virtio-mem-overlap-address.xml | 50 ++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik