diff mbox series

[v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled

Message ID 20221118023535.1903459-1-marmarek@invisiblethingslab.com
State New
Headers
Series [v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled |

Commit Message

Marek Marczykowski-Górecki Nov. 18, 2022, 2:35 a.m. UTC 
  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.
According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
enabled. Change the logic to consider INTx disabled if MSI/MSI-X is
enabled. This applies to two places: checking currently enabled
interrupts type, and transition to MSI/MSI-X - where INTx would be
implicitly disabled.

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          | 19 +++++++++++++------
 .../xen/xen-pciback/conf_space_capability.c   |  3 ++-
 2 files changed, 15 insertions(+), 7 deletions(-)

Comments

Jan Beulich Nov. 18, 2022, 7:36 a.m. UTC | #1 
On 18.11.2022 03:35, 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.
> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> enabled.

Similarly the spec doesn't allow using MSI and MSI-X at the same time.
Before your change xen_pcibk_get_interrupt_type() is consistent for all
three forms of interrupt delivery; imo it also wants to be consistent
after your change. This effectively would mean setting only one bit at
a time (or using an enum right away), but then the question is what
order you do the checks in. IOW I think the change to the function is
wrong.

Furthermore it looks to me as if you're making msi_msix_flags_write()
inconsistent with command_write() - you'd now want to also permit
clearing "INTx disable" when MSI or MSI-X are enabled. Which, I think,
would simply mean allowing the domain unconditional control of the bit
(as long as allow_interrupt_control is set of course).

Especially with these further changes I'm afraid at least for now I
view this as moving in the wrong direction. My view might change in
particular if the description made more clear what was wrong with the
original change (476878e4b2be ["xen-pciback: optionally allow interrupt
enable flag writes"]), or perhaps the discussion having led to the form
which was committed in the end.

Jan
Marek Marczykowski-Górecki Nov. 18, 2022, 12:06 p.m. UTC | #2 
On Fri, Nov 18, 2022 at 08:36:14AM +0100, Jan Beulich wrote:
> On 18.11.2022 03:35, 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.
> > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> > enabled.
> 
> Similarly the spec doesn't allow using MSI and MSI-X at the same time.
> Before your change xen_pcibk_get_interrupt_type() is consistent for all
> three forms of interrupt delivery; imo it also wants to be consistent
> after your change. This effectively would mean setting only one bit at
> a time (or using an enum right away), but then the question is what
> order you do the checks in. IOW I think the change to the function is
> wrong.

IIUC the difference is that enabling MSI or MSI-X implicitly disables
INTx, while enabling both MSI and MSI-X is UB. This means that MSI
active and PCI_COMMAND_INTX_DISABLE bit not set means "only MSI is
active" - which the function now properly reports.
Both MSI and MSI-X active at the same time means a bug somewhere else
and the current code allows only to disable one of them in such case. I
could replace this with BUG_ON, or simply assume such bug doesn't exist
and ignore this case, if you prefer.

> Furthermore it looks to me as if you're making msi_msix_flags_write()
> inconsistent with command_write() - you'd now want to also permit
> clearing "INTx disable" when MSI or MSI-X are enabled. Which, I think,
> would simply mean allowing the domain unconditional control of the bit
> (as long as allow_interrupt_control is set of course).

I think your are correct.

> Especially with these further changes I'm afraid at least for now I
> view this as moving in the wrong direction. My view might change in
> particular if the description made more clear what was wrong with the
> original change (476878e4b2be ["xen-pciback: optionally allow interrupt
> enable flag writes"]), or perhaps the discussion having led to the form
> which was committed in the end.

I'm afraid I don't understand why you think it's the wrong direction.
Can you clarify?
Jan Beulich Nov. 18, 2022, 12:27 p.m. UTC | #3 
On 18.11.2022 13:06, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 18, 2022 at 08:36:14AM +0100, Jan Beulich wrote:
>> On 18.11.2022 03:35, 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.
>>> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
>>> enabled.
>>
>> Similarly the spec doesn't allow using MSI and MSI-X at the same time.
>> Before your change xen_pcibk_get_interrupt_type() is consistent for all
>> three forms of interrupt delivery; imo it also wants to be consistent
>> after your change. This effectively would mean setting only one bit at
>> a time (or using an enum right away), but then the question is what
>> order you do the checks in. IOW I think the change to the function is
>> wrong.
> 
> IIUC the difference is that enabling MSI or MSI-X implicitly disables
> INTx, while enabling both MSI and MSI-X is UB. This means that MSI
> active and PCI_COMMAND_INTX_DISABLE bit not set means "only MSI is
> active" - which the function now properly reports.

Hmm, yes, this is perhaps a good way to look at it.

> Both MSI and MSI-X active at the same time means a bug somewhere else
> and the current code allows only to disable one of them in such case. I
> could replace this with BUG_ON, or simply assume such bug doesn't exist
> and ignore this case, if you prefer.

BUG_ON() would imply this state cannot be reached no matter what is
requested from outside of the kernel. I'm not sure that's true here,
so keeping that aspect unchanged is probably fine (and with the above
I then take back my "wrong" from the earlier reply.

Jan
diff mbox series

Patch

diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c
index 059de92aea7d..d47eee6c5143 100644
--- a/drivers/xen/xen-pciback/conf_space.c
+++ b/drivers/xen/xen-pciback/conf_space.c
@@ -288,12 +288,6 @@  int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
 	u16 val;
 	int ret = 0;
 
-	err = pci_read_config_word(dev, PCI_COMMAND, &val);
-	if (err)
-		return err;
-	if (!(val & PCI_COMMAND_INTX_DISABLE))
-		ret |= INTERRUPT_TYPE_INTX;
-
 	/*
 	 * Do not trust dev->msi(x)_enabled here, as enabling could be done
 	 * bypassing the pci_*msi* functions, by the qemu.
@@ -316,6 +310,19 @@  int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
 		if (val & PCI_MSIX_FLAGS_ENABLE)
 			ret |= INTERRUPT_TYPE_MSIX;
 	}
+
+	/*
+	 * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled,
+	 * so check for INTx only when both are disabled.
+	 */
+	if (!ret) {
+		err = pci_read_config_word(dev, PCI_COMMAND, &val);
+		if (err)
+			return err;
+		if (!(val & PCI_COMMAND_INTX_DISABLE))
+			ret |= INTERRUPT_TYPE_INTX;
+	}
+
 	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..eb4c1af44f5c 100644
--- a/drivers/xen/xen-pciback/conf_space_capability.c
+++ b/drivers/xen/xen-pciback/conf_space_capability.c
@@ -236,10 +236,11 @@  static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value,
 		return PCIBIOS_SET_FAILED;
 
 	if (new_value & field_config->enable_bit) {
-		/* don't allow enabling together with other interrupt types */
+		/* don't allow enabling together with other interrupt type */
 		int int_type = xen_pcibk_get_interrupt_type(dev);
 
 		if (int_type == INTERRUPT_TYPE_NONE ||
+		    int_type == INTERRUPT_TYPE_INTX ||
 		    int_type == field_config->int_type)
 			goto write;
 		return PCIBIOS_SET_FAILED;