[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 06/10] vhost:add support for configure interrupt
From: |
Cindy Lu |
Subject: |
Re: [PATCH v7 06/10] vhost:add support for configure interrupt |
Date: |
Tue, 8 Jun 2021 11:20:18 +0800 |
On Thu, Jun 3, 2021 at 2:28 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/6/2 上午11:47, Cindy Lu 写道:
> > Add configure notifier support in vhost and virtio driver
> > When backend support VIRTIO_NET_F_STATUS,
>
>
> So config interrupt is the basic facility of the virtio device. We need
> to make the code not specific to this feature.
>
> But we can leave the specific device (like virtio-net, or vhost-net) to
> decide whether or not it can be used.
>
> For net, it is also used by guest announcement.
>
sure, I will rewrite this part
>
> > setup the configure
> > interrupt function in vhost_dev_start and release the related
> > resource when vhost_dev_stop
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> > hw/net/vhost_net.c | 9 +++++
> > hw/net/virtio-net.c | 6 ++++
> > hw/virtio/vhost.c | 68 ++++++++++++++++++++++++++++++++++++--
> > hw/virtio/virtio.c | 23 +++++++++++--
> > include/hw/virtio/vhost.h | 2 ++
> > include/hw/virtio/virtio.h | 3 ++
> > include/net/vhost_net.h | 3 ++
> > 7 files changed, 109 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 24d555e764..5d0c35f18d 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -426,6 +426,15 @@ void vhost_net_virtqueue_mask(VHostNetState *net,
> > VirtIODevice *dev,
> > vhost_virtqueue_mask(&net->dev, dev, idx, mask);
> > }
> >
> > +bool vhost_net_config_pending(VHostNetState *net, int idx)
> > +{
> > + return vhost_config_pending(&net->dev, idx);
> > +}
> > +void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev,
> > + bool mask)
> > +{
> > + vhost_config_mask(&net->dev, dev, mask);
> > +}
> > VHostNetState *get_vhost_net(NetClientState *nc)
> > {
> > VHostNetState *vhost_net = 0;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index f50235b5d6..02033be748 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3055,6 +3055,9 @@ static bool
> > virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> > if (idx != VIRTIO_CONFIG_IRQ_IDX) {
> > return vhost_net_virtqueue_pending(get_vhost_net(nc->peer), idx);
> > }
> > + if (idx == VIRTIO_CONFIG_IRQ_IDX) {
> > + return vhost_net_config_pending(get_vhost_net(nc->peer), idx);
> > + }
> > return false;
> > }
> >
> > @@ -3067,6 +3070,9 @@ static void
> > virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> > if (idx != VIRTIO_CONFIG_IRQ_IDX) {
> > vhost_net_virtqueue_mask(get_vhost_net(nc->peer), vdev, idx,
> > mask);
> > }
> > + if (idx == VIRTIO_CONFIG_IRQ_IDX) {
> > + vhost_net_config_mask(get_vhost_net(nc->peer), vdev, mask);
> > + }
> > }
> >
> > static void virtio_net_set_config_size(VirtIONet *n, uint64_t
> > host_features)
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index e2163a0d63..3b05f09d98 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -21,6 +21,7 @@
> > #include "qemu/error-report.h"
> > #include "qemu/memfd.h"
> > #include "standard-headers/linux/vhost_types.h"
> > +#include "standard-headers/linux/virtio_net.h"
>
>
> Inclusion of device specific header in the general vhost code looks like
> a layer violation.
>
>
> > #include "exec/address-spaces.h"
> > #include "hw/virtio/virtio-bus.h"
> > #include "hw/virtio/virtio-access.h"
> > @@ -1505,6 +1506,16 @@ bool vhost_virtqueue_pending(struct vhost_dev *hdev,
> > int n)
> > return event_notifier_test_and_clear(&vq->masked_notifier);
> > }
> >
> > +bool vhost_config_pending(struct vhost_dev *hdev, int n)
> > +{
> > + assert(hdev->vhost_ops);
>
>
> I think we can remove this.
>
>
> > + VirtIODevice *vdev = hdev->vdev;
> > + if ((hdev->started == false) ||
>
>
> We don't check this in vhost_virtqueue_pending(), any reason to do this?
>
>
> > + (hdev->vhost_ops->vhost_set_config_call == NULL)) {
>
>
> I think we need first check what happens if we try to down the link for
> vhost-vdpa network backend in the monitor. (Or should we block that from
> the monitor?)
>
> Then we can decided whether or not we need this.
>
>
I will remove this part
> > + return false;
> > + }
> > + return event_notifier_test_and_clear(&vdev->masked_config_notifier);
> > +}
> > /* Mask/unmask events from this vq. */
> > void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int
> > n,
> > bool mask)
> > @@ -1529,6 +1540,30 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > VirtIODevice *vdev, int n,
> > VHOST_OPS_DEBUG("vhost_set_vring_call failed");
> > }
> > }
> > +void vhost_config_mask(struct vhost_dev *hdev, VirtIODevice *vdev,
> > + bool mask)
> > +{
> > + int fd;
> > + int r;
> > + EventNotifier *masked_config_notifier = &vdev->masked_config_notifier;
> > + EventNotifier *config_notifier = &vdev->config_notifier;
> > + assert(hdev->vhost_ops);
> > +
> > + if ((hdev->started == false) ||
> > + (hdev->vhost_ops->vhost_set_config_call == NULL)) {
> > + return ;
> > + }
> > + if (mask) {
> > + assert(vdev->use_guest_notifier_mask);
> > + fd = event_notifier_get_fd(masked_config_notifier);
> > + } else {
> > + fd = event_notifier_get_fd(config_notifier);
> > + }
> > + r = hdev->vhost_ops->vhost_set_config_call(hdev, &fd);
> > + if (r < 0) {
> > + error_report("vhost_set_config_call failed");
>
>
> VHOST_OPS_DEBUG() please.
>
will fix this
>
> > + }
> > +}
> >
> > uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> > *feature_bits,
> > uint64_t features)
> > @@ -1708,6 +1743,7 @@ int vhost_dev_get_inflight(struct vhost_dev *dev,
> > uint16_t queue_size,
> > int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > {
> > int i, r;
> > + int fd = 0;
> >
> > /* should only be called after backend is connected */
> > assert(hdev->vhost_ops);
> > @@ -1739,7 +1775,14 @@ int vhost_dev_start(struct vhost_dev *hdev,
> > VirtIODevice *vdev)
> > goto fail_vq;
> > }
> > }
> > -
> > + r = event_notifier_init(&vdev->masked_config_notifier, 0);
> > + if (r < 0) {
> > + return r;
> > + }
> > + event_notifier_test_and_clear(&vdev->masked_config_notifier);
> > + if (!vdev->use_guest_notifier_mask) {
> > + vhost_config_mask(hdev, vdev, true);
> > + }
>
>
> Let's leave a new line for separating the logical blocks.
>
>
will fix this
> > if (hdev->log_enabled) {
> > uint64_t log_base;
> >
> > @@ -1773,6 +1816,19 @@ int vhost_dev_start(struct vhost_dev *hdev,
> > VirtIODevice *vdev)
> > vhost_device_iotlb_miss(hdev, vq->used_phys, true);
> > }
> > }
> > + if (!(hdev->features & (0x1ULL << VIRTIO_NET_F_STATUS))) {
> > + return 0;
> > + }
>
>
> Let's move this to vhost-net instead of doing it in the general vhost code.
>
> Or we can even remove this, otherwise it will introduce some burden if
> we need to implement other features that depends on the config interrupt
> for vhost-net/vDPA.
>
>
will fix this
> > + if (hdev->vhost_ops->vhost_set_config_call) {
> > + fd = event_notifier_get_fd(&vdev->config_notifier);
> > + r = hdev->vhost_ops->vhost_set_config_call(hdev, &fd);
> > + if (!r) {
> > + event_notifier_set(&vdev->config_notifier);
>
>
> Any reason you need to trigger the config interrupt here?
>
>
> > + }
> > + if (r) {
> > + goto fail_log;
> > + }
> > + }
> > return 0;
> > fail_log:
> > vhost_log_put(hdev, false);
> > @@ -1795,10 +1851,18 @@ fail_features:
> > void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > {
> > int i;
> > + int fd;
> >
> > /* should only be called after backend is connected */
> > assert(hdev->vhost_ops);
> > -
> > + event_notifier_test_and_clear(&vdev->masked_config_notifier);
> > + event_notifier_test_and_clear(&vdev->config_notifier);
>
>
> If we do this in the start, there's no need for such cleanup here.
>
>
> > + if ((hdev->features & (0x1ULL << VIRTIO_NET_F_STATUS))) {
>
>
> Let's move this to vhost-net or I'd rather remove this.
>
> Thanks
>
>
will fix this
> > + if (hdev->vhost_ops->vhost_set_config_call) {
> > + fd = -1;
> > + hdev->vhost_ops->vhost_set_config_call(hdev, &fd);
> > + }
> > + }
> > if (hdev->vhost_ops->vhost_dev_start) {
> > hdev->vhost_ops->vhost_dev_start(hdev, false);
> > }
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index c5d786bb5e..09ed3f67d9 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3506,14 +3506,27 @@ static void
> > virtio_queue_guest_notifier_read(EventNotifier *n)
> > }
> >
> >
> > +static void virtio_config_read(EventNotifier *n)
> > +{
> > + VirtIODevice *vdev = container_of(n, VirtIODevice, config_notifier);
> > +
> > + if (event_notifier_test_and_clear(n)) {
> > + virtio_notify_config(vdev);
> > + }
> > +}
> > void virtio_set_notifier_fd_handler(VirtIODevice *vdev, int queue_no,
> > bool assign, bool with_irqfd)
> > {
> > EventNotifier *e ;
> > EventNotifierHandler *handler;
> > - VirtQueue *vq = virtio_get_queue(vdev, queue_no);
> > - e = &vq->guest_notifier;
> > - handler = virtio_queue_guest_notifier_read;
> > + if (queue_no != VIRTIO_CONFIG_IRQ_IDX) {
> > + VirtQueue *vq = virtio_get_queue(vdev, queue_no);
> > + e = &vq->guest_notifier;
> > + handler = virtio_queue_guest_notifier_read;
> > + } else {
> > + e = &vdev->config_notifier;
> > + handler = virtio_config_read;
> > + }
> > if (assign && !with_irqfd) {
> > event_notifier_set_handler(e, handler);
> > } else {
> > @@ -3599,6 +3612,10 @@ EventNotifier
> > *virtio_queue_get_host_notifier(VirtQueue *vq)
> > return &vq->host_notifier;
> > }
> >
> > +EventNotifier *virtio_get_config_notifier(VirtIODevice *vdev)
> > +{
> > + return &vdev->config_notifier;
> > +}
> > void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled)
> > {
> > vq->host_notifier_enabled = enabled;
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 4a8bc75415..b8814ece32 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -108,6 +108,8 @@ int vhost_dev_start(struct vhost_dev *hdev,
> > VirtIODevice *vdev);
> > void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> > int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice
> > *vdev);
> > void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice
> > *vdev);
> > +bool vhost_config_pending(struct vhost_dev *hdev, int n);
> > +void vhost_config_mask(struct vhost_dev *hdev, VirtIODevice *vdev, bool
> > mask);
> >
> > /* Test and clear masked event pending status.
> > * Should be called after unmask to avoid losing events.
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 447899dea5..5856517d43 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -110,6 +110,8 @@ struct VirtIODevice
> > bool use_guest_notifier_mask;
> > AddressSpace *dma_as;
> > QLIST_HEAD(, VirtQueue) *vector_queues;
> > + EventNotifier config_notifier;
> > + EventNotifier masked_config_notifier;
> > };
> >
> > struct VirtioDeviceClass {
> > @@ -317,6 +319,7 @@ int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
> > void virtio_device_release_ioeventfd(VirtIODevice *vdev);
> > bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
> > EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
> > +EventNotifier *virtio_get_config_notifier(VirtIODevice *vdev);
> > void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled);
> > void virtio_queue_host_notifier_read(EventNotifier *n);
> > void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext
> > *ctx,
> > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > index 172b0051d8..0d38c97c94 100644
> > --- a/include/net/vhost_net.h
> > +++ b/include/net/vhost_net.h
> > @@ -36,6 +36,9 @@ int vhost_net_set_config(struct vhost_net *net, const
> > uint8_t *data,
> > bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
> > void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
> > int idx, bool mask);
> > +bool vhost_net_config_pending(VHostNetState *net, int n);
> > +void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev,
> > + bool mask);
> > int vhost_net_notify_migration_done(VHostNetState *net, char* mac_addr);
> > VHostNetState *get_vhost_net(NetClientState *nc);
> >
>
- Re: [PATCH v7 02/10] virtio-pci:decouple virtqueue from interrupt setting process, (continued)
- [PATCH v7 03/10] virtio: decouple virtqueue from set notifier fd handler, Cindy Lu, 2021/06/01
- [PATCH v7 04/10] vhost: add new call back function for config interrupt, Cindy Lu, 2021/06/01
- [PATCH v7 05/10] vhost-vdpa: add support for config interrupt call back, Cindy Lu, 2021/06/01
- [PATCH v7 06/10] vhost:add support for configure interrupt, Cindy Lu, 2021/06/01
- [PATCH v7 07/10] virtio-mmio: add support for configure interrupt, Cindy Lu, 2021/06/01
- [PATCH v7 08/10] virtio-pci: decouple virtqueue from kvm_virtio_pci_vector_use, Cindy Lu, 2021/06/01
- [PATCH v7 10/10] virtio-net: add peer_deleted check in virtio_net_handle_rx, Cindy Lu, 2021/06/01
- [PATCH v7 09/10] virtio-pci: add support for configure interrupt, Cindy Lu, 2021/06/01