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(a)redhat.com&gt; --- 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

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/sy... Reported-by: Jaroslav Suchanek <jsuchane(a)redhat.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

On Mon, Nov 06, 2023 at 12:38:25 +0100, Michal Privoznik wrote: 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.

On Mon, Nov 06, 2023 at 12:38:26 +0100, Michal Privoznik wrote: For the above: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; Here the tests starts to check something else per my previous point.

On Mon, Nov 06, 2023 at 12:38:27 +0100, Michal Privoznik wrote: 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 Thu, Nov 23, 2023 at 16:06:08 +0100, Michal Prívozník wrote: [...] Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;