Re: [RFC v2 12/13] vdpa: preemptive kick at enable

[Si-Wei Liu]

On 1/13/2023 1:06 AM, Eugenio Perez Martin wrote:
> On Fri, Jan 13, 2023 at 4:39 AM Jason Wang <jasowang@redhat.com> wrote:
> > On Fri, Jan 13, 2023 at 11:25 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > > On 1/13/2023 10:31 AM, Jason Wang wrote:
> > > > On Fri, Jan 13, 2023 at 1:27 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > Spuriously kick the destination device's queue so it knows in case there
> > > > > are new descriptors.
> > > > >
> > > > > RFC: This is somehow a gray area. The guest may have placed descriptors
> > > > > in a virtqueue but not kicked it, so it might be surprised if the device
> > > > > starts processing it.
> > > > So I think this is kind of the work of the vDPA parent. For the parent
> > > > that needs this trick, we should do it in the parent driver.
> > > Agree, it looks easier implementing this in parent driver,
> > > I can implement it in ifcvf set_vq_ready right now
> > Great, but please check whether or not it is really needed.
> >
> > Some device implementation could check the available descriptions
> > after DRIVER_OK without waiting for a kick.
> So IIUC we can entirely drop this from the series (and I hope we can).
> But then, what with the devices that does *not* check for them?
I wonder how the kick can be missed from the first place. Supposedly the 
moment when vhost_dev_stop() calls .suspend() into vdpa driver, the 
vcpus already stopped running (vm_running = false) and all pending kicks 
are delivered through vhost-vdpa's host notifiers or mapped doorbell 
already then device won't get new ones. If the device intends to 
purposely ignore (note: this could be a device bug) pending kicks during 
.suspend(), then consequently it should check available descriptors 
after reaching driver_ok to process outstanding descriptors, making up 
for the missing kick. If the vdpa driver doesn't support .suspend(), 
then it should flush the work before .reset() - vhost-scsi does it this 
way.  Or otherwise I think it's the norm (right thing to do) device 
should process pending kicks before guest memory is to be unmapped at 
the late game of vhost_dev_stop(). Is there any case kicks may be missing?

-Siwei


> If we drop it it seems to me we must mandate devices to check for
> descriptors at queue_enable. The queue could stall if not, isn't it?
>
> Thanks!
>
> > Thanks
> >
> > > Thanks
> > > Zhu Lingshan
> > > > Thanks
> > > >
> > > > > However, that information is not in the migration stream and it should
> > > > > be an edge case anyhow, being resilient to parallel notifications from
> > > > > the guest.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >    hw/virtio/vhost-vdpa.c | 5 +++++
> > > > >    1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index 40b7e8706a..dff94355dd 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -732,11 +732,16 @@ static int vhost_vdpa_set_vring_ready(struct vhost_dev 
> > > > > *dev, int ready)
> > > > >        }
> > > > >        trace_vhost_vdpa_set_vring_ready(dev);
> > > > >        for (i = 0; i < dev->nvqs; ++i) {
> > > > > +        VirtQueue *vq;
> > > > >            struct vhost_vring_state state = {
> > > > >                .index = dev->vq_index + i,
> > > > >                .num = 1,
> > > > >            };
> > > > >            vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> > > > > +
> > > > > +        /* Preemptive kick */
> > > > > +        vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
> > > > > +        event_notifier_set(virtio_queue_get_host_notifier(vq));
> > > > >        }
> > > > >        return 0;
> > > > >    }
> > > > > --
> > > > > 2.31.1