From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

So it can be safety accessed from multiple threads.

This variable type needs to be changed to unsigned long since
32-bit host platforms lack the necessary addition atomics on 64-bit
variables.

Using 32-bit counters on 32-bit host platforms should not be a problem
in practice since they can't realistically address more memory anyway.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 hw/vfio/migration.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 03890eaa48a9..5532787be63b 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -55,7 +55,7 @@
  */
 #define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
 
-static int64_t bytes_transferred;
+static unsigned long bytes_transferred;
 
 static const char *mig_state_to_str(enum vfio_device_mig_state state)
 {
@@ -391,7 +391,7 @@ static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
     qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE);
     qemu_put_be64(f, data_size);
     qemu_put_buffer(f, migration->data_buffer, data_size);
-    bytes_transferred += data_size;
+    qatomic_add(&bytes_transferred, data_size);
 
     trace_vfio_save_block(migration->vbasedev->name, data_size);
 
@@ -1013,12 +1013,12 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
 
 int64_t vfio_mig_bytes_transferred(void)
 {
-    return bytes_transferred;
+    return MIN(qatomic_read(&bytes_transferred), INT64_MAX);
 }
 
 void vfio_reset_bytes_transferred(void)
 {
-    bytes_transferred = 0;
+    qatomic_set(&bytes_transferred, 0);
 }
 
 /*

On 2/19/25 21:34, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> So it can be safety accessed from multiple threads.
> 
> This variable type needs to be changed to unsigned long since
> 32-bit host platforms lack the necessary addition atomics on 64-bit
> variables.
> 
> Using 32-bit counters on 32-bit host platforms should not be a problem
> in practice since they can't realistically address more memory anyway.
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/migration.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 03890eaa48a9..5532787be63b 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -55,7 +55,7 @@
>    */
>   #define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
>   
> -static int64_t bytes_transferred;
> +static unsigned long bytes_transferred;
>   
>   static const char *mig_state_to_str(enum vfio_device_mig_state state)
>   {
> @@ -391,7 +391,7 @@ static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE);
>       qemu_put_be64(f, data_size);
>       qemu_put_buffer(f, migration->data_buffer, data_size);
> -    bytes_transferred += data_size;
> +    qatomic_add(&bytes_transferred, data_size);
>   
>       trace_vfio_save_block(migration->vbasedev->name, data_size);
>   
> @@ -1013,12 +1013,12 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>   
>   int64_t vfio_mig_bytes_transferred(void)
>   {
> -    return bytes_transferred;
> +    return MIN(qatomic_read(&bytes_transferred), INT64_MAX);
>   }
>   
>   void vfio_reset_bytes_transferred(void)
>   {
> -    bytes_transferred = 0;
> +    qatomic_set(&bytes_transferred, 0);
>   }
>   
>   /*
> 

On 2/19/25 21:34, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> So it can be safety accessed from multiple threads.
> 
> This variable type needs to be changed to unsigned long since
> 32-bit host platforms lack the necessary addition atomics on 64-bit
> variables.
> 
> Using 32-bit counters on 32-bit host platforms should not be a problem
> in practice since they can't realistically address more memory anyway.

Is it useful to have VFIO on 32-bit host platforms ?

If not, VFIO PCI should depend on (AARCH64 || PPC64 || X86_64) and we
could drop this patch. Let's address that independently.

Thanks,

C.






> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>   hw/vfio/migration.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 03890eaa48a9..5532787be63b 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -55,7 +55,7 @@
>    */
>   #define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
>   
> -static int64_t bytes_transferred;
> +static unsigned long bytes_transferred;
>   
>   static const char *mig_state_to_str(enum vfio_device_mig_state state)
>   {
> @@ -391,7 +391,7 @@ static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE);
>       qemu_put_be64(f, data_size);
>       qemu_put_buffer(f, migration->data_buffer, data_size);
> -    bytes_transferred += data_size;
> +    qatomic_add(&bytes_transferred, data_size);
>   
>       trace_vfio_save_block(migration->vbasedev->name, data_size);
>   
> @@ -1013,12 +1013,12 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>   
>   int64_t vfio_mig_bytes_transferred(void)
>   {
> -    return bytes_transferred;
> +    return MIN(qatomic_read(&bytes_transferred), INT64_MAX);
>   }
>   
>   void vfio_reset_bytes_transferred(void)
>   {
> -    bytes_transferred = 0;
> +    qatomic_set(&bytes_transferred, 0);
>   }
>   
>   /*
>

On 26.02.2025 08:52, Cédric Le Goater wrote:
> On 2/19/25 21:34, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> So it can be safety accessed from multiple threads.
>>
>> This variable type needs to be changed to unsigned long since
>> 32-bit host platforms lack the necessary addition atomics on 64-bit
>> variables.
>>
>> Using 32-bit counters on 32-bit host platforms should not be a problem
>> in practice since they can't realistically address more memory anyway.
> 
> Is it useful to have VFIO on 32-bit host platforms ?
> 
> If not, VFIO PCI should depend on (AARCH64 || PPC64 || X86_64) and we
> could drop this patch. Let's address that independently.

Not sure how much use VFIO gets on 32-bit host platforms,
however totally disabling it on these would be a major functional regression -
at least if taken at its face value.

Especially considering that making it work on 32-bit platform requires
just this tiny variable type change here.

> Thanks,
> 
> C.

Thanks,
Maciej

On 2/26/25 14:55, Maciej S. Szmigiero wrote:
> On 26.02.2025 08:52, Cédric Le Goater wrote:
>> On 2/19/25 21:34, Maciej S. Szmigiero wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> So it can be safety accessed from multiple threads.
>>>
>>> This variable type needs to be changed to unsigned long since
>>> 32-bit host platforms lack the necessary addition atomics on 64-bit
>>> variables.
>>>
>>> Using 32-bit counters on 32-bit host platforms should not be a problem
>>> in practice since they can't realistically address more memory anyway.
>>
>> Is it useful to have VFIO on 32-bit host platforms ?
>>
>> If not, VFIO PCI should depend on (AARCH64 || PPC64 || X86_64) and we
>> could drop this patch. Let's address that independently.
> 
> Not sure how much use VFIO gets on 32-bit host platforms,
> however totally disabling it on these would be a major functional regression -
> at least if taken at its face value.

32-bit host platform support is being deprecated in QEMU 10.0 and should
be removed in QEMU 10.2.

> Especially considering that making it work on 32-bit platform requires
> just this tiny variable type change here.

yes. It raised my attention because x86 32-bit was the only host platform
I was not sure about and Alex confirmed it worked. We should simply wait
for removal.


Thanks,

C.