Re: [RFC v2] virtio: support packed ring

[Tiwei Bie]

On Tue, Apr 17, 2018 at 06:54:51PM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 17, 2018 at 10:56:26PM +0800, Tiwei Bie wrote:
> > On Tue, Apr 17, 2018 at 05:04:59PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 17, 2018 at 08:47:16PM +0800, Tiwei Bie wrote:
> > > > On Tue, Apr 17, 2018 at 03:17:41PM +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Apr 17, 2018 at 10:51:33AM +0800, Tiwei Bie wrote:
> > > > > > On Tue, Apr 17, 2018 at 10:11:58AM +0800, Jason Wang wrote:
> > > > > > > On 2018å04æ13æ 15:15, Tiwei Bie wrote:
> > > > > > > > On Fri, Apr 13, 2018 at 12:30:24PM +0800, Jason Wang wrote:
> > > > > > > > > On 2018å04æ01æ 22:12, Tiwei Bie wrote:
> > > > > > [...]
> > > > > > > > > > +static int detach_buf_packed(struct vring_virtqueue *vq, unsigned int head,
> > > > > > > > > > +			      void **ctx)
> > > > > > > > > > +{
> > > > > > > > > > +	struct vring_packed_desc *desc;
> > > > > > > > > > +	unsigned int i, j;
> > > > > > > > > > +
> > > > > > > > > > +	/* Clear data ptr. */
> > > > > > > > > > +	vq->desc_state[head].data = NULL;
> > > > > > > > > > +
> > > > > > > > > > +	i = head;
> > > > > > > > > > +
> > > > > > > > > > +	for (j = 0; j < vq->desc_state[head].num; j++) {
> > > > > > > > > > +		desc = &vq->vring_packed.desc[i];
> > > > > > > > > > +		vring_unmap_one_packed(vq, desc);
> > > > > > > > > > +		desc->flags = 0x0;
> > > > > > > > > Looks like this is unnecessary.
> > > > > > > > It's safer to zero it. If we don't zero it, after we
> > > > > > > > call virtqueue_detach_unused_buf_packed() which calls
> > > > > > > > this function, the desc is still available to the
> > > > > > > > device.
> > > > > > > 
> > > > > > > Well detach_unused_buf_packed() should be called after device is stopped,
> > > > > > > otherwise even if you try to clear, there will still be a window that device
> > > > > > > may use it.
> > > > > > 
> > > > > > This is not about whether the device has been stopped or
> > > > > > not. We don't have other places to re-initialize the ring
> > > > > > descriptors and wrap_counter. So they need to be set to
> > > > > > the correct values when doing detach_unused_buf.
> > > > > > 
> > > > > > Best regards,
> > > > > > Tiwei Bie
> > > > > 
> > > > > find vqs is the time to do it.
> > > > 
> > > > The .find_vqs() will call .setup_vq() which will eventually
> > > > call vring_create_virtqueue(). It's a different case. Here
> > > > we're talking about re-initializing the descs and updating
> > > > the wrap counter when detaching the unused descs (In this
> > > > case, split ring just needs to decrease vring.avail->idx).
> > > > 
> > > > Best regards,
> > > > Tiwei Bie
> > > 
> > > There's no requirement that  virtqueue_detach_unused_buf re-initializes
> > > the descs. It happens on cleanup path just before drivers delete the
> > > vqs.
> > 
> > Cool, I wasn't aware of it. I saw split ring decrease
> > vring.avail->idx after detaching an unused desc, so I
> > thought detaching unused desc also needs to make sure
> > that the ring state will be updated correspondingly.
> 
> 
> Hmm. You are right. Seems to be out console driver being out of spec.
> Will have to look at how to fix that :(
> 
> It was done here:
> 
> Commit b3258ff1d6086bd2b9eeb556844a868ad7d49bc8
> Author: Amit Shah <amit.shah@xxxxxxxxxx>
> Date:   Wed Mar 16 19:12:10 2011 +0530
> 
>     virtio: Decrement avail idx on buffer detach
>     
>     When detaching a buffer from a vq, the avail.idx value should be
>     decremented as well.
>     
>     This was noticed by hot-unplugging a virtio console port and then
>     plugging in a new one on the same number (re-using the vqs which were
>     just 'disowned').  qemu reported
>     
>        'Guest moved used index from 0 to 256'
>     
>     when any IO was attempted on the new port.
>     
>     CC: stable@xxxxxxxxxx
>     Reported-by: juzhang <juzhang@xxxxxxxxxx>
>     Signed-off-by: Amit Shah <amit.shah@xxxxxxxxxx>
>     Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> 
> The spec is quite explicit though:
> 	A driver MUST NOT decrement the available idx on a live virtqueue (ie. there is no way to âunexposeâ
> 	buffers).
> 

Hmm.. Got it. Thanks!

Best regards,
Tiwei Bie


> 
> 
> 
> 
> > If there is no such requirement, do you think it's OK
> > to remove below two lines:
> > 
> > vq->avail_idx_shadow--;
> > vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
> > 
> > from virtqueue_detach_unused_buf(), and we could have
> > one generic function to handle both rings:
> > 
> > void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
> > {
> > 	struct vring_virtqueue *vq = to_vvq(_vq);
> > 	unsigned int num, i;
> > 	void *buf;
> > 
> > 	START_USE(vq);
> > 
> > 	num = vq->packed ? vq->vring_packed.num : vq->vring.num;
> > 
> > 	for (i = 0; i < num; i++) {
> > 		if (!vq->desc_state[i].data)
> > 			continue;
> > 		/* detach_buf clears data, so grab it now. */
> > 		buf = vq->desc_state[i].data;
> > 		detach_buf(vq, i, NULL);
> > 		END_USE(vq);
> > 		return buf;
> > 	}
> > 	/* That should have freed everything. */
> > 	BUG_ON(vq->vq.num_free != num);
> > 
> > 	END_USE(vq);
> > 	return NULL;
> > }
> > 
> > Best regards,
> > Tiwei Bie