Re: [Qemu-devel] [PATCH v2 4/4] vfio/quirks: Enable ioeventfd quirks to be handled by vfio directly

[Peter Xu]

On Thu, May 03, 2018 at 10:29:42AM -0600, Alex Williamson wrote:
> On Thu, 3 May 2018 12:56:03 +0800
> Peter Xu <address@hidden> wrote:
> 
> > On Tue, May 01, 2018 at 10:43:46AM -0600, Alex Williamson wrote:
> > 
> > [...]
> > 
> > > -static void vfio_ioeventfd_exit(VFIOIOEventFD *ioeventfd)
> > > +static void vfio_ioeventfd_exit(VFIOPCIDevice *vdev, VFIOIOEventFD 
> > > *ioeventfd)
> > >  {
> > >      QLIST_REMOVE(ioeventfd, next);
> > > +
> > >      memory_region_del_eventfd(ioeventfd->mr, ioeventfd->addr, 
> > > ioeventfd->size,
> > >                                ioeventfd->match_data, ioeventfd->data,
> > >                                &ioeventfd->e);
> > > -    qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), NULL, 
> > > NULL, NULL);
> > > +
> > > +    if (ioeventfd->vfio) {
> > > +        struct vfio_device_ioeventfd vfio_ioeventfd;
> > > +
> > > +        vfio_ioeventfd.argsz = sizeof(vfio_ioeventfd);
> > > +        vfio_ioeventfd.flags = ioeventfd->size;
> > > +        vfio_ioeventfd.data = ioeventfd->data;
> > > +        vfio_ioeventfd.offset = ioeventfd->region->fd_offset +
> > > +                                ioeventfd->region_addr;
> > > +        vfio_ioeventfd.fd = -1;
> > > +
> > > +        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_IOEVENTFD, 
> > > &vfio_ioeventfd);  
> > 
> > (If the series is going to respin, I am thinking whether it would
> >  worth it to capture this error to dump something.  But it's for sure
> >  optional since even error happened we should have something in dmesg
> >  so it only matters on whether we also want something to be dumped
> >  from QEMU side too... After all there aren't much we can do)
> 
> I'm torn whether to use QEMU standard error handling here, ie.
> abort().  If we failed to remove the KVM ioeventfd, we'd abort before
> we get here, so there's no chance that the vfio ioeventfd will continue
> to be triggered.  Obviously leaving a vfio ioeventfd that we can't
> trigger and might interfere with future ioeventfds is not good, but do
> we really want to kill the VM because we possibly can't add an
> accelerator here later?  I'm inclined to say no, so I think I'll just
> error_report() unless there are objections.

I must be misleading when I said "dump something"... :) Yes the
error_report is exactly what I meant.

(Even an "error_report_once" but we don't have that yet)

> 
> > > +
> > > +    } else {
> > > +        qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e),
> > > +                            NULL, NULL, NULL);
> > > +    }
> > > +
> > >      event_notifier_cleanup(&ioeventfd->e);
> > >      trace_vfio_ioeventfd_exit(memory_region_name(ioeventfd->mr),
> > >                                (uint64_t)ioeventfd->addr, 
> > > ioeventfd->size,  
> > 
> > [...]
> > 
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index ba1239551115..84e27c7bb2d1 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -3177,6 +3177,8 @@ static Property vfio_pci_dev_properties[] = {
> > >                       no_geforce_quirks, false),
> > >      DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", VFIOPCIDevice, 
> > > no_kvm_ioeventfd,
> > >                       false),
> > > +    DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", VFIOPCIDevice, 
> > > no_vfio_ioeventfd,
> > > +                     false),  
> > 
> > Here it looks more like a tri-state for me, so we can either:
> > 
> > - disable the acceleration in general, or
> > - enable QEMU-side acceleration only, or
> > - enable kernel-side acceleration
> 
> So you're looking for a Auto/Off/KVM-only option?  Do you really think
> it's worth defining a new tristate property for this sort of debugging
> option...
> 
> > In other words, IIUC x-no-vfio-ioeventfd won't matter much if
> > x-no-kvm-ioeventfd is already set.  So not sure whether a single
> > parameter would be nicer.
> 
> That's correct, but who do we expect to be using this option and why?
> I added enum OffAutoPCIBAR and the property to enable it for MSI-x
> relocation because it is an option that a normal user might reasonably
> need to use, given the right hardware on the right host, but it's an
> unsupported option because we cannot programatically validate it.
> Support rests with the individual user, if it doesn't work, don't use
> it, if it helps, great.  Here we have options that are really only for
> debugging, to test whether something has gone wrong in this code,
> disable this bypass to make all device interactions visible through
> QEMU, or specifically to evaluate the performance of this path.  Is it
> reasonable to impose yet another property type on the common code for
> this use case when a couple bools work just fine, if perhaps not
> absolutely ideal?  Am I overlooking an existing tri-state that might be
> a reasonable match?

Oh so it's only for debugging.  Then I would be perfectly fine with
two parameters.

Actually I wasn't thinking about any tri-state property, I was
thinking about e.g. string-typed that can satisfy things like
tri-state.  But again now I don't think it'll worth it to repost with
that if only for debugging purpose.

Thanks!

-- 
Peter Xu