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,
