On Wed, Oct 22, 2014 at 10:24:17AM +0200, Cornelia Huck wrote: > On Wed, 22 Oct 2014 01:09:40 +0300 > "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote: > > > This adds wrappers to switch between native endian-ness > > (virtio 0.9) and virtio endian-ness (virtio 1.0). > > Add new typedefs as well, so that we can check > > statically that we didn't miss any accesses. > > All callers simply pass in false (0.9) so no > > functional change for now. > > > > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx> > > > > --- > > > > Sending this early so I can get feedback on this style. > > Hm... > > http://marc.info/?l=linux-virtualization&m=141270444612625&w=2 > > (and other in that series. Forgot to cc: you on those patches...) Thanks, will review. > > Rusty, what's your opinion? Reasonable? > > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > index 67e06fe..32211aa 100644 > > --- a/include/linux/virtio_ring.h > > +++ b/include/linux/virtio_ring.h > > @@ -62,6 +62,26 @@ static inline void virtio_wmb(bool weak_barriers) > > } > > #endif > > > > +#define DEFINE_VIRTIO_XX_TO_CPU(bits) \ > > +static inline u##bits virtio##bits##_to_cpu(bool little_endian, __virtio##bits val) \ > > +{ \ > > + if (little_endian) \ > > + return le##bits##_to_cpu((__force __le##bits)val); \ > > + else \ > > + return (__force u##bits)val; \ > > +} \ > > +static inline __virtio##bits cpu_to_virtio##bits(bool little_endian, u##bits val) \ > > +{ \ > > + if (little_endian) \ > > + return (__force __virtio##bits)cpu_to_le##bits(val); \ > > + else \ > > + return val; \ > > +} > > + > > +DEFINE_VIRTIO_XX_TO_CPU(16) > > +DEFINE_VIRTIO_XX_TO_CPU(32) > > +DEFINE_VIRTIO_XX_TO_CPU(64) > > + > > I'm usually not very fond of creating functions via macros like that as > it makes it hard to grep for them. > > > struct virtio_device; > > struct virtqueue; > > > > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h > > index a99f9b7..744cee1 100644 > > --- a/include/uapi/linux/virtio_ring.h > > +++ b/include/uapi/linux/virtio_ring.h > > @@ -33,6 +33,10 @@ > > * Copyright Rusty Russell IBM Corporation 2007. */ > > #include <linux/types.h> > > > > +typedef __u16 __bitwise __virtio16; > > +typedef __u32 __bitwise __virtio32; > > +typedef __u64 __bitwise __virtio64; > > + > > /* This marks a buffer as continuing via the next field. */ > > #define VRING_DESC_F_NEXT 1 > > /* This marks a buffer as write-only (otherwise read-only). */ > > @@ -61,32 +65,32 @@ > > /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ > > struct vring_desc { > > /* Address (guest-physical). */ > > - __u64 addr; > > + __virtio64 addr; > > /* Length. */ > > - __u32 len; > > + __virtio32 len; > > /* The flags as indicated above. */ > > - __u16 flags; > > + __virtio16 flags; > > /* We chain unused descriptors via this, too */ > > - __u16 next; > > + __virtio16 next; > > }; > > > > struct vring_avail { > > - __u16 flags; > > - __u16 idx; > > - __u16 ring[]; > > + __virtio16 flags; > > + __virtio16 idx; > > + __virtio16 ring[]; > > }; > > This looks weird. At least to me, that would scream "virtio endian", > which is only true for legacy devices. > > I understand that you want it to be statically checkable, but I think > it decreases readability. > > > > > /* u32 is used here for ids for padding reasons. */ > > struct vring_used_elem { > > /* Index of start of used descriptor chain. */ > > - __u32 id; > > + __virtio32 id; > > /* Total length of the descriptor chain which was used (written to) */ > > - __u32 len; > > + __virtio32 len; > > }; > > > > struct vring_used { > > - __u16 flags; > > - __u16 idx; > > + __virtio16 flags; > > + __virtio16 idx; > > struct vring_used_elem ring[]; > > }; > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 61a1fe1..a2f2f22 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -98,6 +98,8 @@ struct vring_virtqueue > > }; > > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq) > > +/* Will become vq->little_endian once we support virtio 1.0 */ > > +#define vq_le(vq) (false) > > All virtqueues inherit this property from their device, right? Do you > want to propagate this to the virtqueues if the guest negotiated > virtio-1 for the device? > > > > > static struct vring_desc *alloc_indirect(unsigned int total_sg, gfp_t gfp) > > { > > > @@ -235,13 +237,13 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > > > /* Put entry in available array (but don't update avail->idx until they > > * do sync). */ > > - avail = (vq->vring.avail->idx & (vq->vring.num-1)); > > - vq->vring.avail->ring[avail] = head; > > + avail = virtio16_to_cpu(vq_le(_vq), vq->vring.avail->idx) & (vq->vring.num - 1); > > + vq->vring.avail->ring[avail] = cpu_to_virtio16(vq_le(_vq), head); > > > > /* Descriptors and available array need to be set before we expose the > > * new available array entries. */ > > virtio_wmb(vq->weak_barriers); > > - vq->vring.avail->idx++; > > + vq->vring.avail->idx = cpu_to_virtio16(vq_le(_vq), virtio16_to_cpu(vq_le(_vq), vq->vring.avail->idx) + 1); > > I think this line looks awful :( > > I also notice that you need to call the converter functions with a > boolean, figuring out whether the queue is le or not in every caller. I > think passing in the device/virtqueue is a nicer interface. > > > vq->num_added++; > > > > /* This is very unlikely, but theoretically possible. Kick -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
