* [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started [not found] <20220802095010.3330793-1-alex.bennee@linaro.org> @ 2022-08-02 9:49 ` Alex Bennée 2022-11-03 16:39 ` Michael S. Tsirkin 2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME Alex Bennée 1 sibling, 1 reply; 9+ messages in thread From: Alex Bennée @ 2022-08-02 9:49 UTC (permalink / raw) To: qemu-devel Cc: slp, mst, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Alex Bennée, Dr. David Alan Gilbert, open list:virtiofs 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> --- 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; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started 2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started Alex Bennée @ 2022-11-03 16:39 ` Michael S. Tsirkin 2022-11-03 19:18 ` Alex Bennée 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2022-11-03 16:39 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Dr. David Alan Gilbert, open list:virtiofs On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote: > 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> How bad is it if we drop this? > --- > 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; > } > -- > 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started 2022-11-03 16:39 ` Michael S. Tsirkin @ 2022-11-03 19:18 ` Alex Bennée 2022-11-03 20:30 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Alex Bennée @ 2022-11-03 19:18 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Dr. David Alan Gilbert, open list:virtiofs "Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote: >> 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> > > How bad is it if we drop this? I assume it will break gpio. Why do we want to drop this now? It has been merged awhile. However there was a follow-up patch which tweaked the order of checks in virtio_device_started. > >> --- >> 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; >> } >> -- >> 2.30.2 -- Alex Bennée ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started 2022-11-03 19:18 ` Alex Bennée @ 2022-11-03 20:30 ` Michael S. Tsirkin 2022-11-03 21:35 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2022-11-03 20:30 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Dr. David Alan Gilbert, open list:virtiofs On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote: > >> 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> > > > > How bad is it if we drop this? > > > I assume it will break gpio. Why do we want to drop this now? It has > been merged awhile. However there was a follow-up patch which tweaked > the order of checks in virtio_device_started. And that follow up patch trips up UBSAN: https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327 > > > >> --- > >> 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; > >> } > >> -- > >> 2.30.2 > > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started 2022-11-03 20:30 ` Michael S. Tsirkin @ 2022-11-03 21:35 ` Michael S. Tsirkin 2022-11-04 7:18 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2022-11-03 21:35 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Dr. David Alan Gilbert, open list:virtiofs On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote: > On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote: > > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote: > > >> 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> > > > > > > How bad is it if we drop this? > > > > > > I assume it will break gpio. Why do we want to drop this now? It has > > been merged awhile. However there was a follow-up patch which tweaked > > the order of checks in virtio_device_started. > > And that follow up patch trips up UBSAN: > > https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327 > And more specifically this https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848 > > > > > > > >> --- > > >> 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; > > >> } > > >> -- > > >> 2.30.2 > > > > > > -- > > Alex Bennée ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started 2022-11-03 21:35 ` Michael S. Tsirkin @ 2022-11-04 7:18 ` Michael S. Tsirkin 0 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2022-11-04 7:18 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Dr. David Alan Gilbert, open list:virtiofs On Thu, Nov 03, 2022 at 05:35:57PM -0400, Michael S. Tsirkin wrote: > On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote: > > On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote: > > > > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote: > > > >> 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> > > > > > > > > How bad is it if we drop this? > > > > > > > > > I assume it will break gpio. Why do we want to drop this now? It has > > > been merged awhile. However there was a follow-up patch which tweaked > > > the order of checks in virtio_device_started. > > > > And that follow up patch trips up UBSAN: > > > > https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327 > > > > > And more specifically this > > https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848 I'm sorry, I misread the results. Triggers without this patch too so it's not to blame. Pls ignore. > > > > > > > > > > > >> --- > > > >> 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; > > > >> } > > > >> -- > > > >> 2.30.2 > > > > > > > > > -- > > > Alex Bennée ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME [not found] <20220802095010.3330793-1-alex.bennee@linaro.org> 2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started Alex Bennée @ 2022-08-02 9:49 ` Alex Bennée 2022-08-07 20:13 ` Raphael Norwitz 2022-11-03 16:39 ` Michael S. Tsirkin 1 sibling, 2 replies; 9+ messages in thread From: Alex Bennée @ 2022-08-02 9:49 UTC (permalink / raw) To: qemu-devel Cc: slp, mst, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Alex Bennée, Raphael Norwitz, Kevin Wolf, Hanna Reitz, Paolo Bonzini, Fam Zheng, Dr. David Alan Gilbert, open list:Block layer core, open list:virtiofs The `started` field is manipulated internally within the vhost code except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck dev state in the vhost_migration_log routine). Mark that as a FIXME because it introduces a potential race. I think the referenced fix should be tracking its state locally. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/hw/virtio/vhost.h | 12 ++++++++++++ hw/block/vhost-user-blk.c | 10 ++++++++-- hw/scsi/vhost-scsi.c | 4 ++-- hw/scsi/vhost-user-scsi.c | 2 +- hw/virtio/vhost-user-fs.c | 3 ++- hw/virtio/vhost-user-i2c.c | 4 ++-- hw/virtio/vhost-user-rng.c | 4 ++-- hw/virtio/vhost-user-vsock.c | 2 +- hw/virtio/vhost-vsock-common.c | 3 ++- hw/virtio/vhost-vsock.c | 2 +- 10 files changed, 33 insertions(+), 13 deletions(-) diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 586c5457e2..61b957e927 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -94,6 +94,7 @@ struct vhost_dev { uint64_t protocol_features; uint64_t max_queues; uint64_t backend_cap; + /* @started: is the vhost device started? */ bool started; bool log_enabled; uint64_t log_size; @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); */ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); +/** + * vhost_dev_is_started() - report status of vhost device + * @hdev: common vhost_dev structure + * + * Return the started status of the vhost device + */ +static inline bool vhost_dev_is_started(struct vhost_dev *hdev) +{ + return hdev->started; +} + /** * vhost_dev_start() - start the vhost device * @hdev: common vhost_dev structure diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 9117222456..2bba42478d 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) return; } - if (s->dev.started == should_start) { + if (vhost_dev_is_started(&s->dev) == should_start) { return; } @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) return; } - if (s->dev.started) { + if (vhost_dev_is_started(&s->dev)) { return; } @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) * the vhost migration code. If disconnect was caught there is an * option for the general vhost code to get the dev state without * knowing its type (in this case vhost-user). + * + * FIXME: this is sketchy to be reaching into vhost_dev + * now because we are forcing something that implies we + * have executed vhost_dev_stop() but that won't happen + * until vhost_user_blk_stop() gets called from the bh. + * Really this state check should be tracked locally. */ s->dev.started = false; } diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 3059068175..bdf337a7a2 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val) start = false; } - if (vsc->dev.started == start) { + if (vhost_dev_is_started(&vsc->dev) == start) { return; } @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque) /* At this point, backend must be stopped, otherwise * it might keep writing to memory. */ - assert(!vsc->dev.started); + assert(!vhost_dev_is_started(&vsc->dev)); return 0; } diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index 1b2f7eed98..bc37317d55 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running; - if (vsc->dev.started == start) { + if (vhost_dev_is_started(&vsc->dev) == start) { return; } diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index d2bebba785..ad0f91c607 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -20,6 +20,7 @@ #include "hw/virtio/virtio-bus.h" #include "hw/virtio/virtio-access.h" #include "qemu/error-report.h" +#include "hw/virtio/vhost.h" #include "hw/virtio/vhost-user-fs.h" #include "monitor/monitor.h" #include "sysemu/sysemu.h" @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status) VHostUserFS *fs = VHOST_USER_FS(vdev); bool should_start = virtio_device_started(vdev, status); - if (fs->vhost_dev.started == should_start) { + if (vhost_dev_is_started(&fs->vhost_dev) == should_start) { return; } diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c index b930cf6d5e..bc58b6c0d1 100644 --- a/hw/virtio/vhost-user-i2c.c +++ b/hw/virtio/vhost-user-i2c.c @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status) VHostUserI2C *i2c = VHOST_USER_I2C(vdev); bool should_start = virtio_device_started(vdev, status); - if (i2c->vhost_dev.started == should_start) { + if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) { return; } @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev) } i2c->connected = false; - if (i2c->vhost_dev.started) { + if (vhost_dev_is_started(&i2c->vhost_dev)) { vu_i2c_stop(vdev); } } diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c index a9c1c4bc79..bc1f36c5ac 100644 --- a/hw/virtio/vhost-user-rng.c +++ b/hw/virtio/vhost-user-rng.c @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status) VHostUserRNG *rng = VHOST_USER_RNG(vdev); bool should_start = virtio_device_started(vdev, status); - if (rng->vhost_dev.started == should_start) { + if (vhost_dev_is_started(&rng->vhost_dev) == should_start) { return; } @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev) rng->connected = false; - if (rng->vhost_dev.started) { + if (vhost_dev_is_started(&rng->vhost_dev)) { vu_rng_stop(vdev); } } diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c index 22c1616ebd..7b67e29d83 100644 --- a/hw/virtio/vhost-user-vsock.c +++ b/hw/virtio/vhost-user-vsock.c @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status) VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev); bool should_start = virtio_device_started(vdev, status); - if (vvc->vhost_dev.started == should_start) { + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { return; } diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c index 7394818e00..29b9ab4f72 100644 --- a/hw/virtio/vhost-vsock-common.c +++ b/hw/virtio/vhost-vsock-common.c @@ -14,6 +14,7 @@ #include "hw/virtio/virtio-access.h" #include "qemu/error-report.h" #include "hw/qdev-properties.h" +#include "hw/virtio/vhost.h" #include "hw/virtio/vhost-vsock.h" #include "qemu/iov.h" #include "monitor/monitor.h" @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque) * At this point, backend must be stopped, otherwise * it might keep writing to memory. */ - assert(!vvc->vhost_dev.started); + assert(!vhost_dev_is_started(&vvc->vhost_dev)); return 0; } diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c index 8031c164a5..7dc3c73931 100644 --- a/hw/virtio/vhost-vsock.c +++ b/hw/virtio/vhost-vsock.c @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status) bool should_start = virtio_device_started(vdev, status); int ret; - if (vvc->vhost_dev.started == should_start) { + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { return; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME 2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME Alex Bennée @ 2022-08-07 20:13 ` Raphael Norwitz 2022-11-03 16:39 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Raphael Norwitz @ 2022-08-07 20:13 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel@nongnu.org, slp@redhat.com, mst@redhat.com, marcandre.lureau@redhat.com, stefanha@redhat.com, mathieu.poirier@linaro.org, viresh.kumar@linaro.org, Raphael Norwitz, Kevin Wolf, Hanna Reitz, Paolo Bonzini, Fam Zheng, Dr. David Alan Gilbert, open list:Block layer core, open list:virtiofs On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote: > The `started` field is manipulated internally within the vhost code > except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck > dev state in the vhost_migration_log routine). Mark that as a FIXME > because it introduces a potential race. I think the referenced fix > should be tracking its state locally. I don't think we can track the state locally. As described in the commit message for f5b22d06fb, the state is used by vhost code in the vhost_migration_log() function so we probably need something at the vhost level. I do agree we shouldn't re-use vdev->started. Maybe we should add another 'active' variable in vhost_dev? I'm happy to send a patch for that. Until we agree on a better solution I'm happy with the FIXME. Reviewed-by: Raphael Norwitz <raphael.norwittz@nutanix.com> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/virtio/vhost.h | 12 ++++++++++++ > hw/block/vhost-user-blk.c | 10 ++++++++-- > hw/scsi/vhost-scsi.c | 4 ++-- > hw/scsi/vhost-user-scsi.c | 2 +- > hw/virtio/vhost-user-fs.c | 3 ++- > hw/virtio/vhost-user-i2c.c | 4 ++-- > hw/virtio/vhost-user-rng.c | 4 ++-- > hw/virtio/vhost-user-vsock.c | 2 +- > hw/virtio/vhost-vsock-common.c | 3 ++- > hw/virtio/vhost-vsock.c | 2 +- > 10 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > index 586c5457e2..61b957e927 100644 > --- a/include/hw/virtio/vhost.h > +++ b/include/hw/virtio/vhost.h > @@ -94,6 +94,7 @@ struct vhost_dev { > uint64_t protocol_features; > uint64_t max_queues; > uint64_t backend_cap; > + /* @started: is the vhost device started? */ > bool started; > bool log_enabled; > uint64_t log_size; > @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); > */ > void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); > > +/** > + * vhost_dev_is_started() - report status of vhost device > + * @hdev: common vhost_dev structure > + * > + * Return the started status of the vhost device > + */ > +static inline bool vhost_dev_is_started(struct vhost_dev *hdev) > +{ > + return hdev->started; > +} > + > /** > * vhost_dev_start() - start the vhost device > * @hdev: common vhost_dev structure > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index 9117222456..2bba42478d 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) > return; > } > > - if (s->dev.started == should_start) { > + if (vhost_dev_is_started(&s->dev) == should_start) { > return; > } > > @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) > return; > } > > - if (s->dev.started) { > + if (vhost_dev_is_started(&s->dev)) { > return; > } > > @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) > * the vhost migration code. If disconnect was caught there is an > * option for the general vhost code to get the dev state without > * knowing its type (in this case vhost-user). > + * > + * FIXME: this is sketchy to be reaching into vhost_dev > + * now because we are forcing something that implies we > + * have executed vhost_dev_stop() but that won't happen > + * until vhost_user_blk_stop() gets called from the bh. > + * Really this state check should be tracked locally. > */ > s->dev.started = false; > } > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 3059068175..bdf337a7a2 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val) > start = false; > } > > - if (vsc->dev.started == start) { > + if (vhost_dev_is_started(&vsc->dev) == start) { > return; > } > > @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque) > > /* At this point, backend must be stopped, otherwise > * it might keep writing to memory. */ > - assert(!vsc->dev.started); > + assert(!vhost_dev_is_started(&vsc->dev)); > > return 0; > } > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > index 1b2f7eed98..bc37317d55 100644 > --- a/hw/scsi/vhost-user-scsi.c > +++ b/hw/scsi/vhost-user-scsi.c > @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running; > > - if (vsc->dev.started == start) { > + if (vhost_dev_is_started(&vsc->dev) == start) { > return; > } > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c > index d2bebba785..ad0f91c607 100644 > --- a/hw/virtio/vhost-user-fs.c > +++ b/hw/virtio/vhost-user-fs.c > @@ -20,6 +20,7 @@ > #include "hw/virtio/virtio-bus.h" > #include "hw/virtio/virtio-access.h" > #include "qemu/error-report.h" > +#include "hw/virtio/vhost.h" > #include "hw/virtio/vhost-user-fs.h" > #include "monitor/monitor.h" > #include "sysemu/sysemu.h" > @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserFS *fs = VHOST_USER_FS(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (fs->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&fs->vhost_dev) == should_start) { > return; > } > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c > index b930cf6d5e..bc58b6c0d1 100644 > --- a/hw/virtio/vhost-user-i2c.c > +++ b/hw/virtio/vhost-user-i2c.c > @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserI2C *i2c = VHOST_USER_I2C(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (i2c->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) { > return; > } > > @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev) > } > i2c->connected = false; > > - if (i2c->vhost_dev.started) { > + if (vhost_dev_is_started(&i2c->vhost_dev)) { > vu_i2c_stop(vdev); > } > } > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c > index a9c1c4bc79..bc1f36c5ac 100644 > --- a/hw/virtio/vhost-user-rng.c > +++ b/hw/virtio/vhost-user-rng.c > @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserRNG *rng = VHOST_USER_RNG(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (rng->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&rng->vhost_dev) == should_start) { > return; > } > > @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev) > > rng->connected = false; > > - if (rng->vhost_dev.started) { > + if (vhost_dev_is_started(&rng->vhost_dev)) { > vu_rng_stop(vdev); > } > } > diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c > index 22c1616ebd..7b67e29d83 100644 > --- a/hw/virtio/vhost-user-vsock.c > +++ b/hw/virtio/vhost-user-vsock.c > @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status) > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (vvc->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { > return; > } > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c > index 7394818e00..29b9ab4f72 100644 > --- a/hw/virtio/vhost-vsock-common.c > +++ b/hw/virtio/vhost-vsock-common.c > @@ -14,6 +14,7 @@ > #include "hw/virtio/virtio-access.h" > #include "qemu/error-report.h" > #include "hw/qdev-properties.h" > +#include "hw/virtio/vhost.h" > #include "hw/virtio/vhost-vsock.h" > #include "qemu/iov.h" > #include "monitor/monitor.h" > @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque) > * At this point, backend must be stopped, otherwise > * it might keep writing to memory. > */ > - assert(!vvc->vhost_dev.started); > + assert(!vhost_dev_is_started(&vvc->vhost_dev)); > > return 0; > } > diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c > index 8031c164a5..7dc3c73931 100644 > --- a/hw/virtio/vhost-vsock.c > +++ b/hw/virtio/vhost-vsock.c > @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status) > bool should_start = virtio_device_started(vdev, status); > int ret; > > - if (vvc->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { > return; > } > > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME 2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME Alex Bennée 2022-08-07 20:13 ` Raphael Norwitz @ 2022-11-03 16:39 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2022-11-03 16:39 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, slp, marcandre.lureau, stefanha, mathieu.poirier, viresh.kumar, Raphael Norwitz, Kevin Wolf, Hanna Reitz, Paolo Bonzini, Fam Zheng, Dr. David Alan Gilbert, open list:Block layer core, open list:virtiofs On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote: > The `started` field is manipulated internally within the vhost code > except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck > dev state in the vhost_migration_log routine). Mark that as a FIXME > because it introduces a potential race. I think the referenced fix > should be tracking its state locally. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> And I guess this for good measure. > --- > include/hw/virtio/vhost.h | 12 ++++++++++++ > hw/block/vhost-user-blk.c | 10 ++++++++-- > hw/scsi/vhost-scsi.c | 4 ++-- > hw/scsi/vhost-user-scsi.c | 2 +- > hw/virtio/vhost-user-fs.c | 3 ++- > hw/virtio/vhost-user-i2c.c | 4 ++-- > hw/virtio/vhost-user-rng.c | 4 ++-- > hw/virtio/vhost-user-vsock.c | 2 +- > hw/virtio/vhost-vsock-common.c | 3 ++- > hw/virtio/vhost-vsock.c | 2 +- > 10 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > index 586c5457e2..61b957e927 100644 > --- a/include/hw/virtio/vhost.h > +++ b/include/hw/virtio/vhost.h > @@ -94,6 +94,7 @@ struct vhost_dev { > uint64_t protocol_features; > uint64_t max_queues; > uint64_t backend_cap; > + /* @started: is the vhost device started? */ > bool started; > bool log_enabled; > uint64_t log_size; > @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); > */ > void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev); > > +/** > + * vhost_dev_is_started() - report status of vhost device > + * @hdev: common vhost_dev structure > + * > + * Return the started status of the vhost device > + */ > +static inline bool vhost_dev_is_started(struct vhost_dev *hdev) > +{ > + return hdev->started; > +} > + > /** > * vhost_dev_start() - start the vhost device > * @hdev: common vhost_dev structure > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index 9117222456..2bba42478d 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) > return; > } > > - if (s->dev.started == should_start) { > + if (vhost_dev_is_started(&s->dev) == should_start) { > return; > } > > @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) > return; > } > > - if (s->dev.started) { > + if (vhost_dev_is_started(&s->dev)) { > return; > } > > @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) > * the vhost migration code. If disconnect was caught there is an > * option for the general vhost code to get the dev state without > * knowing its type (in this case vhost-user). > + * > + * FIXME: this is sketchy to be reaching into vhost_dev > + * now because we are forcing something that implies we > + * have executed vhost_dev_stop() but that won't happen > + * until vhost_user_blk_stop() gets called from the bh. > + * Really this state check should be tracked locally. > */ > s->dev.started = false; > } > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 3059068175..bdf337a7a2 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val) > start = false; > } > > - if (vsc->dev.started == start) { > + if (vhost_dev_is_started(&vsc->dev) == start) { > return; > } > > @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque) > > /* At this point, backend must be stopped, otherwise > * it might keep writing to memory. */ > - assert(!vsc->dev.started); > + assert(!vhost_dev_is_started(&vsc->dev)); > > return 0; > } > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > index 1b2f7eed98..bc37317d55 100644 > --- a/hw/scsi/vhost-user-scsi.c > +++ b/hw/scsi/vhost-user-scsi.c > @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running; > > - if (vsc->dev.started == start) { > + if (vhost_dev_is_started(&vsc->dev) == start) { > return; > } > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c > index d2bebba785..ad0f91c607 100644 > --- a/hw/virtio/vhost-user-fs.c > +++ b/hw/virtio/vhost-user-fs.c > @@ -20,6 +20,7 @@ > #include "hw/virtio/virtio-bus.h" > #include "hw/virtio/virtio-access.h" > #include "qemu/error-report.h" > +#include "hw/virtio/vhost.h" > #include "hw/virtio/vhost-user-fs.h" > #include "monitor/monitor.h" > #include "sysemu/sysemu.h" > @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserFS *fs = VHOST_USER_FS(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (fs->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&fs->vhost_dev) == should_start) { > return; > } > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c > index b930cf6d5e..bc58b6c0d1 100644 > --- a/hw/virtio/vhost-user-i2c.c > +++ b/hw/virtio/vhost-user-i2c.c > @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserI2C *i2c = VHOST_USER_I2C(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (i2c->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) { > return; > } > > @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev) > } > i2c->connected = false; > > - if (i2c->vhost_dev.started) { > + if (vhost_dev_is_started(&i2c->vhost_dev)) { > vu_i2c_stop(vdev); > } > } > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c > index a9c1c4bc79..bc1f36c5ac 100644 > --- a/hw/virtio/vhost-user-rng.c > +++ b/hw/virtio/vhost-user-rng.c > @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status) > VHostUserRNG *rng = VHOST_USER_RNG(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (rng->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&rng->vhost_dev) == should_start) { > return; > } > > @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev) > > rng->connected = false; > > - if (rng->vhost_dev.started) { > + if (vhost_dev_is_started(&rng->vhost_dev)) { > vu_rng_stop(vdev); > } > } > diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c > index 22c1616ebd..7b67e29d83 100644 > --- a/hw/virtio/vhost-user-vsock.c > +++ b/hw/virtio/vhost-user-vsock.c > @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status) > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev); > bool should_start = virtio_device_started(vdev, status); > > - if (vvc->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { > return; > } > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c > index 7394818e00..29b9ab4f72 100644 > --- a/hw/virtio/vhost-vsock-common.c > +++ b/hw/virtio/vhost-vsock-common.c > @@ -14,6 +14,7 @@ > #include "hw/virtio/virtio-access.h" > #include "qemu/error-report.h" > #include "hw/qdev-properties.h" > +#include "hw/virtio/vhost.h" > #include "hw/virtio/vhost-vsock.h" > #include "qemu/iov.h" > #include "monitor/monitor.h" > @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque) > * At this point, backend must be stopped, otherwise > * it might keep writing to memory. > */ > - assert(!vvc->vhost_dev.started); > + assert(!vhost_dev_is_started(&vvc->vhost_dev)); > > return 0; > } > diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c > index 8031c164a5..7dc3c73931 100644 > --- a/hw/virtio/vhost-vsock.c > +++ b/hw/virtio/vhost-vsock.c > @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status) > bool should_start = virtio_device_started(vdev, status); > int ret; > > - if (vvc->vhost_dev.started == should_start) { > + if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) { > return; > } > > -- > 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-04 7:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220802095010.3330793-1-alex.bennee@linaro.org>
2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started Alex Bennée
2022-11-03 16:39 ` Michael S. Tsirkin
2022-11-03 19:18 ` Alex Bennée
2022-11-03 20:30 ` Michael S. Tsirkin
2022-11-03 21:35 ` Michael S. Tsirkin
2022-11-04 7:18 ` Michael S. Tsirkin
2022-08-02 9:49 ` [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME Alex Bennée
2022-08-07 20:13 ` Raphael Norwitz
2022-11-03 16:39 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox