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

[Josef Johansson <josef@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> Fixes: fcacdfbef5a1 ("PCI/MSI: Provide a new set of mask and unmask 
> functions")
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Jason Andryuk <jandryuk@xxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> ---
>  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.