Patchwork [PATCHv4,05/12] virtio: add APIs for queue fields

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 3, 2010, 5:16 p.m.
Message ID <be4ce3d477c6a1133743de04f8e521d2775ace6e.1267636215.git.mst@redhat.com>
Download mbox | patch
Permalink /patch/46840/
State New
Headers show

Comments

Michael S. Tsirkin - March 3, 2010, 5:16 p.m.
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 |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.h |   15 +++++++++++-
 2 files changed, 90 insertions(+), 1 deletions(-)
Amit Shah - March 5, 2010, 12:10 p.m.
On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> vhost needs physical addresses for ring and other queue fields,
> so add APIs for these.

Already discussed on IRC, but mentioning here so that it doesn't get
lost:

> +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;
> +}
> +
> +target_phys_addr_t virtio_queue_get_ring(VirtIODevice *vdev, int n)
> +{
> +    return vdev->vq[n].vring.desc;
> +}

All these functions return the start address of these fields; any better
way to name them?

eg, just by looking at, eg, 'virtio_queue_get_used()', I'd expect that
the function returns the number of used buffers in the ring, not the
start address of the used buffers.

> +target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> +{
> +    return sizeof(VRingDesc) * vdev->vq[n].vring.num;
> +}
> +
> +target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> +{
> +    return offsetof(VRingAvail, ring) +
> +        sizeof(u_int64_t) * vdev->vq[n].vring.num;
> +}
> +
> +target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> +{
> +    return offsetof(VRingUsed, ring) +
> +        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> +}
> +
> +

Extra newline

> +target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
> +{
> +    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
> +	    virtio_queue_get_used_size(vdev, n);
> +}
> +
> +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;
> +}

virtio_queue_last_avail_idx() does make sense, but since you have a
'set_last_avail_idx', better name the previous one 'get_..'?

> +VirtQueue *virtio_queue(VirtIODevice *vdev, int n)
> +{
> +    return vdev->vq + n;
> +}

This really doesn't mean anything; I suggest virtio_queue_get_vq().

> +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> +{
> +    return &vq->guest_notifier;
> +}
> +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> +{
> +    return &vq->host_notifier;
> +}

Why drop the 'get_' for these functions?

virtio_queue_get_guest_notifier()
and
virtio_queue_get_host_notifier()

might be better.

		Amit
Amit Shah - March 5, 2010, 1:08 p.m.
On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> +
> +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> +{
> +    return &vq->guest_notifier;
> +}
> +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> +{
> +    return &vq->host_notifier;
> +}

Where are these host_notifier and guest_notifier fields set? Am I
completely missing it?

		Amit
Michael S. Tsirkin - March 6, 2010, 7:07 p.m.
On Fri, Mar 05, 2010 at 06:38:35PM +0530, Amit Shah wrote:
> On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> > +
> > +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->guest_notifier;
> > +}
> > +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->host_notifier;
> > +}
> 
> Where are these host_notifier and guest_notifier fields set? Am I
> completely missing it?
> 
> 		Amit

