Re: [PATCH v3 3/3] PCI/MSI: Convert pci_msi_ignore_mask to per MSI domain flag

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

On Wed, Feb 19, 2025 at 10:20:57AM +0100, Roger Pau Monne wrote:
> Setting pci_msi_ignore_mask inhibits the toggling of the mask bit for both
> MSI and MSI-X entries globally, regardless of the IRQ chip they are using.
> Only Xen sets the pci_msi_ignore_mask when routing physical interrupts over
> event channels, to prevent PCI code from attempting to toggle the maskbit,
> as it's Xen that controls the bit.
> 
> However, the pci_msi_ignore_mask being global will affect devices that use
> MSI interrupts but are not routing those interrupts over event channels
> (not using the Xen pIRQ chip).  One example is devices behind a VMD PCI
> bridge.  In that scenario the VMD bridge configures MSI(-X) using the
> normal IRQ chip (the pIRQ one in the Xen case), and devices behind the
> bridge configure the MSI entries using indexes into the VMD bridge MSI
> table.  The VMD bridge then demultiplexes such interrupts and delivers to
> the destination device(s).  Having pci_msi_ignore_mask set in that scenario
> prevents (un)masking of MSI entries for devices behind the VMD bridge.
> 
> Move the signaling of no entry masking into the MSI domain flags, as that
> allows setting it on a per-domain basis.  Set it for the Xen MSI domain
> that uses the pIRQ chip, while leaving it unset for the rest of the
> cases.
> 
> Remove pci_msi_ignore_mask at once, since it was only used by Xen code, and
> with Xen dropping usage the variable is unneeded.
> 
> This fixes using devices behind a VMD bridge on Xen PV hardware domains.
> 
> Albeit Devices behind a VMD bridge are not known to Xen, that doesn't mean
> Linux cannot use them.  By inhibiting the usage of
> VMD_FEAT_CAN_BYPASS_MSI_REMAP and the removal of the pci_msi_ignore_mask
> bodge devices behind a VMD bridge do work fine when use from a Linux Xen
> hardware domain.  That's the whole point of the series.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Acked-by: Juergen Gross <jgross@xxxxxxxx>

Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

I assume you'll merge this series via the Xen tree.  Let me know if
otherwise.

