Re: [PATCH 4/5] virDomainMemoryDefCheckConflict: Validate dimm slot too

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