On Thu, 22 Mar 2018 15:02:09 +1100 Alexey Kardashevskiy <aik@xxxxxxxxx> wrote: > 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: > >>> 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. "... a device may die afterwards"? What does that mean? In normal operation, INTx fires, DisINTx gets set, the interrupt is forwarded to the user, the user services and unmasks the interrupt, rinse and repeat. If the device somehow gets into a locked state where it's pulling INTx and disregarding DisINTx, a) this is broken hardware, b) the spurious interrupt handler will mask the interrupt and switch to polling, penalizing everyone sharing that line. I don't know that there's a model where we can factor in catastrophic hardware failure into the equation. > 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. Too. Many. Negatives. If a device shares an INTx line with other devices then it certainly needs to support DisINTx in order to play nicely with userspace drivers. We require exclusive interrupts for devices that do not support DisINTx. If the hypervisor is choosing to avoid dealing with that problem by not providing routing for INTx (effectively a paravirt please don't use this interface), then it seems like it's not fit to support userspace drivers. It's depending on the cooperation of the user. We can't prevent the user from making the card pull INTx, we can only discourage them from using it, which is not sufficient. Therefore if we really do need to block use of such devices (not INTx routing, no DisINTx support) from using vfio, my patch is insufficient and I think the best approach at this stage is to revert 2170dd04316e and you can rehash a solution for pHyp. Thanks, Alex
