diff mbox series

[PULL,07/55] hw/virtio: move vm_running check to virtio_device_started

Message ID 20221010172813.204597-8-mst@redhat.com
State New
Headers show
Series [PULL,01/55] hw/virtio: incorporate backend features in features | expand

Commit Message

Michael S. Tsirkin Oct. 10, 2022, 5:29 p.m. UTC 
From: Alex Bennée <alex.bennee@linaro.org>

All the boilerplate virtio code does the same thing (or should at
least) of checking to see if the VM is running before attempting to
start VirtIO. Push the logic up to the common function to avoid
getting a copy and paste wrong.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio.h   | 5 +++++
 hw/virtio/vhost-user-fs.c    | 6 +-----
 hw/virtio/vhost-user-i2c.c   | 6 +-----
 hw/virtio/vhost-user-rng.c   | 6 +-----
 hw/virtio/vhost-user-vsock.c | 6 +-----
 hw/virtio/vhost-vsock.c      | 6 +-----
 6 files changed, 10 insertions(+), 25 deletions(-)

Comments

Christian Borntraeger Oct. 14, 2022, 8:31 a.m. UTC | #1 
Am 14.10.22 um 09:30 schrieb Christian Borntraeger:
> Am 10.10.22 um 19:29 schrieb Michael S. Tsirkin:
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> This results in a regression for our s390x CI when doing save/restore of guests with vsock:
> 
> 
>                  #1  0x000003ff9a248580 raise (libc.so.6 + 0x48580)
>                  #2  0x000003ff9a22b5c0 abort (libc.so.6 + 0x2b5c0)
>                  #3  0x000003ff9a2409da __assert_fail_base (libc.so.6 + 0x409da)
>                  #4  0x000003ff9a240a4e __assert_fail (libc.so.6 + 0x40a4e)
>                  #5  0x000002aa2d69a066 vhost_vsock_common_pre_save (qemu-system-s390x + 0x39a066)
>                  #6  0x000002aa2d55570e vmstate_save_state_v (qemu-system-s390x + 0x25570e)
>                  #7  0x000002aa2d556218 vmstate_save_state (qemu-system-s390x + 0x256218)
>                  #8  0x000002aa2d570ba4 qemu_savevm_state_complete_precopy_non_iterable (qemu-system-s390x + 0x270ba4)
>                  #9  0x000002aa2d5710b6 qemu_savevm_state_complete_precopy (qemu-system-s390x + 0x2710b6)
>                  #10 0x000002aa2d564d0e migration_completion (qemu-system-s390x + 0x264d0e)
>                  #11 0x000002aa2d8db25c qemu_thread_start (qemu-system-s390x + 0x5db25c)
>                  #12 0x000003ff9a296248 start_thread (libc.so.6 + 0x96248)
>                  #13 0x000003ff9a31183e thread_start (libc.so.6 + 0x11183e)
> 


Something like
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 7dc3c7393122..b4d056ae6f01 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -73,6 +73,10 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
      bool should_start = virtio_device_started(vdev, status);
      int ret;
  
+    if (!vdev->vm_running) {
+        should_start = false;
+    }
+
      if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
          return;
      }

helps.

The problem seems to be that virtio_device_started does ignore vm_running when use_start is set.
Alex Bennée Oct. 14, 2022, 8:37 a.m. UTC | #2 
Christian Borntraeger <borntraeger@linux.ibm.com> writes:

