On Tue, 13 Mar 2018 13:38:46 +1100
Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> On 13/3/18 9:44 am, Alex Williamson wrote:
> > On Fri, 9 Mar 2018 12:17:36 +1100
> > Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> >
> >> Commit 2170dd04316e ("vfio-pci: Mask INTx if a device is not capabable
> >> of enabling it") is causing 'Masking broken INTx support' messages
> >> every time when a PCI without INTx support is enabled. This message is
> >> intended to appear when a device known for broken INTx is being enabled.
> >> However the message also appears on IOV virtual functions where INTx
> >> cannot possibly be enabled so saying that the "broken" support is masked
> >> is not correct.
> >>
> >> This changes the message to only appear when the device advertises INTx
> >> (via PCI_INTERRUPT_PIN != 0) but does not implement PCI 2.3 INTx masking.
> >>
> >> Fixes: 2170dd04316e ("vfio-pci: Mask INTx if a device is not capabable of enabling it")
> >> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
> >> ---
> >> drivers/vfio/pci/vfio_pci.c | 8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index b0f7594..7f2ec47 100644
> >> --- a/drivers/vfio/pci/vfio_pci.c
> >> +++ b/drivers/vfio/pci/vfio_pci.c
> >> @@ -245,7 +245,13 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> >>
> >> if (likely(!nointxmask)) {
> >> if (vfio_pci_nointx(pdev)) {
> >> - dev_info(&pdev->dev, "Masking broken INTx support\n");
> >> + u8 pin = 0;
> >> +
> >> + pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN,
> >> + &pin);
> >> + if (pin)
> >> + dev_info(&pdev->dev,
> >> + "Masking broken INTx support\n");
> >> vdev->nointx = true;
> >> pci_intx(pdev, 0);
> >> } else
> >
> >
> > Why don't we fix it at the location of the bug rather than after the
> > fact? I don't see any reason to invoke the nointx handling for devices
> > that don't actually support INTx. Thanks,
>
>
> We can do that too, this just means that we will keep doing v2.3 masking
> tests and possibly other things for SRIOV VFs when we do not need to as
> dev-irq==0 anyway. Not a big deal though (was not a problem before), are
> you going to post this as a patch or you want me to do this? Thanks,
I tend to prefer keeping the nointx as a special case, where I don't
consider that a device not supporting intx, such as a vf, a special
case. The user of the device can detect that the device doesn't
support intx and we don't need to worry about the device triggering
it. The nointx case is trying to emulate that in software. Despite
the commit subject of 2170dd04316e, I think the intent of that commit
is to invoke the nointx behavior if the device supports intx, but
something in the chipset/platform/whatever has made it unconfigurable.
Now that you mention v2.3 stuff, I further question the validity of the
original patch. The nointx handling depends on DisINTx support being
only half broken, that we can mask it, but not detect a pending
interrupt. OTOH, pdev->irq being NULL precludes our ability to
configure an irq handler, but what does it imply about the device's
ability to cause chaos by triggering the interrupt line regardless?
The nointx case does quite a bit of pci_intx(pdev, 0) calls to stop the
device from pulling intx, which is why I don't want to use it for
devices no supporting intx. Should we only be invoking the nointx path
if a) the device supports intx (pin reg), b) pdev->irq is null, and c)
the device passes the v2.3 masking test (which is cached, not retested
now, btw)? Thanks,
Alex
