Re: [Qemu-devel] [PATCH qemu] vfio: Allow configuration without INTx

[Alex Williamson]

On Thu, 30 Nov 2017 13:08:17 +1100
Alexey Kardashevskiy <address@hidden> wrote:

> On 30/11/17 02:33, Alex Williamson wrote:
> > On Wed, 22 Nov 2017 16:16:49 +1100
> > Alexey Kardashevskiy <address@hidden> wrote:
> >   
> >> On some platforms INTx may not be enabled on a KVM host (one such
> >> example is IBM pHyp hypervisor and this is intentional). However
> >> the PCI_INTERRUPT_PIN is not 0 so QEMU tries initializing INTx, fails as
> >> (!vdev->pdev->irq) in the VFIO's vfio_intx_enable() and this is
> >> a fatal error.
> >>
> >> This adds a debug switch - "x-no-intx" - in order to allow broken INTx
> >> configuration.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> >> ---
> >>
> >> In practice, test teams run PR KVM under HV KVM and there INTx is enabled
> >> on all levels so having this as a debug switch is enough.
> >> ---
> >>  hw/vfio/pci.h | 1 +
> >>  hw/vfio/pci.c | 8 +++++++-
> >>  2 files changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> >> index 502a575..c5e168e 100644
> >> --- a/hw/vfio/pci.h
> >> +++ b/hw/vfio/pci.h
> >> @@ -141,6 +141,7 @@ typedef struct VFIOPCIDevice {
> >>      bool has_flr;
> >>      bool has_pm_reset;
> >>      bool rom_read_failed;
> >> +    bool no_intx;
> >>      bool no_kvm_intx;
> >>      bool no_kvm_msi;
> >>      bool no_kvm_msix;
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index c977ee3..c9caf6a 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -2869,7 +2869,12 @@ static void vfio_realize(PCIDevice *pdev, Error 
> >> **errp)
> >>          pci_device_set_intx_routing_notifier(&vdev->pdev, 
> >> vfio_intx_update);
> >>          ret = vfio_intx_enable(vdev, errp);
> >>          if (ret) {
> >> -            goto out_teardown;
> >> +            if (vdev->no_intx) {
> >> +                error_report_err(*errp);
> >> +                *errp = NULL;
> >> +            } else {
> >> +                goto out_teardown;
> >> +            }  
> > 
> > Why do we try to setup INTx and let it fail, potentially with errors to
> > the log, if the user has specified x-no-intx?  
> 
> Fair enough, could just avoid the whole intx logic in this case.
> 
> > Why does the kernel
> > advertise INTx is available via the pin register if it's not
> > supported?   
> 
> It is a host under phyp (the other IBM hypervisor) so it takes the
> interrupt configuragion from the device tree or RTAS services (for MSI),
> not from the config space so if there is some inconsistency - then it is
> pHyp to blame.

The PCI specification provides that if a device reports zero in the PCI
pin register in configuration space, it indicates that the device does
not support INTx.  It doesn't really matter who is to blame, if the
host where the vfio kernel side lives can figure out that INTx is not
supported then it should be masking the pin register to expose that to
the user.  A user can't be expected to know or care which hypervisor
does what to make this not work.  Thanks,

Alex
 
> > I'm not opposed to a debug flag to disable INTx, but by
> > definition, "x-" prefix flags are not supported and it seems like you
> > have a configuration that needs a supported mechanism of turning this
> > off.  Thanks,  
> 
> Well, running VFIO under PR KVM under pHyp is not supported, I am the only
> person actually trying this and mostly to make sure that we get all these
> guest/host PTEs right. What we could consider supporting is DPDK in the
> pHyps guest but it does not care about this INTx business.
> 
> 
> > 
> > Alex
> >   
> >>          }
> >>      }
> >>  
> >> @@ -2986,6 +2991,7 @@ static Property vfio_pci_dev_properties[] = {
> >>      DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
> >>                      VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
> >>      DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
> >> +    DEFINE_PROP_BOOL("x-no-intx", VFIOPCIDevice, no_intx, false),
> >>      DEFINE_PROP_BOOL("x-no-kvm-intx", VFIOPCIDevice, no_kvm_intx, false),
> >>      DEFINE_PROP_BOOL("x-no-kvm-msi", VFIOPCIDevice, no_kvm_msi, false),
> >>      DEFINE_PROP_BOOL("x-no-kvm-msix", VFIOPCIDevice, no_kvm_msix, false), 
> >>  
> >   
> 
>