> Am 10.10.22 um 19:29 schrieb Michael S. Tsirkin:
>> From: Alex Bennée <alex.bennee@linaro.org>
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> This results in a regression for our s390x CI when doing save/restore of guests with vsock:
>
>
>                 #1  0x000003ff9a248580 raise (libc.so.6 + 0x48580)
>                 #2  0x000003ff9a22b5c0 abort (libc.so.6 + 0x2b5c0)
>                 #3  0x000003ff9a2409da __assert_fail_base (libc.so.6 + 0x409da)
>                 #4  0x000003ff9a240a4e __assert_fail (libc.so.6 + 0x40a4e)
>                 #5  0x000002aa2d69a066 vhost_vsock_common_pre_save (qemu-system-s390x + 0x39a066)
>                 #6  0x000002aa2d55570e vmstate_save_state_v (qemu-system-s390x + 0x25570e)
>                 #7  0x000002aa2d556218 vmstate_save_state (qemu-system-s390x + 0x256218)
>                 #8 0x000002aa2d570ba4
> qemu_savevm_state_complete_precopy_non_iterable (qemu-system-s390x +
> 0x270ba4)
>                 #9  0x000002aa2d5710b6 qemu_savevm_state_complete_precopy (qemu-system-s390x + 0x2710b6)
>                 #10 0x000002aa2d564d0e migration_completion (qemu-system-s390x + 0x264d0e)
>                 #11 0x000002aa2d8db25c qemu_thread_start (qemu-system-s390x + 0x5db25c)
>                 #12 0x000003ff9a296248 start_thread (libc.so.6 + 0x96248)
>                 #13 0x000003ff9a31183e thread_start (libc.so.6 + 0x11183e)

Which test does this break?

Looking at the change the only thing I can think of is there is a subtle
change in the order of checks because if the device is set as
use_started we return the result regardless of vm or config state:

    if (vdev->use_started) {
        return vdev->started;
    }

Could some printfs confirm that?
Christian Borntraeger Oct. 14, 2022, 8:44 a.m. UTC | #3 
Am 14.10.22 um 10:37 schrieb Alex Bennée:
> 
> Christian Borntraeger <borntraeger@linux.ibm.com> writes:
> 
>> Am 10.10.22 um 19:29 schrieb Michael S. Tsirkin:
>>> From: Alex Bennée <alex.bennee@linaro.org>
>>> All the boilerplate virtio code does the same thing (or should at
>>> least) of checking to see if the VM is running before attempting to
>>> start VirtIO. Push the logic up to the common function to avoid
>>> getting a copy and paste wrong.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> This results in a regression for our s390x CI when doing save/restore of guests with vsock:
>>
>>
>>                  #1  0x000003ff9a248580 raise (libc.so.6 + 0x48580)
>>                  #2  0x000003ff9a22b5c0 abort (libc.so.6 + 0x2b5c0)
>>                  #3  0x000003ff9a2409da __assert_fail_base (libc.so.6 + 0x409da)
>>                  #4  0x000003ff9a240a4e __assert_fail (libc.so.6 + 0x40a4e)
>>                  #5  0x000002aa2d69a066 vhost_vsock_common_pre_save (qemu-system-s390x + 0x39a066)
>>                  #6  0x000002aa2d55570e vmstate_save_state_v (qemu-system-s390x + 0x25570e)
>>                  #7  0x000002aa2d556218 vmstate_save_state (qemu-system-s390x + 0x256218)
>>                  #8 0x000002aa2d570ba4
>> qemu_savevm_state_complete_precopy_non_iterable (qemu-system-s390x +
>> 0x270ba4)
>>                  #9  0x000002aa2d5710b6 qemu_savevm_state_complete_precopy (qemu-system-s390x + 0x2710b6)
>>                  #10 0x000002aa2d564d0e migration_completion (qemu-system-s390x + 0x264d0e)
>>                  #11 0x000002aa2d8db25c qemu_thread_start (qemu-system-s390x + 0x5db25c)
>>                  #12 0x000003ff9a296248 start_thread (libc.so.6 + 0x96248)
>>                  #13 0x000003ff9a31183e thread_start (libc.so.6 + 0x11183e)
> 
> Which test does this break?

migrate to file and restore.

> 
> Looking at the change the only thing I can think of is there is a subtle
> change in the order of checks because if the device is set as
> use_started we return the result regardless of vm or config state:
> 
>      if (vdev->use_started) {
>          return vdev->started;
>      }
> 
> Could some printfs confirm that?

