[PATCH 0/5] conf: Check for dimm/nvimm address conflicts too
*** BLURB HERE *** Michal Prívozník (5): domain_validate: Move memdevice address conflict check into a separate function qemuxml2argvdata: Extend memory-hotplug-virtio-mem-overlap-address.xml virDomainMemoryDefCheckConflict: Check dimm & nvdimm models too virDomainMemoryDefCheckConflict: Validate dimm slot too qemu_domain: Drop qemuCheckMemoryDimmConflict() src/conf/domain_validate.c | 165 +++++++++++------- src/qemu/qemu_domain.c | 37 ---- ...rtio-mem-overlap-address.x86_64-latest.err | 2 +- ...ory-hotplug-virtio-mem-overlap-address.xml | 19 +- 4 files changed, 123 insertions(+), 100 deletions(-) -- 2.41.0
At the end of virDomainMemoryDefValidate() there's a code that checks whether two virtio-mem/virtio-pmem devices don't overlap. Separate this code into its own function (virDomainMemoryDefCheckConflict()). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 147 ++++++++++++++++++++++--------------- 1 file changed, 86 insertions(+), 61 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index bc3d00f89c..b4ffef919a 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2217,15 +2217,96 @@ virDomainHostdevDefValidate(const virDomainHostdevDef *hostdev) } +static int +virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, + const virDomainDef *def) +{ + unsigned long long thisStart = 0; + unsigned long long thisEnd = 0; + size_t i; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: + thisStart = mem->target.virtio_pmem.address; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: + thisStart = mem->target.virtio_mem.address; + break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } + + 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; + + /* In case we're updating an existing memory device (e.g. virtio-mem), + * then pointers will be different. But addresses and aliases are the + * same. However, STREQ_NULLABLE() returns true if both strings are + * NULL which is not what we want. */ + if (virDomainDeviceInfoAddressIsEqual(&other->info, + &mem->info)) { + continue; + } + + if (mem->info.alias && + STREQ_NULLABLE(other->info.alias, + mem->info.alias)) { + 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; +} + + static int virDomainMemoryDefValidate(const virDomainMemoryDef *mem, const virDomainDef *def) { 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) { @@ -2307,7 +2388,6 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, pagesize); return -1; } - thisStart = mem->target.virtio_pmem.address; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: @@ -2351,7 +2431,6 @@ 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: @@ -2373,62 +2452,8 @@ 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; - - /* In case we're updating an existing memory device (e.g. virtio-mem), - * then pointers will be different. But addresses and aliases are the - * same. However, STREQ_NULLABLE() returns true if both strings are - * NULL which is not what we want. */ - if (virDomainDeviceInfoAddressIsEqual(&other->info, - &mem->info)) { - continue; - } - - if (mem->info.alias && - STREQ_NULLABLE(other->info.alias, - mem->info.alias)) { - 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; - } - } + if (virDomainMemoryDefCheckConflict(mem, def) < 0) + return -1; return 0; } -- 2.41.0
On Mon, Nov 06, 2023 at 12:38:23 +0100, Michal Privoznik wrote:
At the end of virDomainMemoryDefValidate() there's a code that checks whether two virtio-mem/virtio-pmem devices don't overlap. Separate this code into its own function (virDomainMemoryDefCheckConflict()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 147 ++++++++++++++++++++++--------------- 1 file changed, 86 insertions(+), 61 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Problem with HW_PHYSMEM sysctl on 64-bit macOS is that it returns a 32-bit signed value. Thus it overflows. Switching to HW_MEMSIZE is recommended as it's of an uint_64 type [1]. 1: https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/bsd/sys/s... Reported-by: Jaroslav Suchanek <jsuchane@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhostmem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index 1da2759ac3..a7027af835 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -617,7 +617,10 @@ virHostMemGetTotal(void) unsigned long long physmem = 0; size_t len = sizeof(physmem); - if (sysctlbyname("hw.physmem", &physmem, &len, NULL, 0) < 0) { + /* On macOS hw.physmem is int32_t which doesn't fly with >4GiB of memory. + * But hw.memsize is uint64_t. */ + if (sysctlbyname("hw.memsize", &physmem, &len, NULL, 0) < 0 && + sysctlbyname("hw.physmem", &physmem, &len, NULL, 0) < 0) { virReportSystemError(errno, "%s", _("Unable to query memory total")); return 0; -- 2.41.0
This test case of qemuxml2argvtest is used to check whether we correctly identify overlapping memory devices. Well, so far we consider just virtio-mem and virtio-pmem devices, but this is about to change and be extended for other models too. Extend the test case now to de-clutter next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- ...ory-hotplug-virtio-mem-overlap-address.xml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml index 65999ccd99..f64931c225 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml @@ -1,7 +1,7 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <maxMemory unit='KiB'>1099511627776</maxMemory> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> <memory unit='KiB'>8388608</memory> <currentMemory unit='KiB'>8388608</currentMemory> <vcpu placement='static' cpuset='0-1'>2</vcpu> @@ -22,6 +22,23 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memory model="dimm"> + <target> + <size unit="KiB">131072</size> + <node>0</node> + </target> + <address type="dimm" slot="0" base="0x170000000"/> + </memory> + <memory model="nvdimm"> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit="KiB">131072</size> + <node>0</node> + </target> + <address type="dimm" slot="1" base="0x140000000"/> + </memory> <memory model='virtio-mem'> <target> <size unit='KiB'>1048576</size> -- 2.41.0
On Mon, Nov 06, 2023 at 12:38:25 +0100, Michal Privoznik wrote:
This test case of qemuxml2argvtest is used to check whether we correctly identify overlapping memory devices. Well, so far we consider just virtio-mem and virtio-pmem devices, but this is about to change and be extended for other models too. Extend the test case now to de-clutter next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- ...ory-hotplug-virtio-mem-overlap-address.xml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml index 65999ccd99..f64931c225 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml @@ -1,7 +1,7 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <maxMemory unit='KiB'>1099511627776</maxMemory> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> <memory unit='KiB'>8388608</memory> <currentMemory unit='KiB'>8388608</currentMemory> <vcpu placement='static' cpuset='0-1'>2</vcpu> @@ -22,6 +22,23 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memory model="dimm"> + <target> + <size unit="KiB">131072</size> + <node>0</node> + </target> + <address type="dimm" slot="0" base="0x170000000"/> + </memory> + <memory model="nvdimm"> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit="KiB">131072</size> + <node>0</node> + </target> + <address type="dimm" slot="1" base="0x140000000"/> + </memory>
To properly test negative cases you need to have multiple test cases for each single sub-case you want to test as there's just one error we can check against. Preferrably add another test case for anything you want to test.
<memory model='virtio-mem'> <target> <size unit='KiB'>1048576</size> -- 2.41.0
_______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
So far we check whether virtio-mem and/or virtio-pmem memory devices do not overlap with each other. But we allow specifying address where dimm and nvdimm memory devices are mapped too. And there are left out from this collision check. Not anymore. This leaves just sgx model out, but that's expected since it can't have any address (see virDomainMemoryDefValidate()). Resolves: https://issues.redhat.com/browse/RHEL-4452 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 12 ++++++++++-- ...plug-virtio-mem-overlap-address.x86_64-latest.err | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index b4ffef919a..5d9602666e 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2234,6 +2234,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + thisStart = mem->info.addr.dimm.base; + } + break; case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: @@ -2271,13 +2275,17 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, 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_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (other->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + otherStart = other->info.addr.dimm.base; + } + break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: otherStart = other->target.virtio_pmem.address; break; 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 index 36d5b8a6e6..6a1ad4556d 100644 --- 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 @@ -1 +1 @@ -unsupported configuration: memory device address [0x140000000:0x180000000] overlaps with other memory device (0x170000000) +unsupported configuration: memory device address [0x170000000:0x178000000] overlaps with other memory device (0x170000000) -- 2.41.0
On Mon, Nov 06, 2023 at 12:38:26 +0100, Michal Privoznik wrote:
So far we check whether virtio-mem and/or virtio-pmem memory devices do not overlap with each other. But we allow specifying address where dimm and nvdimm memory devices are mapped too. And there are left out from this collision check. Not anymore.
This leaves just sgx model out, but that's expected since it can't have any address (see virDomainMemoryDefValidate()).
Resolves: https://issues.redhat.com/browse/RHEL-4452 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 12 ++++++++++-- ...plug-virtio-mem-overlap-address.x86_64-latest.err | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index b4ffef919a..5d9602666e 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2234,6 +2234,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + thisStart = mem->info.addr.dimm.base; + } + break; case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: @@ -2271,13 +2275,17 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem,
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_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (other->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + otherStart = other->info.addr.dimm.base; + } + break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: otherStart = other->target.virtio_pmem.address; break;
For the above: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 index 36d5b8a6e6..6a1ad4556d 100644 --- 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 @@ -1 +1 @@ -unsupported configuration: memory device address [0x140000000:0x180000000] overlaps with other memory device (0x170000000) +unsupported configuration: memory device address [0x170000000:0x178000000] overlaps with other memory device (0x170000000)
Here the tests starts to check something else per my previous point.
Since we're iterating over def->mems array, might as well check for dimm slot duplicates. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 5d9602666e..f45ee0a8a5 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2221,6 +2221,7 @@ static int virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, const virDomainDef *def) { + const virDomainDeviceDimmAddress *thisAddr = NULL; unsigned long long thisStart = 0; unsigned long long thisEnd = 0; size_t i; @@ -2235,6 +2236,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + thisAddr = &mem->info.addr.dimm; thisStart = mem->info.addr.dimm.base; } break; @@ -2244,7 +2246,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; } - if (thisStart == 0) { + if (thisStart == 0 && !thisAddr) { return 0; } @@ -2258,19 +2260,27 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, if (other == mem) continue; - /* In case we're updating an existing memory device (e.g. virtio-mem), - * then pointers will be different. But addresses and aliases are the - * same. However, STREQ_NULLABLE() returns true if both strings are - * NULL which is not what we want. */ - if (virDomainDeviceInfoAddressIsEqual(&other->info, - &mem->info)) { - continue; - } + if (thisAddr && other->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && + thisAddr->slot == other->info.addr.dimm.slot) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%1$u' is already being used by another memory device"), + thisAddr->slot); + return -1; + } else if (!thisAddr) { + /* In case we're updating an existing memory device (e.g. + * virtio-mem), then pointers will be different. But addresses and + * aliases are the same. However, STREQ_NULLABLE() returns true if + * both strings are NULL which is not what we want. */ + if (virDomainDeviceInfoAddressIsEqual(&other->info, + &mem->info)) { + continue; + } - if (mem->info.alias && - STREQ_NULLABLE(other->info.alias, - mem->info.alias)) { - continue; + if (mem->info.alias && + STREQ_NULLABLE(other->info.alias, + mem->info.alias)) { + continue; + } } switch (other->model) { -- 2.41.0
On Mon, Nov 06, 2023 at 12:38:27 +0100, Michal Privoznik wrote:
Since we're iterating over def->mems array, might as well check for dimm slot duplicates.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 5d9602666e..f45ee0a8a5 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2221,6 +2221,7 @@ static int virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, const virDomainDef *def) { + const virDomainDeviceDimmAddress *thisAddr = NULL; unsigned long long thisStart = 0; unsigned long long thisEnd = 0; size_t i; @@ -2235,6 +2236,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + thisAddr = &mem->info.addr.dimm; thisStart = mem->info.addr.dimm.base; } break; @@ -2244,7 +2246,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; }
- if (thisStart == 0) { + if (thisStart == 0 && !thisAddr) { return 0; }
This seems a bit suspicious, because if the start address is 0, where qemu will auto-populate it, the code below will still try to validate it against existing devices. Very theoretically if you are adding a big enough device it can possibly clash with another one that is somewhere further in memory (start address already populated) despite the fact that qemu would put the new device into a reasonable location at the end of the address space. IMO if the start address is 0 we must not attempt to do anything about the address validation and the above condition will make that no longer the case.
On 11/16/23 14:05, Peter Krempa wrote:
On Mon, Nov 06, 2023 at 12:38:27 +0100, Michal Privoznik wrote:
Since we're iterating over def->mems array, might as well check for dimm slot duplicates.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 5d9602666e..f45ee0a8a5 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2221,6 +2221,7 @@ static int virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, const virDomainDef *def) { + const virDomainDeviceDimmAddress *thisAddr = NULL; unsigned long long thisStart = 0; unsigned long long thisEnd = 0; size_t i; @@ -2235,6 +2236,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + thisAddr = &mem->info.addr.dimm; thisStart = mem->info.addr.dimm.base; } break; @@ -2244,7 +2246,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; }
- if (thisStart == 0) { + if (thisStart == 0 && !thisAddr) { return 0; }
This seems a bit suspicious, because if the start address is 0, where qemu will auto-populate it, the code below will still try to validate it against existing devices.
Very theoretically if you are adding a big enough device it can possibly clash with another one that is somewhere further in memory (start address already populated) despite the fact that qemu would put the new device into a reasonable location at the end of the address space.
IMO if the start address is 0 we must not attempt to do anything about the address validation and the above condition will make that no longer the case.
Fair enough. Consider this squashed in: diff --git i/src/conf/domain_validate.c w/src/conf/domain_validate.c index f45ee0a8a5..c72108886e 100644 --- i/src/conf/domain_validate.c +++ w/src/conf/domain_validate.c @@ -2304,7 +2304,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; } - if (otherStart == 0) + if (thisStart == 0 || otherStart == 0) continue; if (thisStart <= otherStart && thisEnd > otherStart) { Or do you want me to send v2? Michal
On Thu, Nov 23, 2023 at 16:06:08 +0100, Michal Prívozník wrote:
On 11/16/23 14:05, Peter Krempa wrote:
On Mon, Nov 06, 2023 at 12:38:27 +0100, Michal Privoznik wrote:
[...]
@@ -2244,7 +2246,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; }
- if (thisStart == 0) { + if (thisStart == 0 && !thisAddr) { return 0; }
This seems a bit suspicious, because if the start address is 0, where qemu will auto-populate it, the code below will still try to validate it against existing devices.
Very theoretically if you are adding a big enough device it can possibly clash with another one that is somewhere further in memory (start address already populated) despite the fact that qemu would put the new device into a reasonable location at the end of the address space.
IMO if the start address is 0 we must not attempt to do anything about the address validation and the above condition will make that no longer the case.
Fair enough. Consider this squashed in:
diff --git i/src/conf/domain_validate.c w/src/conf/domain_validate.c index f45ee0a8a5..c72108886e 100644 --- i/src/conf/domain_validate.c +++ w/src/conf/domain_validate.c @@ -2304,7 +2304,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, break; }
- if (otherStart == 0) + if (thisStart == 0 || otherStart == 0) continue;
if (thisStart <= otherStart && thisEnd > otherStart) {
Or do you want me to send v2?
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virDomainMemoryDefCheckConflict() already does the same set of checks. There's no need to duplicate them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ae19ce884b..413f67577e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9082,39 +9082,6 @@ qemuDomainSupportsPCI(virDomainDef *def, } -static bool -qemuCheckMemoryDimmConflict(const virDomainDef *def, - const virDomainMemoryDef *mem) -{ - size_t i; - - for (i = 0; i < def->nmems; i++) { - virDomainMemoryDef *tmp = def->mems[i]; - - if (tmp == mem || - tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) - continue; - - if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%1$u' is already being used by another memory device"), - mem->info.addr.dimm.slot); - return true; - } - - if (mem->info.addr.dimm.base != 0 && - mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device base '0x%1$llx' is already being used by another memory device"), - mem->info.addr.dimm.base); - return true; - } - } - - return false; -} - - static int qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, const virDomainDef *def) @@ -9138,10 +9105,6 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, mem->info.addr.dimm.slot, def->mem.memory_slots); return -1; } - - - if (qemuCheckMemoryDimmConflict(def, mem)) - return -1; } break; -- 2.41.0
On Mon, Nov 06, 2023 at 12:38:28 +0100, Michal Privoznik wrote:
The virDomainMemoryDefCheckConflict() already does the same set of checks. There's no need to duplicate them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 37 ------------------------------------- 1 file changed, 37 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa