[Qemu-devel] Re: [PATCHv2 05/12] virtio: add APIs for queue fields

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/25/2010 12:27 PM, Michael S. Tsirkin wrote:
> vhost needs physical addresses for ring and other queue fields,
> so add APIs for these.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>   hw/virtio.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio.h |   10 +++++++++-
>   2 files changed, 59 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 1f5e7be..b017d7b 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -74,6 +74,11 @@ struct VirtQueue
>       uint16_t vector;
>       void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
>       VirtIODevice *vdev;
> +<<<<<<<  HEAD
> +=======
> +    EventNotifier guest_notifier;
> +    EventNotifier host_notifier;
> +>>>>>>>  8afa4fd... virtio: add APIs for queue fields
>    

That's clearly not right :-)

>   };
>
>   /* virt queue functions */
> @@ -593,6 +598,12 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>       return&vdev->vq[i];
>   }
>
> +void virtio_irq(VirtQueue *vq)
> +{
> +    vq->vdev->isr |= 0x01;
> +    virtio_notify_vector(vq->vdev, vq->vector);
> +}
> +
>   void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       /* Always notify when queue is empty (when feature acknowledge) */
> @@ -736,3 +747,42 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
>       vdev->binding = binding;
>       vdev->binding_opaque = opaque;
>   }
> +
> +target_phys_addr_t virtio_queue_get_desc(VirtIODevice *vdev, int n)
> +{
> +	return vdev->vq[n].vring.desc;
> +}
> +
> +target_phys_addr_t virtio_queue_get_avail(VirtIODevice *vdev, int n)
> +{
> +	return vdev->vq[n].vring.avail;
> +}
> +
> +target_phys_addr_t virtio_queue_get_used(VirtIODevice *vdev, int n)
> +{
> +	return vdev->vq[n].vring.used;
> +}
> +
> +uint16_t virtio_queue_last_avail_idx(VirtIODevice *vdev, int n)
> +{
> +	return vdev->vq[n].last_avail_idx;
> +}
> +
> +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
> +{
> +	vdev->vq[n].last_avail_idx = idx;
> +}
>    

Is it really necessary to return last_avail?  Can't vhost maintain it's 
own last_avail?

I'm not a huge fan of returning physical addresses for each queue 
element.  I think it makes more sense to just return the start of the 
ring queue.  The ABI defines the queue to have a very specific layout 
afterall.

Regards,

Anthony Liguori

> +VirtQueue *virtio_queue(VirtIODevice *vdev, int n)
> +{
> +	return vdev->vq + n;
> +}
> +
> +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> +{
> +	return&vq->guest_notifier;
> +}
> +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> +{
> +	return&vq->host_notifier;
> +}
> diff --git a/hw/virtio.h b/hw/virtio.h
> index af87889..2ebf2dd 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -184,5 +184,13 @@ void virtio_net_exit(VirtIODevice *vdev);
>   	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
>   			VIRTIO_RING_F_INDIRECT_DESC, true)
>
> -
> +target_phys_addr_t virtio_queue_get_desc(VirtIODevice *vdev, int n);
> +target_phys_addr_t virtio_queue_get_avail(VirtIODevice *vdev, int n);
> +target_phys_addr_t virtio_queue_get_used(VirtIODevice *vdev, int n);
> +uint16_t virtio_queue_last_avail_idx(VirtIODevice *vdev, int n);
> +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx);
> +VirtQueue *virtio_queue(VirtIODevice *vdev, int n);
> +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq);
> +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq);
> +void

> virtio_irq(VirtQueue *vq);
>   #endif
>