| Message ID | be4ce3d477c6a1133743de04f8e521d2775ace6e.1267636215.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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!
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
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.
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
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(-)