> ---
> Changes since v2:
>  - Fix subject line.
> 
> Changes since v1:
>  - Fix build.
>  - Expand commit message.
> ---
>  arch/x86/pci/xen.c    |  8 ++------
>  drivers/pci/msi/msi.c | 37 +++++++++++++++++++++----------------
>  include/linux/msi.h   |  3 ++-
>  kernel/irq/msi.c      |  2 +-
>  4 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 0f2fe524f60d..b8755cde2419 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -436,7 +436,8 @@ static struct msi_domain_ops xen_pci_msi_domain_ops = {
>  };
>  
>  static struct msi_domain_info xen_pci_msi_domain_info = {
> -     .flags                  = MSI_FLAG_PCI_MSIX | MSI_FLAG_FREE_MSI_DESCS | 
> MSI_FLAG_DEV_SYSFS,
> +     .flags                  = MSI_FLAG_PCI_MSIX | MSI_FLAG_FREE_MSI_DESCS |
> +                               MSI_FLAG_DEV_SYSFS | MSI_FLAG_NO_MASK,
>       .ops                    = &xen_pci_msi_domain_ops,
>  };
>  
> @@ -484,11 +485,6 @@ static __init void xen_setup_pci_msi(void)
>        * in allocating the native domain and never use it.
>        */
>       x86_init.irqs.create_pci_msi_domain = xen_create_pci_msi_domain;
> -     /*
> -      * With XEN PIRQ/Eventchannels in use PCI/MSI[-X] masking is solely
> -      * controlled by the hypervisor.
> -      */
> -     pci_msi_ignore_mask = 1;
>  }
>  
>  #else /* CONFIG_PCI_MSI */
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 2f647cac4cae..4c8c2b57b5f6 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -10,12 +10,12 @@
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  
>  #include "../pci.h"
>  #include "msi.h"
>  
>  int pci_msi_enable = 1;
> -int pci_msi_ignore_mask;
>  
>  /**
>   * pci_msi_supported - check whether MSI may be enabled on a device
> @@ -285,6 +285,8 @@ static void pci_msi_set_enable(struct pci_dev *dev, int 
> enable)
>  static int msi_setup_msi_desc(struct pci_dev *dev, int nvec,
>                             struct irq_affinity_desc *masks)
>  {
> +     const struct irq_domain *d = dev_get_msi_domain(&dev->dev);
> +     const struct msi_domain_info *info = d->host_data;
>       struct msi_desc desc;
>       u16 control;
>  
> @@ -295,8 +297,7 @@ static int msi_setup_msi_desc(struct pci_dev *dev, int 
> nvec,
>       /* Lies, damned lies, and MSIs */
>       if (dev->dev_flags & PCI_DEV_FLAGS_HAS_MSI_MASKING)
>               control |= PCI_MSI_FLAGS_MASKBIT;
> -     /* Respect XEN's mask disabling */
> -     if (pci_msi_ignore_mask)
> +     if (info->flags & MSI_FLAG_NO_MASK)
>               control &= ~PCI_MSI_FLAGS_MASKBIT;
>  
>       desc.nvec_used                  = nvec;
> @@ -604,12 +605,15 @@ static void __iomem *msix_map_region(struct pci_dev 
> *dev,
>   */
>  void msix_prepare_msi_desc(struct pci_dev *dev, struct msi_desc *desc)
>  {
> +     const struct irq_domain *d = dev_get_msi_domain(&dev->dev);
> +     const struct msi_domain_info *info = d->host_data;
> +
>       desc->nvec_used                         = 1;
>       desc->pci.msi_attrib.is_msix            = 1;
>       desc->pci.msi_attrib.is_64              = 1;
>       desc->pci.msi_attrib.default_irq        = dev->irq;
>       desc->pci.mask_base                     = dev->msix_base;
> -     desc->pci.msi_attrib.can_mask           = !pci_msi_ignore_mask &&
> +     desc->pci.msi_attrib.can_mask           = !(info->flags & 
> MSI_FLAG_NO_MASK) &&
>                                                 
> !desc->pci.msi_attrib.is_virtual;
>  
>       if (desc->pci.msi_attrib.can_mask) {
> @@ -659,9 +663,6 @@ static void msix_mask_all(void __iomem *base, int tsize)
>       u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
>       int i;
>  
> -     if (pci_msi_ignore_mask)
> -             return;
> -
>       for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
>               writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  }
> @@ -714,6 +715,8 @@ static int msix_setup_interrupts(struct pci_dev *dev, 
> struct msix_entry *entries
>  static int msix_capability_init(struct pci_dev *dev, struct msix_entry 
> *entries,
>                               int nvec, struct irq_affinity *affd)
>  {
> +     const struct irq_domain *d = dev_get_msi_domain(&dev->dev);
> +     const struct msi_domain_info *info = d->host_data;
>       int ret, tsize;
>       u16 control;
>  
> @@ -744,15 +747,17 @@ static int msix_capability_init(struct pci_dev *dev, 
> struct msix_entry *entries,
>       /* Disable INTX */
>       pci_intx_for_msi(dev, 0);
>  
> -     /*
> -      * Ensure that all table entries are masked to prevent
> -      * stale entries from firing in a crash kernel.
> -      *
> -      * Done late to deal with a broken Marvell NVME device
> -      * which takes the MSI-X mask bits into account even
> -      * when MSI-X is disabled, which prevents MSI delivery.
> -      */
> -     msix_mask_all(dev->msix_base, tsize);
> +     if (!(info->flags & MSI_FLAG_NO_MASK)) {
> +             /*
> +              * Ensure that all table entries are masked to prevent
> +              * stale entries from firing in a crash kernel.
> +              *
> +              * Done late to deal with a broken Marvell NVME device
> +              * which takes the MSI-X mask bits into account even
> +              * when MSI-X is disabled, which prevents MSI delivery.
> +              */
> +             msix_mask_all(dev->msix_base, tsize);
> +     }
>       pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  
>       pcibios_free_irq(dev);
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index b10093c4d00e..59a421fc42bf 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -73,7 +73,6 @@ struct msi_msg {
>       };
>  };
>  
> -extern int pci_msi_ignore_mask;
>  /* Helper functions */
>  struct msi_desc;
>  struct pci_dev;
> @@ -556,6 +555,8 @@ enum {
>       MSI_FLAG_PCI_MSIX_ALLOC_DYN     = (1 << 20),
>       /* PCI MSIs cannot be steered separately to CPU cores */
>       MSI_FLAG_NO_AFFINITY            = (1 << 21),
> +     /* Inhibit usage of entry masking */
> +     MSI_FLAG_NO_MASK                = (1 << 22),
>  };
>  
>  /**
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 396a067a8a56..7682c36cbccc 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -1143,7 +1143,7 @@ static bool msi_check_reservation_mode(struct 
> irq_domain *domain,
>       if (!(info->flags & MSI_FLAG_MUST_REACTIVATE))
>               return false;
>  
> -     if (IS_ENABLED(CONFIG_PCI_MSI) && pci_msi_ignore_mask)
> +     if (info->flags & MSI_FLAG_NO_MASK)
>               return false;
>  
>       /*
> -- 
> 2.46.0
>