On 19/3/18 1:15 pm, Alex Williamson wrote:
> 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,
Well, pulling INTx is nasty anyway as a device may die afterwards and the
specific INTx line (or a latch in PCIe bridge) will be blocked for the
devices sharing the same line, and DisINTx does not change that. Our other
hypervisor (pHyp) simply prohibits INTx in order to avoid dealing with this
(this is why 2170dd04316e was made). So not enforcing nointx for
DisINTx-incapable devices does not seem much worse than enabling INTx for
pass through itself.
--
Alexey