Right. The problem is we now ignore the vm state and thus run into the assertion in vhost_vsock_common_pre_save.
Removing the asserting then results in virtio errors, which really indicates that the device must not be started.
Alex Bennée Oct. 14, 2022, 11:07 a.m. UTC | #4 
Christian Borntraeger <borntraeger@linux.ibm.com> writes:

> Am 14.10.22 um 09:30 schrieb Christian Borntraeger:
>> Am 10.10.22 um 19:29 schrieb Michael S. Tsirkin:
>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>
>>> All the boilerplate virtio code does the same thing (or should at
>>> least) of checking to see if the VM is running before attempting to
>>> start VirtIO. Push the logic up to the common function to avoid
>>> getting a copy and paste wrong.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> This results in a regression for our s390x CI when doing
>> save/restore of guests with vsock:
>>                  #1  0x000003ff9a248580 raise (libc.so.6 + 0x48580)
>>                  #2  0x000003ff9a22b5c0 abort (libc.so.6 + 0x2b5c0)
>>                  #3  0x000003ff9a2409da __assert_fail_base (libc.so.6 + 0x409da)
>>                  #4  0x000003ff9a240a4e __assert_fail (libc.so.6 + 0x40a4e)
>>                  #5  0x000002aa2d69a066 vhost_vsock_common_pre_save (qemu-system-s390x + 0x39a066)
>>                  #6  0x000002aa2d55570e vmstate_save_state_v (qemu-system-s390x + 0x25570e)
>>                  #7  0x000002aa2d556218 vmstate_save_state (qemu-system-s390x + 0x256218)
>>                  #8  0x000002aa2d570ba4
>> qemu_savevm_state_complete_precopy_non_iterable (qemu-system-s390x +
>> 0x270ba4)
>>                  #9  0x000002aa2d5710b6 qemu_savevm_state_complete_precopy (qemu-system-s390x + 0x2710b6)
>>                  #10 0x000002aa2d564d0e migration_completion (qemu-system-s390x + 0x264d0e)
>>                  #11 0x000002aa2d8db25c qemu_thread_start (qemu-system-s390x + 0x5db25c)
>>                  #12 0x000003ff9a296248 start_thread (libc.so.6 + 0x96248)
>>                  #13 0x000003ff9a31183e thread_start (libc.so.6 + 0x11183e)
>> 
>
>
> Something like
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 7dc3c7393122..b4d056ae6f01 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -73,6 +73,10 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>      bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  +    if (!vdev->vm_running) {
> +        should_start = false;
> +    }
> +
>      if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>
> helps.
>
> The problem seems to be that virtio_device_started does ignore
> vm_running when use_start is set.

Wouldn't it make more sense to re-order the check there, something like:

  static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
  {
      if (!vdev->vm_running) {
          return false;
      }

      if (vdev->use_started) {
          return vdev->started;
      }

      return status & VIRTIO_CONFIG_S_DRIVER_OK;
  }

