Re: [Qemu-devel] [PATCH v1 4/5] s390x/pci: Refuse to realize VFIO-PCI if AIS needed but supported

[Pierre Morel]

On 09/10/2017 16:45, Alex Williamson wrote:
> On Wed,  4 Oct 2017 15:49:38 +0200
> Pierre Morel <address@hidden> wrote:
>
> > In S390x the Adapter Interrupt Suppression facility is used to mask
> > interrupts of other PCI devices during interruption handling.
> >
> > VFIO PCI allows the interrupts to be delivered rapidely through KVM via
> > IRQfd or to be delivered through QEMU.
> > The choice is made through the x-kvm-intx and x-kvo-misx properties of
> > the VFIO PCI device.
> >
> > If the VFIO PCI device is using the direct KVM access through IRQfd and
> > we know that KVM does not implement AIS support we refuse to realize the
> > VFIO PCI device.
> >
> > In all other cases, emulation and VFIO PCI sending interrupts through
> > QEMU, we intercept the propagated IRQ, and protect it with the QEMU AIS
> > implementation before to send it to the guest through KVM.
> >
> > Signed-off-by: Pierre Morel <address@hidden>
> > ---
> >   hw/s390x/s390-pci-bus.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index d9c294a..4afe49b 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -21,18 +21,21 @@
> >   #include "hw/pci/pci_bus.h"
> >   #include "hw/pci/pci_bridge.h"
> >   #include "hw/pci/msi.h"
> > +#include "hw/vfio/pci.h"
> >   #include "qemu/error-report.h"
> >   
> >   #ifndef DEBUG_S390PCI_BUS
> >   #define DEBUG_S390PCI_BUS  0
> >   #endif
> >   
> > +#ifndef DPRINTF
> >   #define DPRINTF(fmt, ...)                                         \
> >       do {                                                          \
> >           if (DEBUG_S390PCI_BUS) {                                  \
> >               fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \
> >           }                                                         \
> >       } while (0)
> > +#endif
>
> Maybe a sign we shouldn't be including vfio/pci.h
>
> >   S390pciState *s390_get_phb(void)
> >   {
> > @@ -751,6 +754,15 @@ static void s390_pcihost_hot_plug(HotplugHandler 
> > *hotplug_dev,
> >           }
> >   
> >           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> > +            S390FLICState *fs = s390_get_flic();
> > +            VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > +
> > +            if ((!vdev->no_kvm_msix || !vdev->no_kvm_msix) &&
> > +                (!fs || !fs->ais_supported)) {
> > +                error_setg(errp, "VFIO PCI is not supported "
> > +                           "because kernel has no AIS capability.");
> > +                return;
> > +            }
>
>
> Hmm, you're basically looking at private data structure fields for
> experimental disable flags, which no user, or more importantly no
> management tool, could rightfully be expected to provide.  Can we not
> take this into account to automatically do the right thing, ie.
> automatically set the necessary flag during the device realize?  Thanks,

I am not sure to understand.

Here we are in a s390x specific device and without vfio/pci.h, we have 
no access to the PCI VFIOdevice structure from the realize function.

Do you mean to add an entry in the VFIOPCIdevice realize?
A new quirk ?
An architecture callback, because we also need access to S390FLICState ?


We also have the possibility to not fall-back to AIS emulation for VFIO
but we would loose the possibility to migrate to a host not supporting AIS.


Thanks for your help.

Pierre


> Alex


--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany