Re: [RFC v5 3/5] virtio_ring: add packed ring support

[Jason Wang]

On 2018å05æ29æ 13:11, Tiwei Bie wrote:
> On Tue, May 29, 2018 at 11:18:57AM +0800, Jason Wang wrote:
> > On 2018å05æ22æ 16:16, Tiwei Bie wrote:
> [...]
> > > +static void detach_buf_packed(struct vring_virtqueue *vq,
> > > +			      unsigned int id, void **ctx)
> > > +{
> > > +	struct vring_desc_state_packed *state;
> > > +	struct vring_packed_desc *desc;
> > > +	unsigned int i;
> > > +	int *next;
> > > +
> > > +	/* Clear data ptr. */
> > > +	vq->desc_state_packed[id].data = NULL;
> > > +
> > > +	next = &id;
> > > +	for (i = 0; i < vq->desc_state_packed[id].num; i++) {
> > > +		state = &vq->desc_state_packed[*next];
> > > +		vring_unmap_state_packed(vq, state);
> > > +		next = &vq->desc_state_packed[*next].next;
> > Have a short discussion with Michael. It looks like the only thing we care
> > is DMA address and length here.
> The length tracked by desc_state_packed[] is also necessary
> when INDIRECT is used and the buffers are writable.
>
> > So a thought is to avoid using desc_state_packed() is vring_use_dma_api() is
> > false? Probably need another id allocator or just use desc_state_packed for
> > free id list.
> I think it's a good suggestion. Thanks!
>
> I won't track the addr/len/flags for non-indirect descs
> when vring_use_dma_api() returns false.
>
> > > +	}
> > > +
> > > +	vq->vq.num_free += vq->desc_state_packed[id].num;
> > > +	*next = vq->free_head;
> > Using pointer seems not elegant and unnecessary here. You can just track
> > next in integer I think?
> It's possible to use integer, but will need one more var
> to track `prev` (desc_state_packed[prev].next == next in
> this case).

Yes, please do this.

> > > +	vq->free_head = id;
> > > +
> > > +	if (vq->indirect) {
> > > +		u32 len;
> > > +
> > > +		/* Free the indirect table, if any, now that it's unmapped. */
> > > +		desc = vq->desc_state_packed[id].indir_desc;
> > > +		if (!desc)
> > > +			return;
> > > +
> > > +		len = vq->desc_state_packed[id].len;
> > > +		for (i = 0; i < len / sizeof(struct vring_packed_desc); i++)
> > > +			vring_unmap_desc_packed(vq, &desc[i]);
> > > +
> > > +		kfree(desc);
> > > +		vq->desc_state_packed[id].indir_desc = NULL;
> > > +	} else if (ctx) {
> > > +		*ctx = vq->desc_state_packed[id].indir_desc;
> > > +	}
> > >    }
> > >    static inline bool more_used_packed(const struct vring_virtqueue *vq)
> > >    {
> > > -	return false;
> > > +	u16 last_used, flags;
> > > +	u8 avail, used;
> > > +
> > > +	last_used = vq->last_used_idx;
> > > +	flags = virtio16_to_cpu(vq->vq.vdev,
> > > +				vq->vring_packed.desc[last_used].flags);
> > > +	avail = !!(flags & VRING_DESC_F_AVAIL(1));
> > > +	used = !!(flags & VRING_DESC_F_USED(1));
> > > +
> > > +	return avail == used && used == vq->used_wrap_counter;
> > Spec does not check avail == used? So there's probably a bug in either of
> > the two places.
> >
> > In what condition that avail != used but used == vq_used_wrap_counter? A
> > buggy device or device set the two bits in two writes?
> Currently, spec doesn't forbid devices to set the status
> bits as: avail != used but used == vq_used_wrap_counter.
> So I think driver cannot assume this won't happen.

Right, but I could not find a situation that device need to something 
like this. We should either forbid this in the spec or change the 
example code in the spec.

Let's see how Michael think about this.

Thanks