Re: [RFC v5 2/5] virtio_ring: support creating packed ring

[Jason Wang]

On 2018å05æ29æ 13:24, Tiwei Bie wrote:
> On Tue, May 29, 2018 at 10:49:11AM +0800, Jason Wang wrote:
> > On 2018å05æ22æ 16:16, Tiwei Bie wrote:
> > > This commit introduces the support for creating packed ring.
> > > All split ring specific functions are added _split suffix.
> > > Some necessary stubs for packed ring are also added.
> > >
> > > Signed-off-by: Tiwei Bie <tiwei.bie@xxxxxxxxx>
> > > ---
> > >    drivers/virtio/virtio_ring.c | 801 +++++++++++++++++++++++------------
> > >    include/linux/virtio_ring.h  |   8 +-
> > >    2 files changed, 546 insertions(+), 263 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 71458f493cf8..f5ef5f42a7cf 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -61,11 +61,15 @@ struct vring_desc_state {
> > >    	struct vring_desc *indir_desc;	/* Indirect descriptor, if any. */
> > >    };
> > > +struct vring_desc_state_packed {
> > > +	int next;			/* The next desc state. */
> > > +};
> > > +
> > >    struct vring_virtqueue {
> > >    	struct virtqueue vq;
> > > -	/* Actual memory layout for this queue */
> > > -	struct vring vring;
> > > +	/* Is this a packed ring? */
> > > +	bool packed;
> > >    	/* Can we use weak barriers? */
> > >    	bool weak_barriers;
> > > @@ -87,11 +91,39 @@ struct vring_virtqueue {
> > >    	/* Last used index we've seen. */
> > >    	u16 last_used_idx;
> > > -	/* Last written value to avail->flags */
> > > -	u16 avail_flags_shadow;
> > > +	union {
> > > +		/* Available for split ring */
> > > +		struct {
> > > +			/* Actual memory layout for this queue. */
> > > +			struct vring vring;
> > > -	/* Last written value to avail->idx in guest byte order */
> > > -	u16 avail_idx_shadow;
> > > +			/* Last written value to avail->flags */
> > > +			u16 avail_flags_shadow;
> > > +
> > > +			/* Last written value to avail->idx in
> > > +			 * guest byte order. */
> > > +			u16 avail_idx_shadow;
> > > +		};
> > > +
> > > +		/* Available for packed ring */
> > > +		struct {
> > > +			/* Actual memory layout for this queue. */
> > > +			struct vring_packed vring_packed;
> > > +
> > > +			/* Driver ring wrap counter. */
> > > +			u8 avail_wrap_counter;
> > > +
> > > +			/* Device ring wrap counter. */
> > > +			u8 used_wrap_counter;
> > How about just use boolean?
> I can change it to bool if you want.

Yes, please.

> [...]
> > > -static int vring_mapping_error(const struct vring_virtqueue *vq,
> > > -			       dma_addr_t addr)
> > > -{
> > > -	if (!vring_use_dma_api(vq->vq.vdev))
> > > -		return 0;
> > > -
> > > -	return dma_mapping_error(vring_dma_dev(vq), addr);
> > > -}
> > It looks to me if you keep vring_mapping_error behind
> > vring_unmap_one_split(), lots of changes were unncessary.
> [...]
> > > +	}
> > > +	/* That should have freed everything. */
> > > +	BUG_ON(vq->vq.num_free != vq->vring.num);
> > > +
> > > +	END_USE(vq);
> > > +	return NULL;
> > > +}
> > I think the those copy-and-paste hunks could be avoided and the diff should
> > only contains renaming of the function. If yes, it would be very welcomed
> > since it requires to compare the changes verbatim otherwise.
> Michael suggested to lay out the code as:
>
> XXX_split
>
> XXX_packed
>
> XXX wrappers
>
> https://lkml.org/lkml/2018/4/13/410
>
> That's why I moved some code.

I see, then no need to change but it still looks unnecessary.

> > > +
> > > +/*
> > > + * The layout for the packed ring is a continuous chunk of memory
> > > + * which looks like this.
> > > + *
> > > + * struct vring_packed {
> > > + *	// The actual descriptors (16 bytes each)
> > > + *	struct vring_packed_desc desc[num];
> > > + *
> > > + *	// Padding to the next align boundary.
> > > + *	char pad[];
> > > + *
> > > + *	// Driver Event Suppression
> > > + *	struct vring_packed_desc_event driver;
> > > + *
> > > + *	// Device Event Suppression
> > > + *	struct vring_packed_desc_event device;
> > > + * };
> > > + */
> > > +static inline void vring_init_packed(struct vring_packed *vr, unsigned int num,
> > > +				     void *p, unsigned long align)
> > > +{
> > > +	vr->num = num;
> > > +	vr->desc = p;
> > > +	vr->driver = (void *)(((uintptr_t)p + sizeof(struct vring_packed_desc)
> > > +		* num + align - 1) & ~(align - 1));
> > If we choose not to go uapi, maybe we can use ALIGN() macro here?
> Okay.
>
> > > +	vr->device = vr->driver + 1;
> > > +}
> [...]
> > > +/* Only available for split ring */
> > >    const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> > >    {
> > A possible issue with this is:
> >
> > After commit d4674240f31f8c4289abba07d64291c6ddce51bc ("KVM: s390:
> > virtio-ccw revision 1 SET_VQ"). CCW tries to use
> > virtqueue_get_avail()/virtqueue_get_used(). Looks like a bug either here or
> > ccw code.
> Do we still need to support:
>
> include/linux/virtio.h
> /*
>   * Legacy accessors -- in almost all cases, these are the wrong functions
>   * to use.
>   */
> static inline void *virtqueue_get_desc(struct virtqueue *vq)
> {
>          return virtqueue_get_vring(vq)->desc;
> }
> static inline void *virtqueue_get_avail(struct virtqueue *vq)
> {
>          return virtqueue_get_vring(vq)->avail;
> }
> static inline void *virtqueue_get_used(struct virtqueue *vq)
> {
>          return virtqueue_get_vring(vq)->used;
> }
>
> in packed ring?

I think it was probably a bug in ccw, they should use e.g 
virtqueue_get_desc_addr() instead.

Thanks

> If so, I think maybe it's better to expose them as
> symbols and implement them in virtio_ring.c.
>
> Best regards,
> Tiwei Bie