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

[Raj, Ashok]

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@xxxxxxxxx>
> Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> ---
>  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;
>