What do you mean by "set"?
You get a pointer, you can e.g. init it.
Michael S. Tsirkin - March 6, 2010, 7:09 p.m.
On Fri, Mar 05, 2010 at 05:40:18PM +0530, Amit Shah wrote:
> On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> > vhost needs physical addresses for ring and other queue fields,
> > so add APIs for these.
> 
> Already discussed on IRC, but mentioning here so that it doesn't get
> lost:
> 
> > +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;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_ring(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.desc;
> > +}
> 
> All these functions return the start address of these fields; any better
> way to name them?
> 
> eg, just by looking at, eg, 'virtio_queue_get_used()', I'd expect that
> the function returns the number of used buffers in the ring, not the
> start address of the used buffers.
> 
> > +target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > +{
> > +    return sizeof(VRingDesc) * vdev->vq[n].vring.num;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > +{
> > +    return offsetof(VRingAvail, ring) +
> > +        sizeof(u_int64_t) * vdev->vq[n].vring.num;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> > +{
> > +    return offsetof(VRingUsed, ring) +
> > +        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> > +}
> > +
> > +
> 
> Extra newline
> 
> > +target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
> > +	    virtio_queue_get_used_size(vdev, n);
> > +}
> > +
> > +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;
> > +}
> 
> virtio_queue_last_avail_idx() does make sense, but since you have a
> 'set_last_avail_idx', better name the previous one 'get_..'?
> 
> > +VirtQueue *virtio_queue(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq + n;
> > +}
> 
> This really doesn't mean anything; I suggest virtio_queue_get_vq().
> 
> > +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->guest_notifier;
> > +}
> > +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->host_notifier;
> > +}
> 
> Why drop the 'get_' for these functions?
> 
> virtio_queue_get_guest_notifier()
> and
> virtio_queue_get_host_notifier()
> 
> might be better.
> 
> 		Amit

Not sure we want 'get' all around, but I'll think about the names, thanks!
Amit Shah - March 8, 2010, 6:16 a.m.
On (Sat) Mar 06 2010 [21:07:57], Michael S. Tsirkin wrote:
> On Fri, Mar 05, 2010 at 06:38:35PM +0530, Amit Shah wrote:
> > On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> > > +
> > > +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> > > +{
> > > +    return &vq->guest_notifier;
> > > +}
> > > +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> > > +{
> > > +    return &vq->host_notifier;
> > > +}
> > 
> > Where are these host_notifier and guest_notifier fields set? Am I
> > completely missing it?
> 
> What do you mean by "set"?
> You get a pointer, you can e.g. init it.

I mean where is vq->host_notifier initialised?

		Amit
Michael S. Tsirkin - March 8, 2010, 6:11 p.m.
On Mon, Mar 08, 2010 at 11:46:46AM +0530, Amit Shah wrote:
> On (Sat) Mar 06 2010 [21:07:57], Michael S. Tsirkin wrote:
> > On Fri, Mar 05, 2010 at 06:38:35PM +0530, Amit Shah wrote:
> > > On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> > > > +
> > > > +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> > > > +{
> > > > +    return &vq->guest_notifier;
> > > > +}
> > > > +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> > > > +{
> > > > +    return &vq->host_notifier;
> > > > +}
> > > 
> > > Where are these host_notifier and guest_notifier fields set? Am I
> > > completely missing it?
> > 
> > What do you mean by "set"?
> > You get a pointer, you can e.g. init it.
> 
> I mean where is vq->host_notifier initialised?
> 
> 		Amit

There's a function to do this. Called from virtio-pci.

Patch

diff --git a/hw/virtio.c b/hw/virtio.c
index 1f5e7be..e5787fa 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -74,6 +74,8 @@  struct VirtQueue
     uint16_t vector;
     void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
     VirtIODevice *vdev;
+    EventNotifier guest_notifier;
+    EventNotifier host_notifier;
 };
 
 /* virt queue functions */
@@ -593,6 +595,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 +744,71 @@  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;
+}
+
+target_phys_addr_t virtio_queue_get_ring(VirtIODevice *vdev, int n)
+{
+    return vdev->vq[n].vring.desc;
+}
+
+target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
+{
+    return sizeof(VRingDesc) * vdev->vq[n].vring.num;
+}
+
+target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
+{
+    return offsetof(VRingAvail, ring) +
+        sizeof(u_int64_t) * vdev->vq[n].vring.num;
+}
+
+target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n)
+{
+    return offsetof(VRingUsed, ring) +
+        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
+}
+
+
+target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
+{
+    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
+	    virtio_queue_get_used_size(vdev, n);
+}
+
+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;
+}
+
+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..26cf5fd 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -184,5 +184,18 @@  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);
+target_phys_addr_t virtio_queue_get_ring(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n);
+target_phys_addr_t virtio_queue_get_ring_size(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