Re: [RFC v3 0/5] virtio: support packed ring

[Jason Wang]

On 2018å04æ27æ 17:12, Tiwei Bie wrote:
> On Fri, Apr 27, 2018 at 02:17:51PM +0800, Jason Wang wrote:
> > On 2018å04æ27æ 12:18, Michael S. Tsirkin wrote:
> > > On Fri, Apr 27, 2018 at 11:56:05AM +0800, Jason Wang wrote:
> > > > On 2018å04æ25æ 13:15, Tiwei Bie wrote:
> > > > > Hello everyone,
> > > > >
> > > > > This RFC implements packed ring support in virtio driver.
> > > > >
> > > > > Some simple functional tests have been done with Jason's
> > > > > packed ring implementation in vhost:
> > > > >
> > > > > https://lkml.org/lkml/2018/4/23/12
> > > > >
> > > > > Both of ping and netperf worked as expected (with EVENT_IDX
> > > > > disabled). But there are below known issues:
> > > > >
> > > > > 1. Reloading the guest driver will break the Tx/Rx;
> > > > Will have a look at this issue.
> > > >
> > > > > 2. Zeroing the flags when detaching a used desc will
> > > > >       break the guest -> host path.
> > > > I still think zeroing flags is unnecessary or even a bug. At host, I track
> > > > last observed avail wrap counter and detect avail like (what is suggested in
> > > > the example code in the spec):
> > > >
> > > > static bool desc_is_avail(struct vhost_virtqueue *vq, __virtio16 flags)
> > > > {
> > > >   ÂÂÂÂÂÂ bool avail = flags & cpu_to_vhost16(vq, DESC_AVAIL);
> > > >
> > > >   ÂÂÂÂÂÂ return avail == vq->avail_wrap_counter;
> > > > }
> > > >
> > > > So zeroing wrap can not work with this obviously.
> > > >
> > > > Thanks
> > > I agree. I think what one should do is flip the available bit.
> > But is this flipping a must?
> >
> > Thanks
> Yeah, that's my question too. It seems to be a requirement
> for driver that, the only change to the desc status that a
> driver can do during running is to mark the desc as avail,
> and any other changes to the desc status are not allowed.
> Similarly, the device can only mark the desc as used, and
> any other changes to the desc status are also not allowed.
> So the question is, are there such requirements?

Looks not, but I think we need clarify this in the spec.

Thanks

> Based on below contents in the spec:
>
> """
> Thus VIRTQ_DESC_F_AVAIL and VIRTQ_DESC_F_USED bits are different
> for an available descriptor and equal for a used descriptor.
>
> Note that this observation is mostly useful for sanity-checking
> as these are necessary but not sufficient conditions
> """
>
> It seems that, it's necessary for devices to check whether
> the AVAIL bit and USED bit are different.
>
> Best regards,
> Tiwei Bie