| Message ID | 20221117114122.1588338-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen-pciback: Consider MSI-X enabled only when MASKALL bit is cleared | expand |
On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > the table is filled. Then it disables INTx just before clearing MASKALL > bit. Currently this approach is rejected by xen-pciback. > Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > as PCI_MSIX_FLAGS_MASKALL is set too. > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> The PCI spec states that devices are not permitted to use INTx when MSI or MSI-X is enabled. The mask status has no legitimate bearing on irq type. INTx_DISABLE exists as a bodge to mean "INTx not permitted even when neither MSI nor MSI-X are enabled", and exists because in some case, transiently disabling MSI is the only safe way to update the descriptor. I can believe that this change fixes a an issue, but the logic surely cannot be correct overall. ~Andrew
On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > the table is filled. Then it disables INTx just before clearing MASKALL > bit. Currently this approach is rejected by xen-pciback. > Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > as PCI_MSIX_FLAGS_MASKALL is set too. The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the Command register. PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx interrupts (if implemented)." And there is similar wording for MSI Enable. I think you need to shuffle the checks for MSI/MSI-X in xen_pcibk_get_interrupt_type() so they are _before_ the check of the Interrupt Disable bit in the Command register. David
On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: > On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > > the table is filled. Then it disables INTx just before clearing MASKALL > > bit. Currently this approach is rejected by xen-pciback. > > Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > > as PCI_MSIX_FLAGS_MASKALL is set too. > > The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. > Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the > Command register. That means the second chunk of the patch may even drop the '(new_value & PCI_MSIX_FLAGS_MASKALL)' part, right? > PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx > interrupts (if implemented)." And there is similar wording for MSI Enable. And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' part isn't necessary either. Jan in another thread pointed out that disabling INTx explicitly is still a useful workaround for a flawed hardware. But if that isn't mandated by the spec, maybe it doesn't need to be enforced by pciback either? > I think you need to shuffle the checks for MSI/MSI-X in > xen_pcibk_get_interrupt_type() so they are _before_ the check of the > Interrupt Disable bit in the Command register. Note the xen_pcibk_get_interrupt_type() returns a bitmask of enabled types. It seems it should consider INTx only if both MSI and MSI-X are disabled. I'll make the adjustment. But this alone, won't cover enabling part, as INTx is the only one active at the time.
On 17.11.2022 14:13, Marek Marczykowski-Górecki wrote: > On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: >> On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: >>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until >>> the table is filled. Then it disables INTx just before clearing MASKALL >>> bit. Currently this approach is rejected by xen-pciback. >>> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long >>> as PCI_MSIX_FLAGS_MASKALL is set too. >> >> The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. >> Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the >> Command register. > > That means the second chunk of the patch may even drop the '(new_value & > PCI_MSIX_FLAGS_MASKALL)' part, right? > >> PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx >> interrupts (if implemented)." And there is similar wording for MSI Enable. > > And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' > part isn't necessary either. > > Jan in another thread pointed out that disabling INTx explicitly is > still a useful workaround for a flawed hardware. But if that isn't > mandated by the spec, maybe it doesn't need to be enforced by pciback > either? Well, allowing a device to go into a mode exhibiting undefined behavior is what we ought to prevent when it comes to a DomU doing so vs overall host safety. Jan
On 17.11.2022 13:28, Andrew Cooper wrote: > On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: >> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until >> the table is filled. Then it disables INTx just before clearing MASKALL >> bit. Currently this approach is rejected by xen-pciback. >> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long >> as PCI_MSIX_FLAGS_MASKALL is set too. >> >> Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") >> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > The PCI spec states that devices are not permitted to use INTx when MSI > or MSI-X is enabled. The mask status has no legitimate bearing on irq type. > > INTx_DISABLE exists as a bodge to mean "INTx not permitted even when > neither MSI nor MSI-X are enabled", and exists because in some case, > transiently disabling MSI is the only safe way to update the descriptor. > > > I can believe that this change fixes a an issue, but the logic surely > cannot be correct overall. Question then is - what can we do without altering the sequence of steps Linux (and likely other OSes) take? Imo Marek's proposal is the least bad option, because everything else would be more intrusive or wouldn't take effect for existing released kernel versions running in guests. Jan
On 17.11.2022 12:41, Marek Marczykowski-Górecki wrote: > --- a/drivers/xen/xen-pciback/conf_space.c > +++ b/drivers/xen/xen-pciback/conf_space.c > @@ -313,7 +313,7 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > &val); > if (err) > return err; > - if (val & PCI_MSIX_FLAGS_ENABLE) > + if (val & PCI_MSIX_FLAGS_ENABLE && !(val & PCI_MSIX_FLAGS_MASKALL)) > ret |= INTERRUPT_TYPE_MSIX; > } > return ret ?: INTERRUPT_TYPE_NONE; Considering Andrew's reply, maybe it wasn't a good suggestion to change the code here. If, however, you/we decide to keep the change, then please add another pair of parentheses around the operands of the left hand &. If the change was to be dropped again, I think ... > --- a/drivers/xen/xen-pciback/conf_space_capability.c > +++ b/drivers/xen/xen-pciback/conf_space_capability.c > @@ -242,6 +242,10 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, > if (int_type == INTERRUPT_TYPE_NONE || > int_type == field_config->int_type) > goto write; > + if (int_type == INTERRUPT_TYPE_INTX && ... this would need extending to also cover the INTX|MSIX case (i.e. if a second such write made it here when the enable+maskall bits are already set). Jan > + field_config->int_type == INTERRUPT_TYPE_MSIX && > + (new_value & PCI_MSIX_FLAGS_MASKALL)) > + goto write; > return PCIBIOS_SET_FAILED; > } >
On Thu, Nov 17, 2022 at 02:33:16PM +0100, Jan Beulich wrote: > On 17.11.2022 14:13, Marek Marczykowski-Górecki wrote: > > On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: > >> On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > >>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > >>> the table is filled. Then it disables INTx just before clearing MASKALL > >>> bit. Currently this approach is rejected by xen-pciback. > >>> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > >>> as PCI_MSIX_FLAGS_MASKALL is set too. > >> > >> The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. > >> Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the > >> Command register. > > > > That means the second chunk of the patch may even drop the '(new_value & > > PCI_MSIX_FLAGS_MASKALL)' part, right? > > > >> PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx > >> interrupts (if implemented)." And there is similar wording for MSI Enable. > > > > And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' > > part isn't necessary either. > > > > Jan in another thread pointed out that disabling INTx explicitly is > > still a useful workaround for a flawed hardware. But if that isn't > > mandated by the spec, maybe it doesn't need to be enforced by pciback > > either? > > Well, allowing a device to go into a mode exhibiting undefined behavior > is what we ought to prevent when it comes to a DomU doing so vs overall > host safety. If the spec prohibits using INTx if MSI/MSI-X is enabled (regardless of PCI_COMMAND_INTX_DISABLE bit), then well-behaving device should be fine (we aren't hitting undefined behavior). As for buggy device, it wouldn't be much different from a device ignoring PCI_COMMAND_INTX_DISABLE completely, no (besides the latter being probably much less probable bug)? If the above is assumption is correct, it seems such device may not function correctly without extra workarounds (which are in the driver interest to apply), but should not affect overall host safety (as in: beyond the guest having that device assigned). I think pciback should only enforce what's necessary to prevent one guest hurting others (or the hypervisor), but it doesn't need to prevent guest hurting itself.
diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c index 059de92aea7d..e8923bffc175 100644 --- a/drivers/xen/xen-pciback/conf_space.c +++ b/drivers/xen/xen-pciback/conf_space.c @@ -313,7 +313,7 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) &val); if (err) return err; - if (val & PCI_MSIX_FLAGS_ENABLE) + if (val & PCI_MSIX_FLAGS_ENABLE && !(val & PCI_MSIX_FLAGS_MASKALL)) ret |= INTERRUPT_TYPE_MSIX; } return ret ?: INTERRUPT_TYPE_NONE; diff --git a/drivers/xen/xen-pciback/conf_space_capability.c b/drivers/xen/xen-pciback/conf_space_capability.c index 097316a74126..5c851f916ebc 100644 --- a/drivers/xen/xen-pciback/conf_space_capability.c +++ b/drivers/xen/xen-pciback/conf_space_capability.c @@ -242,6 +242,10 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, if (int_type == INTERRUPT_TYPE_NONE || int_type == field_config->int_type) goto write; + if (int_type == INTERRUPT_TYPE_INTX && + field_config->int_type == INTERRUPT_TYPE_MSIX && + (new_value & PCI_MSIX_FLAGS_MASKALL)) + goto write; return PCIBIOS_SET_FAILED; }
Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until the table is filled. Then it disables INTx just before clearing MASKALL bit. Currently this approach is rejected by xen-pciback. Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long as PCI_MSIX_FLAGS_MASKALL is set too. Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- drivers/xen/xen-pciback/conf_space.c | 2 +- drivers/xen/xen-pciback/conf_space_capability.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)