| Message ID | b00ef5d572ec9bc3b884c496025512abc1f14349.1304541919.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > Define a new feature bit for the host to > declare that it uses an avail_event index > (like Xen) instead of a feature bit > to enable/disable interrupts. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/linux/virtio_ring.h | 11 ++++++++--- > 1 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > index f5c1b75..f791772 100644 > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -32,6 +32,9 @@ > /* The Guest publishes the used index for which it expects an interrupt > * at the end of the avail ring. Host should ignore the avail->flags field. */ > #define VIRTIO_RING_F_USED_EVENT_IDX 29 > +/* The Host publishes the avail index for which it expects a kick > + * at the end of the used ring. Guest should ignore the used->flags field. */ > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler. Or are there cases where the old style makes more sense? Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 09, 2011 at 01:43:15PM +0930, Rusty Russell wrote: > On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > Define a new feature bit for the host to > > declare that it uses an avail_event index > > (like Xen) instead of a feature bit > > to enable/disable interrupts. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > include/linux/virtio_ring.h | 11 ++++++++--- > > 1 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > index f5c1b75..f791772 100644 > > --- a/include/linux/virtio_ring.h > > +++ b/include/linux/virtio_ring.h > > @@ -32,6 +32,9 @@ > > /* The Guest publishes the used index for which it expects an interrupt > > * at the end of the avail ring. Host should ignore the avail->flags field. */ > > #define VIRTIO_RING_F_USED_EVENT_IDX 29 > > +/* The Host publishes the avail index for which it expects a kick > > + * at the end of the used ring. Guest should ignore the used->flags field. */ > > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 > > Are you really sure we want to separate the two? Seems a little simpler > to have one bit to mean "we're publishing our threshold". For someone > implementing this from scratch, it's a little simpler. > > Or are there cases where the old style makes more sense? > > Thanks, > Rusty. Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling. But I'm not hung up on this. Let me know pls.
On Sun, 15 May 2011 15:47:27 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, May 09, 2011 at 01:43:15PM +0930, Rusty Russell wrote: > > On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > #define VIRTIO_RING_F_USED_EVENT_IDX 29 > > > +/* The Host publishes the avail index for which it expects a kick > > > + * at the end of the used ring. Guest should ignore the used->flags field. */ > > > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 > > > > Are you really sure we want to separate the two? Seems a little simpler > > to have one bit to mean "we're publishing our threshold". For someone > > implementing this from scratch, it's a little simpler. > > > > Or are there cases where the old style makes more sense? > > > > Thanks, > > Rusty. > > Hmm, it makes debugging easier as each side can disable > publishing separately - I used it all the time when I saw > e.g. networking stuck to guess whether I need to investigate the > interrupt or the exit handling. > > But I'm not hung up on this. > > Let me know pls. If we combine them into one, then these patches no longer depend on the feature bit expansion, which is worthwhile (though I'll take both). Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 03:53:19PM +0930, Rusty Russell wrote: > On Sun, 15 May 2011 15:47:27 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Mon, May 09, 2011 at 01:43:15PM +0930, Rusty Russell wrote: > > > On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > #define VIRTIO_RING_F_USED_EVENT_IDX 29 > > > > +/* The Host publishes the avail index for which it expects a kick > > > > + * at the end of the used ring. Guest should ignore the used->flags field. */ > > > > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 > > > > > > Are you really sure we want to separate the two? Seems a little simpler > > > to have one bit to mean "we're publishing our threshold". For someone > > > implementing this from scratch, it's a little simpler. > > > > > > Or are there cases where the old style makes more sense? > > > > > > Thanks, > > > Rusty. > > > > Hmm, it makes debugging easier as each side can disable > > publishing separately - I used it all the time when I saw > > e.g. networking stuck to guess whether I need to investigate the > > interrupt or the exit handling. > > > > But I'm not hung up on this. > > > > Let me know pls. > > If we combine them into one, then these patches no longer depend on > the feature bit expansion, which is worthwhile (though I'll take both). > > Thanks, > Rusty. Yes, I know. But if we do expand feature bits anyway, for debugging and profiling if nothing else it's useful to have them separate ... If you take both why does the order matter?
On Tue, 17 May 2011 09:00:52 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, May 16, 2011 at 03:53:19PM +0930, Rusty Russell wrote: > > On Sun, 15 May 2011 15:47:27 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, May 09, 2011 at 01:43:15PM +0930, Rusty Russell wrote: > > > > On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > #define VIRTIO_RING_F_USED_EVENT_IDX 29 > > > > > +/* The Host publishes the avail index for which it expects a kick > > > > > + * at the end of the used ring. Guest should ignore the used->flags field. */ > > > > > +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 > > > > > > > > Are you really sure we want to separate the two? Seems a little simpler > > > > to have one bit to mean "we're publishing our threshold". For someone > > > > implementing this from scratch, it's a little simpler. > > > > > > > > Or are there cases where the old style makes more sense? > > > > > > > > Thanks, > > > > Rusty. > > > > > > Hmm, it makes debugging easier as each side can disable > > > publishing separately - I used it all the time when I saw > > > e.g. networking stuck to guess whether I need to investigate the > > > interrupt or the exit handling. > > > > > > But I'm not hung up on this. > > > > > > Let me know pls. > > > > If we combine them into one, then these patches no longer depend on > > the feature bit expansion, which is worthwhile (though I'll take both). > > > > Thanks, > > Rusty. > > Yes, I know. But if we do expand feature bits anyway, for debugging > and profiling if nothing else it's useful to have them separate ... > If you take both why does the order matter? Damage control. Then if something breaks, it doesn't break everything. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h index f5c1b75..f791772 100644 --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -32,6 +32,9 @@ /* The Guest publishes the used index for which it expects an interrupt * at the end of the avail ring. Host should ignore the avail->flags field. */ #define VIRTIO_RING_F_USED_EVENT_IDX 29 +/* The Host publishes the avail index for which it expects a kick + * at the end of the used ring. Guest should ignore the used->flags field. */ +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32 /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ struct vring_desc { @@ -96,11 +99,13 @@ struct vring { * __u16 used_flags; * __u16 used_idx; * struct vring_used_elem used[num]; + * __u16 avail_event_idx; * }; */ -/* We publish the used event index at the end of the available ring. - * It is at the end for backwards compatibility. */ +/* We publish the used event index at the end of the available ring, and vice + * versa. They are at the end for backwards compatibility. */ #define vring_used_event(vr) ((vr)->avail->ring[(vr)->num]) +#define vring_avail_event(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num]) static inline void vring_init(struct vring *vr, unsigned int num, void *p, unsigned long align) @@ -116,7 +121,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align) { return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num) + align - 1) & ~(align - 1)) - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num; + + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num; } #ifdef __KERNEL__
Define a new feature bit for the host to declare that it uses an avail_event index (like Xen) instead of a feature bit to enable/disable interrupts. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/linux/virtio_ring.h | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)