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

[Jason Wang <jasowang@redhat.com>]

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@intel.com>
>>> ---
>>>    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