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" <[email protected]> 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 <[email protected]>
> >
> > ---
> >
> > 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