Is the problem that vdev->started gets filled during the migration but
because the VM isn't running yet we can never actually run?
Christian Borntraeger Oct. 14, 2022, 11:58 a.m. UTC | #5 
Am 14.10.22 um 13:07 schrieb Alex Bennée:
> 
> Christian Borntraeger <borntraeger@linux.ibm.com> writes:
> 
>> Am 14.10.22 um 09:30 schrieb Christian Borntraeger:
>>> Am 10.10.22 um 19:29 schrieb Michael S. Tsirkin:
>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>> All the boilerplate virtio code does the same thing (or should at
>>>> least) of checking to see if the VM is running before attempting to
>>>> start VirtIO. Push the logic up to the common function to avoid
>>>> getting a copy and paste wrong.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> This results in a regression for our s390x CI when doing
>>> save/restore of guests with vsock:
>>>                   #1  0x000003ff9a248580 raise (libc.so.6 + 0x48580)
>>>                   #2  0x000003ff9a22b5c0 abort (libc.so.6 + 0x2b5c0)
>>>                   #3  0x000003ff9a2409da __assert_fail_base (libc.so.6 + 0x409da)
>>>                   #4  0x000003ff9a240a4e __assert_fail (libc.so.6 + 0x40a4e)
>>>                   #5  0x000002aa2d69a066 vhost_vsock_common_pre_save (qemu-system-s390x + 0x39a066)
>>>                   #6  0x000002aa2d55570e vmstate_save_state_v (qemu-system-s390x + 0x25570e)
>>>                   #7  0x000002aa2d556218 vmstate_save_state (qemu-system-s390x + 0x256218)
>>>                   #8  0x000002aa2d570ba4
>>> qemu_savevm_state_complete_precopy_non_iterable (qemu-system-s390x +
>>> 0x270ba4)
>>>                   #9  0x000002aa2d5710b6 qemu_savevm_state_complete_precopy (qemu-system-s390x + 0x2710b6)
>>>                   #10 0x000002aa2d564d0e migration_completion (qemu-system-s390x + 0x264d0e)
>>>                   #11 0x000002aa2d8db25c qemu_thread_start (qemu-system-s390x + 0x5db25c)
>>>                   #12 0x000003ff9a296248 start_thread (libc.so.6 + 0x96248)
>>>                   #13 0x000003ff9a31183e thread_start (libc.so.6 + 0x11183e)
>>>
>>
>>
>> Something like
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 7dc3c7393122..b4d056ae6f01 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -73,6 +73,10 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>>       bool should_start = virtio_device_started(vdev, status);
>>       int ret;
>>   +    if (!vdev->vm_running) {
>> +        should_start = false;
>> +    }
>> +
>>       if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>>           return;
>>       }
>>
>> helps.
>>
>> The problem seems to be that virtio_device_started does ignore
>> vm_running when use_start is set.
> 
> Wouldn't it make more sense to re-order the check there, something like:
> 
>    static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>    {
>        if (!vdev->vm_running) {
>            return false;
>        }
> 
>        if (vdev->use_started) {
>            return vdev->started;
>        }
> 
>        return status & VIRTIO_CONFIG_S_DRIVER_OK;
>    }

That does work as well. (and it restores the original ordering so that makes sense).

> 
> Is the problem that vdev->started gets filled during the migration but
> because the VM isn't running yet we can never actually run?

I dont know.
Michael S. Tsirkin Nov. 5, 2022, 4:45 p.m. UTC | #6 
On Mon, Oct 10, 2022 at 01:29:10PM -0400, Michael S. Tsirkin wrote:
> From: Alex Bennée <alex.bennee@linaro.org>
> 
> All the boilerplate virtio code does the same thing (or should at
> least) of checking to see if the VM is running before attempting to
> start VirtIO. Push the logic up to the common function to avoid
> getting a copy and paste wrong.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

So, looking at the resulting code, I missed the fact that this function
is also used in virtio core.  So this patch does not do what it's saying
it does (just refactor code).
Instead it completely changes the meaning for virtio core.
I thunk we should revert upstream, however, gpio has grown a
dependency on this since then.
Alex, could you take a look please?

