Re: [PATCH] xen-pciback: Consider MSI-X enabled only when MASKALL bit is cleared

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]