Re: [PATCH v8 5/9] vhost-net:add support for configure interrupt

[Jason Wang]

在 2021/7/6 下午3:20, Cindy Lu 写道:
> Add configure notifier support in vhost and virtio driver
> When backend support VIRTIO_NET_F_STATUS,setup the configure
> interrupt function in vhost_net_start and release the related
> resource when vhost_net_stop
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>   hw/net/vhost_net.c         | 36 +++++++++++++++++++++++++++++++
>   hw/net/virtio-net.c        |  6 ++++++
>   hw/virtio/vhost.c          | 44 ++++++++++++++++++++++++++++++++++++++
>   hw/virtio/virtio.c         | 33 ++++++++++++++++++++++++++--
>   include/hw/virtio/vhost.h  |  2 ++
>   include/hw/virtio/virtio.h |  5 +++++
>   include/net/vhost_net.h    |  3 +++
>   7 files changed, 127 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 24d555e764..be453717c4 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -310,6 +310,31 @@ static void vhost_net_stop_one(struct vhost_net *net,
>       vhost_dev_disable_notifiers(&net->dev, dev);
>   }
>   
> +static void vhost_net_stop_config_intr(struct vhost_net *net)
> +{
> +    struct vhost_dev *dev = &net->dev;
> +    if (dev->features & (0x1ULL << VIRTIO_NET_F_STATUS)) {
> +        if (dev->vhost_ops->vhost_set_config_call) {
> +            int fd = -1;
> +            dev->vhost_ops->vhost_set_config_call(dev, fd);
> +        }
> +    }
> +}
> +
> +static void vhost_net_start_config_intr(struct vhost_net *net)
> +{
> +    struct vhost_dev *dev = &net->dev;
> +    if (!(dev->features & (0x1ULL << VIRTIO_NET_F_STATUS))) {
> +        return;
> +    }


Rethink about this, I don't think we need such whitelist. Config 
interrupt works like a basic device facility.


> +    if (dev->vhost_ops->vhost_set_config_call) {
> +        int fd = event_notifier_get_fd(&dev->vdev->config_notifier);
> +        int r = dev->vhost_ops->vhost_set_config_call(dev, fd);
> +        if (!r) {
> +            event_notifier_set(&dev->vdev->config_notifier);
> +        }
> +    }
> +}
>   int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>                       int total_queues)
>   {
> @@ -364,6 +389,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>           }
>       }
>   
> +    vhost_net_start_config_intr(get_vhost_net(ncs[0].peer));


I think we can reuse vhost_vdpa_dev_start()?


>       return 0;
>   
>   err_start:
> @@ -397,6 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>           fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", r);
>           fflush(stderr);
>       }
> +    vhost_net_stop_config_intr(get_vhost_net(ncs[0].peer));
>       assert(r >= 0);
>   }
>   
> @@ -426,6 +453,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);
> +}


Blank line is needed.


> +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);


I think there's no need to pass idx to vhost_net_config_pending.


> +   }
>       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..6716109448 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1505,6 +1505,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);
> +    VirtIODevice *vdev = hdev->vdev;
> +    if ((hdev->started == false) ||
> +        (hdev->vhost_ops->vhost_set_config_call == NULL)) {
> +        return false;
> +    }
> +    return event_notifier_test_and_clear(&vdev->masked_config_notifier);


n is not used.


> +}
>   /* Mask/unmask events from this vq. */
>   void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>                            bool mask)
> @@ -1529,6 +1539,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) ||


We don't check this in vhost_virtqueue_mask(), any reason for doing this?


> +        (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);
> +    }


Please check the indentation above.


> +   r = hdev->vhost_ops->vhost_set_config_call(hdev, fd);
> +   if (r < 0) {
> +        VHOST_OPS_DEBUG("vhost_set_config_call failed");
> +    }
> +}
>   
>   uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                               uint64_t features)
> @@ -1740,6 +1774,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice 
> *vdev)
>           }
>       }
>   
> +    r = event_notifier_init(&vdev->masked_config_notifier, 0);
> +    if (r < 0) {
> +        return r;
> +    }
> +    event_notifier_test_and_clear(&vdev->masked_config_notifier);


This wont' work in the case of multiqueue. And it's kind of layer 
violation if you initialize the VirtioDevice stuff in the vhost layer.

Note that the virtqueue mask notifiers is embed in structure 
vhost_virtqueue, it means you probably need to embed the 
masked_config_notifier to be embed in struct vhost_dev.

And deal with the multiqueue stuffs, (e.g we will only use the notifier 
for the first queue pair).


> +    if (!vdev->use_guest_notifier_mask) {
> +        vhost_config_mask(hdev, vdev, true);
> +    }
>       if (hdev->log_enabled) {
>           uint64_t log_base;
>   
> @@ -1798,6 +1840,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>   
>       /* 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 (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 07f4e60b30..dbd2e36403 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3504,10 +3504,18 @@ static void 
> virtio_queue_guest_notifier_read(EventNotifier *n)
>           virtio_irq(vq);
>       }
>   }
> +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);
> +    }
> +}


This (the changes in virtio.c) need to be done via an separate patch, it 
belongs to the abstraction layer of guest notifier.

And we need rename this as virtio_config_guest_notifier_read().


>   void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                   bool with_irqfd)
>   {
> +


Unnecessary change.


>       if (assign && !with_irqfd) {
>           event_notifier_set_handler(&vq->guest_notifier,
>                                      virtio_queue_guest_notifier_read);
> @@ -3515,12 +3523,29 @@ void 
> virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>           event_notifier_set_handler(&vq->guest_notifier, NULL);
>       }
>       if (!assign) {
> -        /* Test and clear notifier before closing it,
> -         * in case poll callback didn't have time to run. */
> +        /* Test and clear notifier before closing it,*/
> +        /* in case poll callback didn't have time to run. */


Unnecessary changes?


>           virtio_queue_guest_notifier_read(&vq->guest_notifier);
>       }
>   }
>   
> +void virtio_set_config_notifier_fd_handler(VirtIODevice *vdev, bool assign,
> +                                              bool with_irqfd)


Let's use "virtio_config_set_guest_notifier_fd_handler()


> +{
> +    EventNotifier *n;
> +    n = &vdev->config_notifier;
> +    if (assign && !with_irqfd) {
> +        event_notifier_set_handler(n, virtio_config_read);
> +    } else {
> +        event_notifier_set_handler(n, NULL);
> +    }
> +    if (!assign) {
> +        /* Test and clear notifier before closing it,*/
> +        /* in case poll callback didn't have time to run. */
> +        virtio_config_read(n);
> +    }
> +}
> +
>   EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
>   {
>       return &vq->guest_notifier;
> @@ -3594,6 +3619,10 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue 
> *vq)
>       return &vq->host_notifier;
>   }
>   
> +EventNotifier *virtio_get_config_notifier(VirtIODevice *vdev)


Let's use "virtio_config_get_guest_notifier" to be consistent with the 
guest notifier abstraction.

Thanks


> +{
> +    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 63cb9455ed..3bfd0692a4 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 {
> @@ -312,11 +314,14 @@ uint16_t virtio_get_queue_index(VirtQueue *vq);
>   EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
>   void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                   bool with_irqfd);
> + void virtio_set_config_notifier_fd_handler(VirtIODevice *vdev, bool assign,
> +                                            bool with_irqfd);
>   int virtio_device_start_ioeventfd(VirtIODevice *vdev);
>   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);
>