Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device
On Mon, May 11, 2020 at 11:03:01AM +0800, Jason Wang wrote:
>
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Introduce new wrappers to set/reset guest notifiers for the virtio
> >device in the vhost device module:
> > vhost_dev_assign_guest_notifiers
> > ->set_guest_notifiers(..., ..., true);
> > vhost_dev_drop_guest_notifiers
> > ->set_guest_notifiers(..., ..., false);
> >This is a preliminary step to refactor code,
>
>
> Maybe I miss something, I don't see any add-on patch to modify the new
> wrapper in this series?
Hi, in fact the next 3/5 patch:
"[PATCH v2 3/5] vhost-user-blk: add mechanism to track the guest
notifiers init state"
is about using these wrappers. But disregard it, i decided to follow
Raphael suggestion. So we will fix the vhost-user-blk case first, so i
will not introduce these wrappers. And the code will be more easier to
read and straightforward.
I will send v3 as soon as we decide what to do with the migration fix
in this patchset.
No other comments mixed in below.
>
>
> > so the set_guest_notifiers
> >methods could be called based on the vhost device state.
> >Update all vhost used devices to use these wrappers instead of direct
> >method call.
> >
> >Signed-off-by: Dima Stepanov
> >---
> > backends/cryptodev-vhost.c | 26 +++---
> > backends/vhost-user.c | 16 +---
> > hw/block/vhost-user-blk.c | 15 +--
> > hw/net/vhost_net.c | 30 +-
> > hw/scsi/vhost-scsi-common.c | 15 +--
> > hw/virtio/vhost-user-fs.c | 17 +++--
> > hw/virtio/vhost-vsock.c | 18 --
> > hw/virtio/vhost.c | 38 ++
> > hw/virtio/virtio.c | 13 +
> > include/hw/virtio/vhost.h | 4
> > include/hw/virtio/virtio.h | 1 +
> > 11 files changed, 118 insertions(+), 75 deletions(-)
> >
> >diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> >index 8337c9a..4522195 100644
> >--- a/backends/cryptodev-vhost.c
> >+++ b/backends/cryptodev-vhost.c
> >@@ -169,16 +169,13 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
> > int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
> > {
> > VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> >-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >-VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> > int r, e;
> > int i;
> > CryptoDevBackend *b = vcrypto->cryptodev;
> > CryptoDevBackendVhost *vhost_crypto;
> > CryptoDevBackendClient *cc;
> >-if (!k->set_guest_notifiers) {
> >+if (!virtio_device_guest_notifiers_initialized(dev)) {
> > error_report("binding does not support guest notifiers");
> > return -ENOSYS;
> > }
> >@@ -198,9 +195,13 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
> >total_queues)
> > }
> > }
> >-r = k->set_guest_notifiers(qbus->parent, total_queues, true);
> >+/*
> >+ * Since all the states are handled by one vhost device,
> >+ * use the first one in array.
> >+ */
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+r = vhost_dev_assign_guest_notifiers(_crypto->dev, dev,
> >total_queues);
> > if (r < 0) {
> >-error_report("error binding guest notifier: %d", -r);
> > goto err;
> > }
> >@@ -232,7 +233,8 @@ err_start:
> > vhost_crypto = cryptodev_get_vhost(cc, b, i);
> > cryptodev_vhost_stop_one(vhost_crypto, dev);
> > }
> >-e = k->set_guest_notifiers(qbus->parent, total_queues, false);
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+e = vhost_dev_drop_guest_notifiers(_crypto->dev, dev,
> >total_queues);
> > if (e < 0) {
> > error_report("vhost guest notifier cleanup failed: %d", e);
> > }
> >@@ -242,9 +244,6 @@ err:
> > void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
> > {
> >-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >-VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> > VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> > CryptoDevBackend *b = vcrypto->cryptodev;
> > CryptoDevBackendVhost *vhost_crypto;
> >@@ -259,7 +258,12 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
> >total_queues)
> > cryptodev_vhost_stop_one(vhost_crypto, dev);
> > }
> >-r = k->set_guest_notifiers(qbus->parent, total_queues, false);
> >+/*
> >+ * Since all the states are handled by one vhost device,
> >+ * use the first one in array.
> >+ */
> >+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
> >+r = vhost_dev_drop_guest_notifiers(_crypto->dev, dev,
> >total_queues);
> > if (r < 0) {
> > error_report("vhost guest notifier cleanup failed: %d", r);
> > }
> >diff
Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device
On 2020/4/30 下午9:36, Dima Stepanov wrote:
Introduce new wrappers to set/reset guest notifiers for the virtio
device in the vhost device module:
vhost_dev_assign_guest_notifiers
->set_guest_notifiers(..., ..., true);
vhost_dev_drop_guest_notifiers
->set_guest_notifiers(..., ..., false);
This is a preliminary step to refactor code,
Maybe I miss something, I don't see any add-on patch to modify the new
wrapper in this series?
so the set_guest_notifiers
methods could be called based on the vhost device state.
Update all vhost used devices to use these wrappers instead of direct
method call.
Signed-off-by: Dima Stepanov
---
backends/cryptodev-vhost.c | 26 +++---
backends/vhost-user.c | 16 +---
hw/block/vhost-user-blk.c | 15 +--
hw/net/vhost_net.c | 30 +-
hw/scsi/vhost-scsi-common.c | 15 +--
hw/virtio/vhost-user-fs.c | 17 +++--
hw/virtio/vhost-vsock.c | 18 --
hw/virtio/vhost.c | 38 ++
hw/virtio/virtio.c | 13 +
include/hw/virtio/vhost.h | 4
include/hw/virtio/virtio.h | 1 +
11 files changed, 118 insertions(+), 75 deletions(-)
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a..4522195 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -169,16 +169,13 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
{
VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
int r, e;
int i;
CryptoDevBackend *b = vcrypto->cryptodev;
CryptoDevBackendVhost *vhost_crypto;
CryptoDevBackendClient *cc;
-if (!k->set_guest_notifiers) {
+if (!virtio_device_guest_notifiers_initialized(dev)) {
error_report("binding does not support guest notifiers");
return -ENOSYS;
}
@@ -198,9 +195,13 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
total_queues)
}
}
-r = k->set_guest_notifiers(qbus->parent, total_queues, true);
+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_assign_guest_notifiers(_crypto->dev, dev,
total_queues);
if (r < 0) {
-error_report("error binding guest notifier: %d", -r);
goto err;
}
@@ -232,7 +233,8 @@ err_start:
vhost_crypto = cryptodev_get_vhost(cc, b, i);
cryptodev_vhost_stop_one(vhost_crypto, dev);
}
-e = k->set_guest_notifiers(qbus->parent, total_queues, false);
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+e = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
if (e < 0) {
error_report("vhost guest notifier cleanup failed: %d", e);
}
@@ -242,9 +244,6 @@ err:
void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
{
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
CryptoDevBackend *b = vcrypto->cryptodev;
CryptoDevBackendVhost *vhost_crypto;
@@ -259,7 +258,12 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
total_queues)
cryptodev_vhost_stop_one(vhost_crypto, dev);
}
-r = k->set_guest_notifiers(qbus->parent, total_queues, false);
+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
if (r < 0) {
error_report("vhost guest notifier cleanup failed: %d", r);
}
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 2bf3406..e116bc6 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -60,15 +60,13 @@ vhost_user_backend_dev_init(VhostUserBackend *b,
VirtIODevice *vdev,
void
vhost_user_backend_start(VhostUserBackend *b)
{
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret, i ;
if (b->started) {
return;
}
-if (!k->set_guest_notifiers) {
+if (!virtio_device_guest_notifiers_initialized(b->vdev)) {
error_report("binding does not support guest notifiers");
return;
}
@@ -78,9 +76,8 @@ vhost_user_backend_start(VhostUserBackend *b)
return;
}
-ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs,
Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device
On Sun, May 03, 2020 at 08:36:45PM -0400, Raphael Norwitz wrote: > I’m happy from the vhost, vhost-user-blk and vhost-user-scsi side. For > other device types it looks pretty straightforward, but their maintainers > should probably confirm. > > Since you plan to change the behavior of these helpers in subsequent > patches, maybe consider sending the other device types separately > after the rest of the series has been merged? That way the changes to > individual devices will be much easier to review. Thanks for comments. Agree, will make a more straightforward fix only for vhost-user-blk. After it we can figure out how to propogate this change to other devices. > > On Thu, Apr 30, 2020 at 9:48 AM Dima Stepanov wrote: > > > > Introduce new wrappers to set/reset guest notifiers for the virtio > > device in the vhost device module: > > vhost_dev_assign_guest_notifiers > > ->set_guest_notifiers(..., ..., true); > > vhost_dev_drop_guest_notifiers > > ->set_guest_notifiers(..., ..., false); > > This is a preliminary step to refactor code, so the set_guest_notifiers > > methods could be called based on the vhost device state. > > Update all vhost used devices to use these wrappers instead of direct > > method call. > > > > Signed-off-by: Dima Stepanov > > --- > > backends/cryptodev-vhost.c | 26 +++--- > > backends/vhost-user.c | 16 +--- > > hw/block/vhost-user-blk.c | 15 +-- > > hw/net/vhost_net.c | 30 +- > > hw/scsi/vhost-scsi-common.c | 15 +-- > > hw/virtio/vhost-user-fs.c | 17 +++-- > > hw/virtio/vhost-vsock.c | 18 -- > > hw/virtio/vhost.c | 38 ++ > > hw/virtio/virtio.c | 13 + > > include/hw/virtio/vhost.h | 4 > > include/hw/virtio/virtio.h | 1 + > > 11 files changed, 118 insertions(+), 75 deletions(-) > >
Re: [PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device
I’m happy from the vhost, vhost-user-blk and vhost-user-scsi side. For other device types it looks pretty straightforward, but their maintainers should probably confirm. Since you plan to change the behavior of these helpers in subsequent patches, maybe consider sending the other device types separately after the rest of the series has been merged? That way the changes to individual devices will be much easier to review. On Thu, Apr 30, 2020 at 9:48 AM Dima Stepanov wrote: > > Introduce new wrappers to set/reset guest notifiers for the virtio > device in the vhost device module: > vhost_dev_assign_guest_notifiers > ->set_guest_notifiers(..., ..., true); > vhost_dev_drop_guest_notifiers > ->set_guest_notifiers(..., ..., false); > This is a preliminary step to refactor code, so the set_guest_notifiers > methods could be called based on the vhost device state. > Update all vhost used devices to use these wrappers instead of direct > method call. > > Signed-off-by: Dima Stepanov > --- > backends/cryptodev-vhost.c | 26 +++--- > backends/vhost-user.c | 16 +--- > hw/block/vhost-user-blk.c | 15 +-- > hw/net/vhost_net.c | 30 +- > hw/scsi/vhost-scsi-common.c | 15 +-- > hw/virtio/vhost-user-fs.c | 17 +++-- > hw/virtio/vhost-vsock.c | 18 -- > hw/virtio/vhost.c | 38 ++ > hw/virtio/virtio.c | 13 + > include/hw/virtio/vhost.h | 4 > include/hw/virtio/virtio.h | 1 + > 11 files changed, 118 insertions(+), 75 deletions(-) >
[PATCH v2 2/5] vhost: introduce wrappers to set guest notifiers for virtio device
Introduce new wrappers to set/reset guest notifiers for the virtio
device in the vhost device module:
vhost_dev_assign_guest_notifiers
->set_guest_notifiers(..., ..., true);
vhost_dev_drop_guest_notifiers
->set_guest_notifiers(..., ..., false);
This is a preliminary step to refactor code, so the set_guest_notifiers
methods could be called based on the vhost device state.
Update all vhost used devices to use these wrappers instead of direct
method call.
Signed-off-by: Dima Stepanov
---
backends/cryptodev-vhost.c | 26 +++---
backends/vhost-user.c | 16 +---
hw/block/vhost-user-blk.c | 15 +--
hw/net/vhost_net.c | 30 +-
hw/scsi/vhost-scsi-common.c | 15 +--
hw/virtio/vhost-user-fs.c | 17 +++--
hw/virtio/vhost-vsock.c | 18 --
hw/virtio/vhost.c | 38 ++
hw/virtio/virtio.c | 13 +
include/hw/virtio/vhost.h | 4
include/hw/virtio/virtio.h | 1 +
11 files changed, 118 insertions(+), 75 deletions(-)
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8337c9a..4522195 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -169,16 +169,13 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
{
VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
int r, e;
int i;
CryptoDevBackend *b = vcrypto->cryptodev;
CryptoDevBackendVhost *vhost_crypto;
CryptoDevBackendClient *cc;
-if (!k->set_guest_notifiers) {
+if (!virtio_device_guest_notifiers_initialized(dev)) {
error_report("binding does not support guest notifiers");
return -ENOSYS;
}
@@ -198,9 +195,13 @@ int cryptodev_vhost_start(VirtIODevice *dev, int
total_queues)
}
}
-r = k->set_guest_notifiers(qbus->parent, total_queues, true);
+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_assign_guest_notifiers(_crypto->dev, dev,
total_queues);
if (r < 0) {
-error_report("error binding guest notifier: %d", -r);
goto err;
}
@@ -232,7 +233,8 @@ err_start:
vhost_crypto = cryptodev_get_vhost(cc, b, i);
cryptodev_vhost_stop_one(vhost_crypto, dev);
}
-e = k->set_guest_notifiers(qbus->parent, total_queues, false);
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+e = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
if (e < 0) {
error_report("vhost guest notifier cleanup failed: %d", e);
}
@@ -242,9 +244,6 @@ err:
void cryptodev_vhost_stop(VirtIODevice *dev, int total_queues)
{
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
-VirtioBusState *vbus = VIRTIO_BUS(qbus);
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
CryptoDevBackend *b = vcrypto->cryptodev;
CryptoDevBackendVhost *vhost_crypto;
@@ -259,7 +258,12 @@ void cryptodev_vhost_stop(VirtIODevice *dev, int
total_queues)
cryptodev_vhost_stop_one(vhost_crypto, dev);
}
-r = k->set_guest_notifiers(qbus->parent, total_queues, false);
+/*
+ * Since all the states are handled by one vhost device,
+ * use the first one in array.
+ */
+vhost_crypto = cryptodev_get_vhost(b->conf.peers.ccs[0], b, 0);
+r = vhost_dev_drop_guest_notifiers(_crypto->dev, dev, total_queues);
if (r < 0) {
error_report("vhost guest notifier cleanup failed: %d", r);
}
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 2bf3406..e116bc6 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -60,15 +60,13 @@ vhost_user_backend_dev_init(VhostUserBackend *b,
VirtIODevice *vdev,
void
vhost_user_backend_start(VhostUserBackend *b)
{
-BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
-VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret, i ;
if (b->started) {
return;
}
-if (!k->set_guest_notifiers) {
+if (!virtio_device_guest_notifiers_initialized(b->vdev)) {
error_report("binding does not support guest notifiers");
return;
}
@@ -78,9 +76,8 @@ vhost_user_backend_start(VhostUserBackend *b)
return;
}
-ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs, true);
+ret = vhost_dev_assign_guest_notifiers(>dev, b->vdev, b->dev.nvqs);
if (ret < 0) {
-error_report("Error binding guest notifier");
goto err_host_notifiers;
}
@@ -104,7 +101,7 @@
