Message-ID: <20180912120900-mutt-send-email-mst@kernel.org>
Date:   Wed, 12 Sep 2018 12:12:48 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Tiwei Bie <tiwei.bie@...el.com>
Cc:     jasowang@...hat.com, virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        virtio-dev@...ts.oasis-open.org, wexu@...hat.com,
        jfreimann@...hat.com
Subject: Re: [PATCH net-next v2 2/5] virtio_ring: support creating packed ring

On Wed, Sep 12, 2018 at 03:51:40PM +0800, Tiwei Bie wrote:
> On Mon, Sep 10, 2018 at 10:28:37AM +0800, Tiwei Bie wrote:
> > On Fri, Sep 07, 2018 at 10:03:24AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Jul 11, 2018 at 10:27:08AM +0800, 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@...el.com>
> > > 
> [...]
> > > > +
> > > > +/*
> > > > + * 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;
> > > > + * };
> > > > + */
> > > 
> > > Why not just allocate event structures separately?
> > > Is it a win to have them share a cache line for some reason?
> 
> Users may call vring_new_virtqueue() with preallocated
> memory to setup the ring, e.g.:
> 
> https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/s390/virtio/virtio_ccw.c#L513-L522
> https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/misc/mic/vop/vop_main.c#L306-L307
> 
> Below is the corresponding definition in split ring:
> 
> https://github.com/oasis-tcs/virtio-spec/blob/89dd55f5e606/split-ring.tex#L64-L78
> 
> If my understanding is correct, this is just for legacy
> interfaces, and we won't define layout in packed ring
> and don't need to support vring_new_virtqueue() in packed
> ring. Is it correct? Thanks!

mic doesn't support 1.0 yet but ccw does.

It's probably best to look into converting ccw to
virtio_create_virtqueue, then you can just fail
vring_new_virtqueue for packed.

Cornelia, are there any gotchas to look out for?

> 
> 
> > 
> > Will do that.
> > 
> > > 
> > > > +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 *)ALIGN(((uintptr_t)p +
> > > > +		sizeof(struct vring_packed_desc) * num), align);
> > > > +	vr->device = vr->driver + 1;
> > > > +}
> > > 
> > > What's all this about alignment? Where does it come from?
> > 
> > It comes from the `vring_align` parameter of vring_create_virtqueue()
> > and vring_new_virtqueue():
> > 
> > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1061
> > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1123
> > 
> > Should I just ignore it in packed ring?
> > 
> > CCW defined this:
> > 
> > #define KVM_VIRTIO_CCW_RING_ALIGN 4096
> > 
> > I'm not familiar with CCW. Currently, in this patch set, packed ring
> > isn't enabled on CCW dues to some legacy accessors are not implemented
> > in packed ring yet.
> > 
> > > 
> > > > +
> > > > +static inline unsigned vring_size_packed(unsigned int num, unsigned long align)
> > > > +{
> > > > +	return ((sizeof(struct vring_packed_desc) * num + align - 1)
> > > > +		& ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2;
> > > > +}
> > [...]
> > > > @@ -1129,10 +1388,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
> > > >  				      void (*callback)(struct virtqueue *vq),
> > > >  				      const char *name)
> > > >  {
> > > > -	struct vring vring;
> > > > -	vring_init(&vring, num, pages, vring_align);
> > > > -	return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
> > > > -				     notify, callback, name);
> > > > +	union vring_union vring;
> > > > +	bool packed;
> > > > +
> > > > +	packed = virtio_has_feature(vdev, VIRTIO_F_RING_PACKED);
> > > > +	if (packed)
> > > > +		vring_init_packed(&vring.vring_packed, num, pages, vring_align);
> > > > +	else
> > > > +		vring_init(&vring.vring_split, num, pages, vring_align);
> > > 
> > > 
> > > vring_init in the UAPI header is more or less a bug.
> > > I'd just stop using it, keep it around for legacy userspace.
> > 
> > Got it. I'd like to do that. Thanks.
> > 
> > > 
> > > > +
> > > > +	return __vring_new_virtqueue(index, vring, packed, vdev, weak_barriers,
> > > > +				     context, notify, callback, name);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(vring_new_virtqueue);
> > > >  
> > > > @@ -1142,7 +1408,9 @@ void vring_del_virtqueue(struct virtqueue *_vq)
> > > >  
> > > >  	if (vq->we_own_ring) {
> > > >  		vring_free_queue(vq->vq.vdev, vq->queue_size_in_bytes,
> > > > -				 vq->vring.desc, vq->queue_dma_addr);
> > > > +				 vq->packed ? (void *)vq->vring_packed.desc :
> > > > +					      (void *)vq->vring.desc,
> > > > +				 vq->queue_dma_addr);
> > > >  	}
> > > >  	list_del(&_vq->list);
> > > >  	kfree(vq);
> > > > @@ -1184,7 +1452,7 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> > > >  
> > > >  	struct vring_virtqueue *vq = to_vvq(_vq);
> > > >  
> > > > -	return vq->vring.num;
> > > > +	return vq->packed ? vq->vring_packed.num : vq->vring.num;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
> > > >  
> > > > @@ -1227,6 +1495,10 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> > > >  
> > > >  	BUG_ON(!vq->we_own_ring);
> > > >  
> > > > +	if (vq->packed)
> > > > +		return vq->queue_dma_addr + ((char *)vq->vring_packed.driver -
> > > > +				(char *)vq->vring_packed.desc);
> > > > +
> > > >  	return vq->queue_dma_addr +
> > > >  		((char *)vq->vring.avail - (char *)vq->vring.desc);
> > > >  }
> > > > @@ -1238,11 +1510,16 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> > > >  
> > > >  	BUG_ON(!vq->we_own_ring);
> > > >  
> > > > +	if (vq->packed)
> > > > +		return vq->queue_dma_addr + ((char *)vq->vring_packed.device -
> > > > +				(char *)vq->vring_packed.desc);
> > > > +
> > > >  	return vq->queue_dma_addr +
> > > >  		((char *)vq->vring.used - (char *)vq->vring.desc);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
> > > >  
> > > > +/* Only available for split ring */
> > > >  const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> > > >  {
> > > >  	return &to_vvq(vq)->vring;
> > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > > > index fab02133a919..992142b35f55 100644
> > > > --- a/include/linux/virtio_ring.h
> > > > +++ b/include/linux/virtio_ring.h
> > > > @@ -60,6 +60,11 @@ static inline void virtio_store_mb(bool weak_barriers,
> > > >  struct virtio_device;
> > > >  struct virtqueue;
> > > >  
> > > > +union vring_union {
> > > > +	struct vring vring_split;
> > > > +	struct vring_packed vring_packed;
> > > > +};
> > > > +
> > > >  /*
> > > >   * Creates a virtqueue and allocates the descriptor ring.  If
> > > >   * may_reduce_num is set, then this may allocate a smaller ring than
> > > > @@ -79,7 +84,8 @@ struct virtqueue *vring_create_virtqueue(unsigned int index,
> > > >  
> > > >  /* Creates a virtqueue with a custom layout. */
> > > >  struct virtqueue *__vring_new_virtqueue(unsigned int index,
> > > > -					struct vring vring,
> > > > +					union vring_union vring,
> > > > +					bool packed,
> > > >  					struct virtio_device *vdev,
> > > >  					bool weak_barriers,
> > > >  					bool ctx,
> > > > -- 
> > > > 2.18.0

Powered by blists - more mailing lists