Re: [PATCH] PCI/MSI: Move non-mask check back into low level accessors

[Josef Johansson <josef@oderland.se>]

On 10/27/21 11:50, Thomas Gleixner wrote:
> The recent rework of PCI/MSI[X] masking moved the non-mask checks from the
> low level accessors into the higher level mask/unmask functions.
>
> This missed the fact that these accessors can be invoked from other places
> as well. The missing checks break XEN-PV which sets pci_msi_ignore_mask and
> also violates the virtual MSIX and the msi_attrib.maskbit protections.
>
> Instead of sprinkling checks all over the place, lift them back into the
> low level accessor functions. To avoid checking three different conditions
> combine them into one property of msi_desc::msi_attrib.
>
> Reported-by: Josef Johansson <josef@oderland.se>
> Fixes: fcacdfbef5a1 ("PCI/MSI: Provide a new set of mask and unmask functions")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Andryuk <jandryuk@gmail.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: linux-pci@vger.kernel.org
> Cc: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> ---
>  drivers/pci/msi.c   |   26 ++++++++++++++------------
>  include/linux/msi.h |    2 +-
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -148,6 +148,9 @@ static noinline void pci_msi_update_mask
>  	raw_spinlock_t *lock = &desc->dev->msi_lock;
>  	unsigned long flags;
>  
> +	if (!desc->msi_attrib.can_mask)
> +		return;
> +
>  	raw_spin_lock_irqsave(lock, flags);
>  	desc->msi_mask &= ~clear;
>  	desc->msi_mask |= set;
> @@ -181,7 +184,8 @@ static void pci_msix_write_vector_ctrl(s
>  {
>  	void __iomem *desc_addr = pci_msix_desc_addr(desc);
>  
> -	writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> +	if (desc->msi_attrib.can_mask)
> +		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  }
>  
>  static inline void pci_msix_mask(struct msi_desc *desc)
> @@ -200,23 +204,17 @@ static inline void pci_msix_unmask(struc
>  
>  static void __pci_msi_mask_desc(struct msi_desc *desc, u32 mask)
>  {
> -	if (pci_msi_ignore_mask || desc->msi_attrib.is_virtual)
> -		return;
> -
>  	if (desc->msi_attrib.is_msix)
>  		pci_msix_mask(desc);
> -	else if (desc->msi_attrib.maskbit)
> +	else
>  		pci_msi_mask(desc, mask);
>  }
>  
>  static void __pci_msi_unmask_desc(struct msi_desc *desc, u32 mask)
>  {
> -	if (pci_msi_ignore_mask || desc->msi_attrib.is_virtual)
> -		return;
> -
>  	if (desc->msi_attrib.is_msix)
>  		pci_msix_unmask(desc);
> -	else if (desc->msi_attrib.maskbit)
> +	else
>  		pci_msi_unmask(desc, mask);
>  }
>  
> @@ -484,7 +482,8 @@ msi_setup_entry(struct pci_dev *dev, int
>  	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
>  	entry->msi_attrib.is_virtual    = 0;
>  	entry->msi_attrib.entry_nr	= 0;
> -	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
> +	entry->msi_attrib.can_mask	= !pci_msi_ignore_mask &&
> +					  !!(control & PCI_MSI_FLAGS_MASKBIT);
>  	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
>  	entry->msi_attrib.multi_cap	= (control & PCI_MSI_FLAGS_QMASK) >> 1;
>  	entry->msi_attrib.multiple	= ilog2(__roundup_pow_of_two(nvec));
> @@ -495,7 +494,7 @@ msi_setup_entry(struct pci_dev *dev, int
>  		entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
>  
>  	/* Save the initial mask status */
> -	if (entry->msi_attrib.maskbit)
> +	if (entry->msi_attrib.can_mask)
>  		pci_read_config_dword(dev, entry->mask_pos, &entry->msi_mask);
>  
>  out:
> @@ -638,10 +637,13 @@ static int msix_setup_entries(struct pci
>  		entry->msi_attrib.is_virtual =
>  			entry->msi_attrib.entry_nr >= vec_count;
>  
> +		entry->msi_attrib.can_mask	= !pci_msi_ignore_mask &&
> +						  !entry->msi_attrib.is_virtual;
> +
>  		entry->msi_attrib.default_irq	= dev->irq;
>  		entry->mask_base		= base;
>  
> -		if (!entry->msi_attrib.is_virtual) {
> +		if (!entry->msi_attrib.can_mask) {
>  			addr = pci_msix_desc_addr(entry);
>  			entry->msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  		}
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -148,7 +148,7 @@ struct msi_desc {
>  				u8	is_msix		: 1;
>  				u8	multiple	: 3;
>  				u8	multi_cap	: 3;
> -				u8	maskbit		: 1;
> +				u8	can_mask	: 1;
>  				u8	is_64		: 1;
>  				u8	is_virtual	: 1;
>  				u16	entry_nr;
Thanks,
I'll test this out ASAP.