> ---
>  include/hw/virtio/virtio.h   | 5 +++++
>  hw/virtio/vhost-user-fs.c    | 6 +-----
>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>  hw/virtio/vhost-user-rng.c   | 6 +-----
>  hw/virtio/vhost-user-vsock.c | 6 +-----
>  hw/virtio/vhost-vsock.c      | 6 +-----
>  6 files changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9bb2485415..74e7ad5a92 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -100,6 +100,7 @@ struct VirtIODevice
>      VirtQueue *vq;
>      MemoryListener listener;
>      uint16_t device_id;
> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>          return vdev->started;
>      }
>  
> +    if (!vdev->vm_running) {
> +        return false;
> +    }
> +
>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>  }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index e513e4fdda..d2bebba785 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (fs->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 6020eee093..b930cf6d5e 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (i2c->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index 3a7bf8e32d..a9c1c4bc79 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (rng->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 0f8ff99f85..22c1616ebd 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (vvc->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 0338de892f..8031c164a5 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> +    bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> -
>      if (vvc->vhost_dev.started == should_start) {
>          return;
>      }
> -- 
> MST
>
Alex Bennée Nov. 7, 2022, 9:21 a.m. UTC | #7 
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Oct 10, 2022 at 01:29:10PM -0400, Michael S. Tsirkin wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>> 
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> So, looking at the resulting code, I missed the fact that this function
> is also used in virtio core.  So this patch does not do what it's saying
> it does (just refactor code).
> Instead it completely changes the meaning for virtio core.
> I thunk we should revert upstream, however, gpio has grown a
> dependency on this since then.
> Alex, could you take a look please?

So I guess we have three choices:

  new function for use by backends
  new function for use by core
  parameterise virtio_device_started to ignore vm state

I'll add some usage doc comments whichever way.

Do you have a preference?

>
>> ---
>>  include/hw/virtio/virtio.h   | 5 +++++
>>  hw/virtio/vhost-user-fs.c    | 6 +-----
>>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>>  hw/virtio/vhost-user-rng.c   | 6 +-----
>>  hw/virtio/vhost-user-vsock.c | 6 +-----
>>  hw/virtio/vhost-vsock.c      | 6 +-----
>>  6 files changed, 10 insertions(+), 25 deletions(-)
>> 
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 9bb2485415..74e7ad5a92 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -100,6 +100,7 @@ struct VirtIODevice
>>      VirtQueue *vq;
>>      MemoryListener listener;
>>      uint16_t device_id;
>> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>>      bool vm_running;
>>      bool broken; /* device in invalid state, needs reset */
>>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>>          return vdev->started;
>>      }
>>  
>> +    if (!vdev->vm_running) {
>> +        return false;
>> +    }
>> +
>>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>>  }
>>  
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index e513e4fdda..d2bebba785 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (fs->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> index 6020eee093..b930cf6d5e 100644
>> --- a/hw/virtio/vhost-user-i2c.c
>> +++ b/hw/virtio/vhost-user-i2c.c
>> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (i2c->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> index 3a7bf8e32d..a9c1c4bc79 100644
>> --- a/hw/virtio/vhost-user-rng.c
>> +++ b/hw/virtio/vhost-user-rng.c
>> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (rng->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>> index 0f8ff99f85..22c1616ebd 100644
>> --- a/hw/virtio/vhost-user-vsock.c
>> +++ b/hw/virtio/vhost-user-vsock.c
>> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 0338de892f..8031c164a5 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> +    bool should_start = virtio_device_started(vdev, status);
>>      int ret;
>>  
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> -
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>>      }
>> -- 
>> MST
>>
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9bb2485415..74e7ad5a92 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -100,6 +100,7 @@  struct VirtIODevice
     VirtQueue *vq;
     MemoryListener listener;
     uint16_t device_id;
+    /* @vm_running: current VM running state via virtio_vmstate_change() */
     bool vm_running;
     bool broken; /* device in invalid state, needs reset */
     bool use_disabled_flag; /* allow use of 'disable' flag when needed */
@@ -376,6 +377,10 @@  static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
         return vdev->started;
     }
 
+    if (!vdev->vm_running) {
+        return false;
+    }
+
     return status & VIRTIO_CONFIG_S_DRIVER_OK;
 }
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index e513e4fdda..d2bebba785 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -122,11 +122,7 @@  static void vuf_stop(VirtIODevice *vdev)
 static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserFS *fs = VHOST_USER_FS(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (fs->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 6020eee093..b930cf6d5e 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -93,11 +93,7 @@  static void vu_i2c_stop(VirtIODevice *vdev)
 static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (i2c->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index 3a7bf8e32d..a9c1c4bc79 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -90,11 +90,7 @@  static void vu_rng_stop(VirtIODevice *vdev)
 static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (rng->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 0f8ff99f85..22c1616ebd 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -55,11 +55,7 @@  const VhostDevConfigOps vsock_ops = {
 static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (vvc->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 0338de892f..8031c164a5 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -70,13 +70,9 @@  static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
 static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+    bool should_start = virtio_device_started(vdev, status);
     int ret;
 
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
-
     if (vvc->vhost_dev.started == should_start) {
         return;
     }