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

[Josef Johansson <josef@xxxxxxxxxxx>]

On 10/27/21 11:54, Josef Johansson wrote:
> 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.
I'm adding

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 6a5ecee6e567..28d509452958 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -529,10 +529,10 @@ static bool msi_check_reservation_mode(struct irq_domain 
*domain,
 
        /*
         * Checking the first MSI descriptor is sufficient. MSIX supports
-        * masking and MSI does so when the maskbit is set.
+        * masking and MSI does so when the can_mask is set.
         */
        desc = first_msi_entry(dev);
-       return desc->msi_attrib.is_msix || desc->msi_attrib.maskbit;
+       return desc->msi_attrib.is_msix || desc->msi_attrib.can_mask;
 }
 
 int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,