On Thu, Mar 11, 2021 at 07:53:53PM +0100, Eugenio Perez Martin wrote:
> On Fri, Jan 22, 2021 at 7:18 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Dec 10, 2020 at 12:55 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Wed, Dec 09, 2020 at 07:41:23PM +0100, Eugenio Perez Martin wrote:
> > > > On Tue, Dec 8, 2020 at 9:16 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > > > On Fri, Nov 20, 2020 at 07:50:51PM +0100, Eugenio Pérez wrote:
> > > > > > +        while (true) {
> > > > > > +            int r;
> > > > > > +            if (virtio_queue_full(vq)) {
> > > > > > +                break;
> > > > > > +            }
> > > > >
> > > > > Why is this check necessary? The guest cannot provide more descriptors
> > > > > than there is ring space. If that happens somehow then it's a driver
> > > > > error that is already reported in virtqueue_pop() below.
> > > > >
> > > >
> > > > It's just checked because virtqueue_pop prints an error on that case,
> > > > and there is no way to tell the difference between a regular error and
> > > > another caused by other causes. Maybe the right thing to do is just to
> > > > not to print that error? Caller should do the error printing in that
> > > > case. Should we return an error code?
> > >
> > > The reason an error is printed today is because it's a guest error that
> > > never happens with correct guest drivers. Something is broken and the
> > > user should know about it.
> > >
> > > Why is "virtio_queue_full" (I already forgot what that actually means,
> > > it's not clear whether this is referring to avail elements or used
> > > elements) a condition that should be silently ignored in shadow vqs?
> > >
> >
> > TL;DR: It can be changed to a check of the number of available
> > descriptors in shadow vq, instead of returning as a regular operation.
> > However, I think that making it a special return of virtqueue_pop
> > could help in devices that run to completion, avoiding having to
> > duplicate the count logic in them.
> >
> > The function virtio_queue_empty checks if the vq has all descriptors
> > available, so the device has no more work to do until the driver makes
> > another descriptor available. I can see how it can be a bad name
> > choice, but virtio_queue_full means the opposite: device has pop()
> > every descriptor available, and it has not returned any, so the driver
> > cannot progress until the device marks some descriptors as used.
> >
> > As I understand, if vq->in_use >vq->num would mean we have a bug in
> > the device vq code, not in the driver. virtio_queue_full could even be
> > changed to "assert(vq->inuse <= vq->vring.num); return vq->inuse ==
> > vq->vring.num", as long as vq->in_use is operated right.
> >
> > If we hit vq->in_use == vq->num in virtqueue_pop it means the device
> > tried to pop() one more buffer after having all of them available and
> > pop'ed. This would be invalid if the device is counting right the
> > number of in_use descriptors, but then we are duplicating that logic
> > in the device and the vq.

Devices call virtqueue_pop() until it returns NULL. They don't need to
count virtqueue buffers explicitly. It returns NULL when vq->num
virtqueue buffers have already been popped (either because
virtio_queue_empty() is true or because an invalid driver state is
detected by checking vq->num in virtqueue_pop()).

> > In shadow vq this situation happens with the correct guest network
> > driver, since the rx queue is filled for the device to write. Network
> > device in qemu fetch descriptors on demand, but shadow vq fetch all
> > available in batching. If the driver just happens to fill the queue of
> > available descriptors, the log will raise, so we need to check in
> > handle_sw_lm_vq before calling pop(). Of course the shadow vq can
> > duplicate guest_vq->in_use instead of checking virtio_queue_full, but
> > then it needs to check two things for every virtqueue_pop() [1].

I don't understand this scenario. It sounds like you are saying the
guest and shadow rx vq are not in sync so there is a case where
vq->in_use > vq->num is triggered? I'm not sure how that can happen
since both vqs have equal vq->num. Maybe you can explain the scenario in
more detail?

Stefan