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

_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org