diff mbox series

[3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update

Message ID 20210721192650.408910288@linutronix.de (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI/MSI, x86: Cure a couple of inconsistencies | expand

Commit Message

Thomas Gleixner July 21, 2021, 7:11 p.m. UTC
The specification states:

    For MSI-X, a function is permitted to cache Address and Data values
    from unmasked MSI-X Table entries. However, anytime software unmasks a
    currently masked MSI-X Table entry either by clearing its Mask bit or
    by clearing the Function Mask bit, the function must update any Address
    or Data values that it cached from that entry. If software changes the
    Address or Data value of an entry while the entry is unmasked, the
    result is undefined.

The Linux kernel's MSI-X support never enforced that the entry is masked
before the entry is modified hence the Fixes tag refers to a commit in:
      git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Enforce the entry to be masked across the update.

There is no point in enforcing this to be handled at all possible call
sites as this is just pointless code duplication and the common update
function is the obvious place to enforce this.

Reported-by: Kevin Tian <kevin.tian@intel.com>
Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Raj, Ashok July 21, 2021, 10:32 p.m. UTC | #1
On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
> The specification states:
> 
>     For MSI-X, a function is permitted to cache Address and Data values
>     from unmasked MSI-X Table entries. However, anytime software unmasks a
>     currently masked MSI-X Table entry either by clearing its Mask bit or
>     by clearing the Function Mask bit, the function must update any Address
>     or Data values that it cached from that entry. If software changes the
>     Address or Data value of an entry while the entry is unmasked, the
>     result is undefined.
> 
> The Linux kernel's MSI-X support never enforced that the entry is masked
> before the entry is modified hence the Fixes tag refers to a commit in:
>       git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> Enforce the entry to be masked across the update.
> 
> There is no point in enforcing this to be handled at all possible call
> sites as this is just pointless code duplication and the common update
> function is the obvious place to enforce this.
> 
> Reported-by: Kevin Tian <kevin.tian@intel.com>
> Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/msi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -289,13 +289,28 @@ void __pci_write_msi_msg(struct msi_desc
>  		/* Don't touch the hardware now */
>  	} else if (entry->msi_attrib.is_msix) {
>  		void __iomem *base = pci_msix_desc_addr(entry);
> +		bool unmasked = !(entry->masked & PCI_MSIX_ENTRY_CTRL_MASKBIT);
>  
>  		if (!base)
>  			goto skip;
>  
> +		/*
> +		 * The specification mandates that the entry is masked
> +		 * when the message is modified:
> +		 *
> +		 * "If software changes the Address or Data value of an
> +		 * entry while the entry is unmasked, the result is
> +		 * undefined."
> +		 */
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
> +

Is there any locking needs here? say during cpu hotplug and some user-space
setting affinity?

>  		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
>  		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
>  		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
> +
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, 0);
>  	} else {
>  		int pos = dev->msi_cap;
>  		u16 msgctl;
>
Thomas Gleixner July 21, 2021, 10:59 p.m. UTC | #2
Ashok,

On Wed, Jul 21 2021 at 15:32, Ashok Raj wrote:
> On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
>> +		/*
>> +		 * The specification mandates that the entry is masked
>> +		 * when the message is modified:
>> +		 *
>> +		 * "If software changes the Address or Data value of an
>> +		 * entry while the entry is unmasked, the result is
>> +		 * undefined."
>> +		 */
>> +		if (unmasked)
>> +			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
>> +
>
> Is there any locking needs here? say during cpu hotplug and some user-space
> setting affinity?

No. Except for suspend/resume this is always serialized by irq_desc::lock().

Thanks,

        tglx
Bjorn Helgaas July 22, 2021, 9:46 p.m. UTC | #3
On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
> The specification states:

Maybe add "(PCIe r5.0, sec 6.1.4.5)"

>     For MSI-X, a function is permitted to cache Address and Data values
>     from unmasked MSI-X Table entries. However, anytime software unmasks a
>     currently masked MSI-X Table entry either by clearing its Mask bit or
>     by clearing the Function Mask bit, the function must update any Address
>     or Data values that it cached from that entry. If software changes the
>     Address or Data value of an entry while the entry is unmasked, the
>     result is undefined.
diff mbox series

Patch

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -289,13 +289,28 @@  void __pci_write_msi_msg(struct msi_desc
 		/* Don't touch the hardware now */
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
+		bool unmasked = !(entry->masked & PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
 		if (!base)
 			goto skip;
 
+		/*
+		 * The specification mandates that the entry is masked
+		 * when the message is modified:
+		 *
+		 * "If software changes the Address or Data value of an
+		 * entry while the entry is unmasked, the result is
+		 * undefined."
+		 */
+		if (unmasked)
+			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
+
 		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
 		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
 		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
+
+		if (unmasked)
+			__pci_msix_desc_mask_irq(entry, 0);
 	} else {
 		int pos = dev->msi_cap;
 		u16 msgctl;