On Mon, 19 Mar 2018 01:05:27 +1100
Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> On 15/3/18 2:02 pm, Alex Williamson wrote:
> > 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
> >>>
> > 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)?
>
> Yes we should, and yes I missed that last bit about caching the result of
> the test which is performed anyway in the common pci code. Thanks,
I spent a while considering whether we should include c) in the patch I
posted, but I couldn't get past the question of what we'd do with the
device if it doesn't pass the INTx masking test. We'd want to emulate
the INTx pin register and report no INTx irq info, as the nointx path
does, then what? If writing DisINTx doesn't do anything, we need to
decide whether lack of INTx routing means the device can harmlessly
pull INTx, or if pulling INTx is not harmless, then we'd need to ban
the device from being assigned to the user. Since we don't know of any
problems and it seems sort of reasonable to assume that if there's no
INTx routing that we can tug it all we want (because if someone is
listening, it's a platform bug that it's not routed), I left it alone
in my follow-up patch. Let me know if you have other thoughts. Thanks,
Alex
