Re: [PATCH] PCI/MSI: Avoid torn updates to MSI pairs

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Thomas, Marc, Christoph, Rajat]

On Thu, Jan 16, 2020 at 01:31:28PM -0800, Evan Green wrote:
> __pci_write_msi_msg() updates three registers in the device: address
> high, address low, and data. On x86 systems, address low contains
> CPU targeting info, and data contains the vector. The order of writes
> is address, then data.
> 
> This is problematic if an interrupt comes in after address has
> been written, but before data is updated, and the SMP affinity of
> the interrupt is changing. In this case, the interrupt targets the
> wrong vector on the new CPU.
> 
> This case is pretty easy to stumble into using xhci and CPU hotplugging.
> Create a script that targets interrupts at a set of cores and then
> offlines those cores. Put some stress on USB, and then watch xhci lose
> an interrupt and die.
> 
> Avoid this by disabling MSIs during the update.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
> 
> Bjorn,
> I was unsure whether disabling MSIs temporarily is actually an okay
> thing to do. I considered using the mask bit, but got the impression
> that not all devices support the mask bit. Let me know if this going to
> cause problems or there's a better way. I can include the repro
> script I used to cause mayhem if needed.

I suspect this *is* a problem because I think disabling MSI doesn't
disable interrupts; it just means the device will interrupt using INTx
instead of MSI.  And the driver is probably not prepared to handle
INTx.

PCIe r5.0, sec 7.7.1.2, seems relevant: "If MSI and MSI-X are both
disabled, the Function requests servicing using INTx interrupts (if
supported)."

Maybe the IRQ guys have ideas about how to solve this?

> ---
>  drivers/pci/msi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6b43a5455c7af..97856ef862d68 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -328,7 +328,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  		u16 msgctl;
>  
>  		pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
> -		msgctl &= ~PCI_MSI_FLAGS_QSIZE;
> +		msgctl &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
>  		msgctl |= entry->msi_attrib.multiple << 4;
>  		pci_write_config_word(dev, pos + PCI_MSI_FLAGS, msgctl);
>  
> @@ -343,6 +343,9 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  			pci_write_config_word(dev, pos + PCI_MSI_DATA_32,
>  					      msg->data);
>  		}
> +
> +		msgctl |= PCI_MSI_FLAGS_ENABLE;
> +		pci_write_config_word(dev, pos + PCI_MSI_FLAGS, msgctl);
>  	}
>  
>  skip:
> -- 
> 2.24.1
>