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

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

[Marek Marczykowski-Górecki]

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(-)

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;
 	}
 
-- 
2.37.3

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[David Vrabel]

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

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

[Marek Marczykowski-Górecki]

[-- 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 --]

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

[Jan Beulich]

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

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

[Marek Marczykowski-Górecki]

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

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.

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

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

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

[Jan Beulich]

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;
>  	}
>