Re: [PATCH v1 1/5] virtio: Introduce notify_queue

["Alex Bennée" <alex.bennee@linaro.org>]

"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  hw/virtio/virtio.c             | 7 +++++++
>  include/hw/virtio/virtio-bus.h | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 153ee0a0cf..8a53fb5f93 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2700,12 +2700,19 @@ static void virtio_irq(VirtQueue *vq)
>  
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
>      WITH_RCU_READ_LOCK_GUARD() {
>          if (!virtio_should_notify(vdev, vq)) {
>              return;
>          }
>      }
>  
> +    if (k->notify_queue) {
> +        k->notify_queue(qbus->parent, virtio_get_queue_index(vq));
> +    }
> +
>      trace_virtio_notify(vdev, vq);
>      virtio_irq(vq);
>  }
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 7ab8c9dab0..043dbeb4cf 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -39,6 +39,7 @@ DECLARE_OBJ_CHECKERS(VirtioBusState, VirtioBusClass,
>  struct VirtioBusClass {
>      /* This is what a VirtioBus must implement */
>      BusClass parent;
> +    void (*notify_queue)(DeviceState *d, uint16_t index);
>      void (*notify)(DeviceState *d, uint16_t vector);
>      void (*save_config)(DeviceState *d, QEMUFile *f);
>      void (*save_queue)(DeviceState *d, int n, QEMUFile *f);

The code looks fine but we could do with a little outline of why we need
this is the commit messages. Why do we have notify and notify_queue? Are
they mutually exclusive? 

Not specific to this patch but we should strive to document the
individual methods in each class to give a clearer idea of what they do.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro