All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
@ 2021-07-21 19:11 Thomas Gleixner
  2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
                   ` (10 more replies)
  0 siblings, 11 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, David S. Miller, Bjorn Helgaas,
	linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

A recent discussion about the PCI/MSI management for virtio unearthed a
violation of the MSI-X specification vs. writing the MSI-X message: under
certain circumstances the entry is written without being masked.

While looking at that and the related violation of the x86 non-remapped
interrupt affinity mechanism a few other issues were discovered by
inspection.

The following series addresses these.

Note this does not fix the virtio issue, but while staring at the above
problems I came up with a plan to address this. I'm still trying to
convince myself that I can get away without sprinkling locking all over the
place, so don't hold your breath that this will materialize tomorrow.

The series is also available from git:

    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi

Thanks,

	tglx
---
 arch/x86/kernel/apic/io_apic.c |    6 +-
 arch/x86/kernel/apic/msi.c     |   11 +++-
 arch/x86/kernel/hpet.c         |    2 
 drivers/pci/msi.c              |   98 +++++++++++++++++++++++++++--------------
 include/linux/irq.h            |    2 
 kernel/irq/chip.c              |    5 +-
 6 files changed, 85 insertions(+), 39 deletions(-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 1/8] PCI/MSI: Enable and mask MSIX early
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 21:38   ` Raj, Ashok
  2021-07-22 21:43   ` Bjorn Helgaas
  2021-07-21 19:11 ` [patch 2/8] PCI/MSI: Mask all unused MSI-X entries Thomas Gleixner
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, David S. Miller, Bjorn Helgaas,
	linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

The ordering of MSI-X enable in hardware is disfunctional:

 1) MSI-X is disabled in the control register
 2) Various setup functions
 3) pci_msi_setup_msi_irqs() is invoked which ends up accessing
    the MSI-X table entries
 4) MSI-X is enabled and masked in the control register with the
    comment that enabling is required for some hardware to access
    the MSI-X table

#4 obviously contradicts #3. The history of this is an issue with the NIU
hardware. When #4 was introduced the table access actually happened in
msix_program_entries() which was invoked after enabling and masking MSI-X.

This was changed in commit d71d6432e105 ("PCI/MSI: Kill redundant call of
irq_set_msi_desc() for MSI-X interrupts") which removed the table write
from msix_program_entries().

Interestingly enough nobody noticed and either NIU still works or it did
not get any testing with a kernel 3.19 or later.

Nevertheless this is inconsistent and there is no reason why MSI-X can't be
enabled and masked in the control register early on, i.e. move #4 above to
#1. This preserves the NIU workaround and has no side effects on other
hardware.

Fixes: d71d6432e105 ("PCI/MSI: Kill redundant call of irq_set_msi_desc() for MSI-X interrupts")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |   28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -772,18 +772,25 @@ static int msix_capability_init(struct p
 	u16 control;
 	void __iomem *base;
 
-	/* Ensure MSI-X is disabled while it is set up */
-	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
+	/*
+	 * Some devices require MSI-X to be enabled before the MSI-X
+	 * registers can be accessed.  Mask all the vectors to prevent
+	 * interrupts coming in before they're fully set up.
+	 */
+	pci_msix_clear_and_set_ctrl(dev, 0, PCI_MSIX_FLAGS_MASKALL |
+				    PCI_MSIX_FLAGS_ENABLE);
 
 	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
 	/* Request & Map MSI-X table region */
 	base = msix_map_region(dev, msix_table_size(control));
-	if (!base)
-		return -ENOMEM;
+	if (!base) {
+		ret = -ENOMEM;
+		goto out_disable;
+	}
 
 	ret = msix_setup_entries(dev, base, entries, nvec, affd);
 	if (ret)
-		return ret;
+		goto out_disable;
 
 	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
@@ -794,14 +801,6 @@ static int msix_capability_init(struct p
 	if (ret)
 		goto out_free;
 
-	/*
-	 * Some devices require MSI-X to be enabled before we can touch the
-	 * MSI-X registers.  We need to mask all the vectors to prevent
-	 * interrupts coming in before they're fully set up.
-	 */
-	pci_msix_clear_and_set_ctrl(dev, 0,
-				PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
-
 	msix_program_entries(dev, entries);
 
 	ret = populate_msi_sysfs(dev);
@@ -836,6 +835,9 @@ static int msix_capability_init(struct p
 out_free:
 	free_msi_irqs(dev);
 
+out_disable:
+	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
+
 	return ret;
 }
 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
  2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 22:23   ` Raj, Ashok
  2021-07-22 21:45   ` Bjorn Helgaas
  2021-07-21 19:11 ` [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update Thomas Gleixner
                   ` (8 subsequent siblings)
  10 siblings, 2 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

When MSI-X is enabled the ordering of calls is:

  msix_map_region();
  msix_setup_entries();
  pci_msi_setup_msi_irqs();
  msix_program_entries();

This has a few interesting issues:

 1) msix_setup_entries() allocates the msi descriptors and initializes them
    except for the msi_desc:masked member which is left zero initialized.

 2) pci_msi_setup_msi_irqs() allocates the interrupt descriptors and sets
    up the MSI interrupts which ends up in pci_write_msi_msg() unless the
    interrupt chip provides it's own irq_write_msi_msg() function.

 3) msix_program_entries() does not do what the name suggests. It solely
    updates the entries array (if not NULL) and initializes the masked
    member for each msi descriptor by reading the hardware state and then
    masks the entry.

Obviously this has some issues:

 1) The uninitialized masked member of msi_desc prevents the enforcement
    of masking the entry in pci_write_msi_msg() depending on the cached
    masked bit. Aside of that half initialized data is a NONO in general

 2) msix_program_entries() only ensures that the actually allocated entries
    are masked. This is wrong as experimentation with crash testing and
    crash kernel kexec has shown.

    This limited testing unearthed that when the production kernel had more
    entries in use and unmasked when it crashed and the crash kernel
    allocated a smaller amount of entries, then a full scan of all entries
    found unmasked entries which were in use in the production kernel.

    This is obviously a device or emulation issue as the device reset
    should mask all MSI-X table entries, but obviously that's just part
    of the paper specification.

Cure this by:

 1) Masking all table entries in hardware
 2) Initializing msi_desc::masked in msix_setup_entries()
 3) Removing the mask dance in msix_program_entries()
 4) Renaming msix_program_entries() to msix_update_entries() to
    reflect the purpose of that function.

As the masking of unused entries has never been done the Fixes tag refers
to a commit in:
   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |   45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -691,6 +691,7 @@ static int msix_setup_entries(struct pci
 {
 	struct irq_affinity_desc *curmsk, *masks = NULL;
 	struct msi_desc *entry;
+	void __iomem *addr;
 	int ret, i;
 	int vec_count = pci_msix_vec_count(dev);
 
@@ -711,6 +712,7 @@ static int msix_setup_entries(struct pci
 
 		entry->msi_attrib.is_msix	= 1;
 		entry->msi_attrib.is_64		= 1;
+
 		if (entries)
 			entry->msi_attrib.entry_nr = entries[i].entry;
 		else
@@ -722,6 +724,10 @@ static int msix_setup_entries(struct pci
 		entry->msi_attrib.default_irq	= dev->irq;
 		entry->mask_base		= base;
 
+		addr = pci_msix_desc_addr(entry);
+		if (addr)
+			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
+
 		list_add_tail(&entry->list, dev_to_msi_list(&dev->dev));
 		if (masks)
 			curmsk++;
@@ -732,26 +738,25 @@ static int msix_setup_entries(struct pci
 	return ret;
 }
 
-static void msix_program_entries(struct pci_dev *dev,
-				 struct msix_entry *entries)
+static void msix_update_entries(struct pci_dev *dev, struct msix_entry *entries)
 {
 	struct msi_desc *entry;
-	int i = 0;
-	void __iomem *desc_addr;
 
 	for_each_pci_msi_entry(entry, dev) {
-		if (entries)
-			entries[i++].vector = entry->irq;
+		if (entries) {
+			entries->vector = entry->irq;
+			entries++;
+		}
+	}
+}
 
-		desc_addr = pci_msix_desc_addr(entry);
-		if (desc_addr)
-			entry->masked = readl(desc_addr +
-					      PCI_MSIX_ENTRY_VECTOR_CTRL);
-		else
-			entry->masked = 0;
+static void msix_mask_all(void __iomem *base, int tsize)
+{
+	u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
+	int i;
 
-		msix_mask_irq(entry, 1);
-	}
+	for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
+		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);
 }
 
 /**
@@ -768,9 +773,9 @@ static void msix_program_entries(struct
 static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
 				int nvec, struct irq_affinity *affd)
 {
-	int ret;
-	u16 control;
 	void __iomem *base;
+	int ret, tsize;
+	u16 control;
 
 	/*
 	 * Some devices require MSI-X to be enabled before the MSI-X
@@ -782,12 +787,16 @@ static int msix_capability_init(struct p
 
 	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
 	/* Request & Map MSI-X table region */
-	base = msix_map_region(dev, msix_table_size(control));
+	tsize = msix_table_size(control);
+	base = msix_map_region(dev, tsize);
 	if (!base) {
 		ret = -ENOMEM;
 		goto out_disable;
 	}
 
+	/* Ensure that all table entries are masked. */
+	msix_mask_all(base, tsize);
+
 	ret = msix_setup_entries(dev, base, entries, nvec, affd);
 	if (ret)
 		goto out_disable;
@@ -801,7 +810,7 @@ static int msix_capability_init(struct p
 	if (ret)
 		goto out_free;
 
-	msix_program_entries(dev, entries);
+	msix_update_entries(dev, entries);
 
 	ret = populate_msi_sysfs(dev);
 	if (ret)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
  2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
  2021-07-21 19:11 ` [patch 2/8] PCI/MSI: Mask all unused MSI-X entries Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 22:32   ` Raj, Ashok
  2021-07-22 21:46   ` Bjorn Helgaas
  2021-07-21 19:11 ` [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible Thomas Gleixner
                   ` (7 subsequent siblings)
  10 siblings, 2 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, Kevin Tian, Bjorn Helgaas,
	linux-pci, David S. Miller, Marc Zyngier, Ingo Molnar, x86

The specification states:

    For MSI-X, a function is permitted to cache Address and Data values
    from unmasked MSI-X Table entries. However, anytime software unmasks a
    currently masked MSI-X Table entry either by clearing its Mask bit or
    by clearing the Function Mask bit, the function must update any Address
    or Data values that it cached from that entry. If software changes the
    Address or Data value of an entry while the entry is unmasked, the
    result is undefined.

The Linux kernel's MSI-X support never enforced that the entry is masked
before the entry is modified hence the Fixes tag refers to a commit in:
      git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Enforce the entry to be masked across the update.

There is no point in enforcing this to be handled at all possible call
sites as this is just pointless code duplication and the common update
function is the obvious place to enforce this.

Reported-by: Kevin Tian <kevin.tian@intel.com>
Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -289,13 +289,28 @@ void __pci_write_msi_msg(struct msi_desc
 		/* Don't touch the hardware now */
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
+		bool unmasked = !(entry->masked & PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
 		if (!base)
 			goto skip;
 
+		/*
+		 * The specification mandates that the entry is masked
+		 * when the message is modified:
+		 *
+		 * "If software changes the Address or Data value of an
+		 * entry while the entry is unmasked, the result is
+		 * undefined."
+		 */
+		if (unmasked)
+			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
+
 		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
 		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
 		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
+
+		if (unmasked)
+			__pci_msix_desc_mask_irq(entry, 0);
 	} else {
 		int pos = dev->msi_cap;
 		u16 msgctl;


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (2 preceding siblings ...)
  2021-07-21 19:11 ` [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-22 21:48   ` Bjorn Helgaas
  2021-07-21 19:11 ` [patch 5/8] PCI/MSI: Simplify msi_verify_entries() Thomas Gleixner
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

Nothing enforces the posted writes to be visible when the function
returns. Flush them even if the flush might be redundant when the entry is
masked already as the unmask will flush as well. This is either setup or a
rare affinity change event so the extra flush is not the end of the world.

While this is more a theoretical issue especially the X86 MSI affinity
stter mechanism relies on the assumption that the update has reached the
hardware when the function returns.

Again, as this never has been enfocred the Fixes tag refers to a commit in:
   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -311,6 +311,9 @@ void __pci_write_msi_msg(struct msi_desc
 
 		if (unmasked)
 			__pci_msix_desc_mask_irq(entry, 0);
+
+		/* Ensure that the writes are visible in the device */
+		readl(base + PCI_MSIX_ENTRY_DATA);
 	} else {
 		int pos = dev->msi_cap;
 		u16 msgctl;
@@ -331,6 +334,8 @@ void __pci_write_msi_msg(struct msi_desc
 			pci_write_config_word(dev, pos + PCI_MSI_DATA_32,
 					      msg->data);
 		}
+		/* Ensure that the writes are visible in the device */
+		pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
 	}
 
 skip:


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 5/8] PCI/MSI: Simplify msi_verify_entries()
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (3 preceding siblings ...)
  2021-07-21 19:11 ` [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 19:11 ` [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP Thomas Gleixner
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

No point in looping over all entries when 64bit addressing mode is enabled
for nothing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/msi.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -612,8 +612,11 @@ static int msi_verify_entries(struct pci
 {
 	struct msi_desc *entry;
 
+	if (!dev->no_64bit_msi)
+		return 0;
+
 	for_each_pci_msi_entry(entry, dev) {
-		if (entry->msg.address_hi && dev->no_64bit_msi) {
+		if (entry->msg.address_hi) {
 			pci_err(dev, "arch assigned 64-bit MSI address %#x%08x but device only supports 32 bits\n",
 				entry->msg.address_hi, entry->msg.address_lo);
 			return -EIO;


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (4 preceding siblings ...)
  2021-07-21 19:11 ` [patch 5/8] PCI/MSI: Simplify msi_verify_entries() Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-22 15:12   ` Marc Zyngier
  2021-07-21 19:11 ` [patch 7/8] x86/ioapic: Force affinity setup before startup Thomas Gleixner
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, Marc Zyngier, Ingo Molnar,
	David S. Miller, Bjorn Helgaas, linux-pci, Kevin Tian, x86

X86 IO/APIC and MSI interrupts (when used without interrupts remapping)
require that the affinity setup on startup is done before the interrupt is
enabled for the first time as the non-remapped operation mode cannot safely
migrate enabled interrupts from arbitrary contexts. Provide a new irq chip
flag which allows affected hardware to request this.

This has to be opt-in because there have been reports in the past that some
interrupt chips cannot handle affinity setting before startup.

Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
---
 include/linux/irq.h |    2 ++
 kernel/irq/chip.c   |    5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -569,6 +569,7 @@ struct irq_chip {
  * IRQCHIP_SUPPORTS_NMI:              Chip can deliver NMIs, only for root irqchips
  * IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND:  Invokes __enable_irq()/__disable_irq() for wake irqs
  *                                    in the suspend path if they are in disabled state
+ * IRQCHIP_AFFINITY_PRE_STARTUP:      Default affinity update before startup
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED			= (1 <<  0),
@@ -581,6 +582,7 @@ enum {
 	IRQCHIP_SUPPORTS_LEVEL_MSI		= (1 <<  7),
 	IRQCHIP_SUPPORTS_NMI			= (1 <<  8),
 	IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND	= (1 <<  9),
+	IRQCHIP_AFFINITY_PRE_STARTUP		= (1 << 10),
 };
 
 #include <linux/irqdesc.h>
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b
 	} else {
 		switch (__irq_startup_managed(desc, aff, force)) {
 		case IRQ_STARTUP_NORMAL:
+			if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)
+				irq_setup_affinity(desc);
 			ret = __irq_startup(desc);
-			irq_setup_affinity(desc);
+			if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP))
+				irq_setup_affinity(desc);
 			break;
 		case IRQ_STARTUP_MANAGED:
 			irq_do_set_affinity(d, aff, false);


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 7/8] x86/ioapic: Force affinity setup before startup
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (5 preceding siblings ...)
  2021-07-21 19:11 ` [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 19:11 ` [patch 8/8] x86/msi: " Thomas Gleixner
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, x86, David S. Miller, Bjorn Helgaas,
	linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar

The IO/APIC cannot handle interrupt affinity changes safely after startup
other than from an interrupt handler. The startup sequence in the generic
interrupt code violates that assumption.

Mark the irq chip with the new IRQCHIP_AFFINITY_PRE_STARTUP flag so that
the default interrupt setting happens before the interrupt is started up
for the first time.

Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
---
 arch/x86/kernel/apic/io_apic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1986,7 +1986,8 @@ static struct irq_chip ioapic_chip __rea
 	.irq_set_affinity	= ioapic_set_affinity,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_get_irqchip_state	= ioapic_irq_get_chip_state,
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
+	.flags			= IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static struct irq_chip ioapic_ir_chip __read_mostly = {
@@ -1999,7 +2000,8 @@ static struct irq_chip ioapic_ir_chip __
 	.irq_set_affinity	= ioapic_set_affinity,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_get_irqchip_state	= ioapic_irq_get_chip_state,
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
+	.flags			= IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static inline void init_IO_APIC_traps(void)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch 8/8] x86/msi: Force affinity setup before startup
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (6 preceding siblings ...)
  2021-07-21 19:11 ` [patch 7/8] x86/ioapic: Force affinity setup before startup Thomas Gleixner
@ 2021-07-21 19:11 ` Thomas Gleixner
  2021-07-21 21:10 ` [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Raj, Ashok
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 19:11 UTC (permalink / raw)
  To: LKML
  Cc: Alex Williamson, Raj, Ashok, David S. Miller, Bjorn Helgaas,
	linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

The X86 MSI mechanism cannot handle interrupt affinity changes safely after
startup other than from an interrupt handler, unless interrupt remapping is
enabled. The startup sequence in the generic interrupt code violates that
assumption.

Mark the irq chips with the new IRQCHIP_AFFINITY_PRE_STARTUP flag so that
the default interrupt setting happens before the interrupt is started up
for the first time.

While the interrupt remapping MSI chip does not require this, there is no
point in treating it differently as this might spare an interrupt to a CPU
which is not in the default affinity mask.

For the non-remapping case go to the direct write path when the interrupt
is not yet started similar to the not yet activated case.

Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apic/msi.c |   11 ++++++++---
 arch/x86/kernel/hpet.c     |    2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -58,11 +58,13 @@ msi_set_affinity(struct irq_data *irqd,
 	 *   The quirk bit is not set in this case.
 	 * - The new vector is the same as the old vector
 	 * - The old vector is MANAGED_IRQ_SHUTDOWN_VECTOR (interrupt starts up)
+	 * - The interrupt is not yet started up
 	 * - The new destination CPU is the same as the old destination CPU
 	 */
 	if (!irqd_msi_nomask_quirk(irqd) ||
 	    cfg->vector == old_cfg.vector ||
 	    old_cfg.vector == MANAGED_IRQ_SHUTDOWN_VECTOR ||
+	    !irqd_is_started(irqd) ||
 	    cfg->dest_apicid == old_cfg.dest_apicid) {
 		irq_msi_update_msg(irqd, cfg);
 		return ret;
@@ -150,7 +152,8 @@ static struct irq_chip pci_msi_controlle
 	.irq_ack		= irq_chip_ack_parent,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_affinity	= msi_set_affinity,
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
+	.flags			= IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec,
@@ -219,7 +222,8 @@ static struct irq_chip pci_msi_ir_contro
 	.irq_mask		= pci_msi_mask_irq,
 	.irq_ack		= irq_chip_ack_parent,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
+	.flags			= IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static struct msi_domain_info pci_msi_ir_domain_info = {
@@ -273,7 +277,8 @@ static struct irq_chip dmar_msi_controll
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_compose_msi_msg	= dmar_msi_compose_msg,
 	.irq_write_msi_msg	= dmar_msi_write_msg,
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
+	.flags			= IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static int dmar_msi_init(struct irq_domain *domain,
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -508,7 +508,7 @@ static struct irq_chip hpet_msi_controll
 	.irq_set_affinity = msi_domain_set_affinity,
 	.irq_retrigger = irq_chip_retrigger_hierarchy,
 	.irq_write_msi_msg = hpet_msi_write_msg,
-	.flags = IRQCHIP_SKIP_SET_WAKE,
+	.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static int hpet_msi_init(struct irq_domain *domain,


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (7 preceding siblings ...)
  2021-07-21 19:11 ` [patch 8/8] x86/msi: " Thomas Gleixner
@ 2021-07-21 21:10 ` Raj, Ashok
  2021-07-21 22:39   ` Thomas Gleixner
  2021-07-22 15:17 ` Marc Zyngier
  2021-07-22 21:43 ` Bjorn Helgaas
  10 siblings, 1 reply; 30+ messages in thread
From: Raj, Ashok @ 2021-07-21 21:10 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, David S. Miller, Bjorn Helgaas, linux-pci,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

On Wed, Jul 21, 2021 at 09:11:26PM +0200, Thomas Gleixner wrote:
> A recent discussion about the PCI/MSI management for virtio unearthed a

nit:
virtio or VFIO? :-)

> violation of the MSI-X specification vs. writing the MSI-X message: under
> certain circumstances the entry is written without being masked.
> 
> While looking at that and the related violation of the x86 non-remapped
> interrupt affinity mechanism a few other issues were discovered by
> inspection.

Cheers,
Ashok

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 1/8] PCI/MSI: Enable and mask MSIX early
  2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
@ 2021-07-21 21:38   ` Raj, Ashok
  2021-07-21 22:51     ` Thomas Gleixner
  2021-07-22 21:43   ` Bjorn Helgaas
  1 sibling, 1 reply; 30+ messages in thread
From: Raj, Ashok @ 2021-07-21 21:38 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, David S. Miller, Bjorn Helgaas, linux-pci,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

On Wed, Jul 21, 2021 at 09:11:27PM +0200, Thomas Gleixner wrote:
> The ordering of MSI-X enable in hardware is disfunctional:
> 
>  1) MSI-X is disabled in the control register
>  2) Various setup functions
>  3) pci_msi_setup_msi_irqs() is invoked which ends up accessing
>     the MSI-X table entries
>  4) MSI-X is enabled and masked in the control register with the
>     comment that enabling is required for some hardware to access
>     the MSI-X table
> 
> #4 obviously contradicts #3. The history of this is an issue with the NIU
> hardware. When #4 was introduced the table access actually happened in
> msix_program_entries() which was invoked after enabling and masking MSI-X.
> 
> This was changed in commit d71d6432e105 ("PCI/MSI: Kill redundant call of
> irq_set_msi_desc() for MSI-X interrupts") which removed the table write
> from msix_program_entries().
> 
> Interestingly enough nobody noticed and either NIU still works or it did
> not get any testing with a kernel 3.19 or later.
> 
> Nevertheless this is inconsistent and there is no reason why MSI-X can't be
> enabled and masked in the control register early on, i.e. move #4 above to

Does the above comment also apply to legacy MSI when it support per-vector
masking capability? Probably not interesting since without IR, we only give
1 vector to MSI. 

Reviewed-by: Ashok Raj <ashok.raj@intel.com>

> #1. This preserves the NIU workaround and has no side effects on other
> hardware.
> 
> Fixes: d71d6432e105 ("PCI/MSI: Kill redundant call of irq_set_msi_desc() for MSI-X interrupts")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/msi.c |   28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -772,18 +772,25 @@ static int msix_capability_init(struct p
>  	u16 control;
>  	void __iomem *base;
>  
> -	/* Ensure MSI-X is disabled while it is set up */
> -	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> +	/*
> +	 * Some devices require MSI-X to be enabled before the MSI-X
> +	 * registers can be accessed.  Mask all the vectors to prevent
> +	 * interrupts coming in before they're fully set up.
> +	 */
> +	pci_msix_clear_and_set_ctrl(dev, 0, PCI_MSIX_FLAGS_MASKALL |
> +				    PCI_MSIX_FLAGS_ENABLE);
>  
>  	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
>  	/* Request & Map MSI-X table region */
>  	base = msix_map_region(dev, msix_table_size(control));
> -	if (!base)
> -		return -ENOMEM;
> +	if (!base) {
> +		ret = -ENOMEM;
> +		goto out_disable;
> +	}
>  
>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);
>  	if (ret)
> -		return ret;
> +		goto out_disable;
>  
>  	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
>  	if (ret)
> @@ -794,14 +801,6 @@ static int msix_capability_init(struct p
>  	if (ret)
>  		goto out_free;
>  
> -	/*
> -	 * Some devices require MSI-X to be enabled before we can touch the
> -	 * MSI-X registers.  We need to mask all the vectors to prevent
> -	 * interrupts coming in before they're fully set up.
> -	 */
> -	pci_msix_clear_and_set_ctrl(dev, 0,
> -				PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
> -
>  	msix_program_entries(dev, entries);
>  
>  	ret = populate_msi_sysfs(dev);
> @@ -836,6 +835,9 @@ static int msix_capability_init(struct p
>  out_free:
>  	free_msi_irqs(dev);
>  
> +out_disable:
> +	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> +
>  	return ret;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-21 19:11 ` [patch 2/8] PCI/MSI: Mask all unused MSI-X entries Thomas Gleixner
@ 2021-07-21 22:23   ` Raj, Ashok
  2021-07-21 22:57     ` Thomas Gleixner
  2021-07-22 21:45   ` Bjorn Helgaas
  1 sibling, 1 reply; 30+ messages in thread
From: Raj, Ashok @ 2021-07-21 22:23 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Bjorn Helgaas, linux-pci, David S. Miller,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

Hi Thomas

On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:

[snip]

> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -691,6 +691,7 @@ static int msix_setup_entries(struct pci
>  {
>  	struct irq_affinity_desc *curmsk, *masks = NULL;
>  	struct msi_desc *entry;
> +	void __iomem *addr;
>  	int ret, i;
>  	int vec_count = pci_msix_vec_count(dev);
>  
> @@ -711,6 +712,7 @@ static int msix_setup_entries(struct pci
>  
>  		entry->msi_attrib.is_msix	= 1;
>  		entry->msi_attrib.is_64		= 1;
> +
>  		if (entries)
>  			entry->msi_attrib.entry_nr = entries[i].entry;
>  		else
> @@ -722,6 +724,10 @@ static int msix_setup_entries(struct pci
>  		entry->msi_attrib.default_irq	= dev->irq;
>  		entry->mask_base		= base;
>  
> +		addr = pci_msix_desc_addr(entry);
> +		if (addr)
> +			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);

Silly question:
Do we have to read what the HW has to set this entry->masked? Shouldn't
this be all masked before we start the setup?

> +
>  		list_add_tail(&entry->list, dev_to_msi_list(&dev->dev));
>  		if (masks)
>  			curmsk++;
> @@ -732,26 +738,25 @@ static int msix_setup_entries(struct pci
>  	return ret;
>  }
>  
> -static void msix_program_entries(struct pci_dev *dev,
> -				 struct msix_entry *entries)
> +static void msix_update_entries(struct pci_dev *dev, struct msix_entry *entries)
>  {
>  	struct msi_desc *entry;
> -	int i = 0;
> -	void __iomem *desc_addr;
>  
>  	for_each_pci_msi_entry(entry, dev) {
> -		if (entries)
> -			entries[i++].vector = entry->irq;
> +		if (entries) {
> +			entries->vector = entry->irq;
> +			entries++;
> +		}
> +	}
> +}
>  
> -		desc_addr = pci_msix_desc_addr(entry);
> -		if (desc_addr)
> -			entry->masked = readl(desc_addr +
> -					      PCI_MSIX_ENTRY_VECTOR_CTRL);
> -		else
> -			entry->masked = 0;
> +static void msix_mask_all(void __iomem *base, int tsize)
> +{
> +	u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +	int i;
>  
> -		msix_mask_irq(entry, 1);
> -	}
> +	for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
> +		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);

shouldn't we initialize entry->masked here?

>  }
>  
>  /**
> @@ -768,9 +773,9 @@ static void msix_program_entries(struct
>  static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
>  				int nvec, struct irq_affinity *affd)
>  {
> -	int ret;
> -	u16 control;
>  	void __iomem *base;
> +	int ret, tsize;
> +	u16 control;
>  
>  	/*
>  	 * Some devices require MSI-X to be enabled before the MSI-X
> @@ -782,12 +787,16 @@ static int msix_capability_init(struct p
>  
>  	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
>  	/* Request & Map MSI-X table region */
> -	base = msix_map_region(dev, msix_table_size(control));
> +	tsize = msix_table_size(control);
> +	base = msix_map_region(dev, tsize);
>  	if (!base) {
>  		ret = -ENOMEM;
>  		goto out_disable;
>  	}
>  
> +	/* Ensure that all table entries are masked. */
> +	msix_mask_all(base, tsize);
> +
>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);
>  	if (ret)
>  		goto out_disable;
> @@ -801,7 +810,7 @@ static int msix_capability_init(struct p
>  	if (ret)
>  		goto out_free;
>  
> -	msix_program_entries(dev, entries);
> +	msix_update_entries(dev, entries);
>  
>  	ret = populate_msi_sysfs(dev);
>  	if (ret)
> 

-- 
Cheers,
Ashok

[Forgiveness is the attribute of the STRONG - Gandhi]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update
  2021-07-21 19:11 ` [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update Thomas Gleixner
@ 2021-07-21 22:32   ` Raj, Ashok
  2021-07-21 22:59     ` Thomas Gleixner
  2021-07-22 21:46   ` Bjorn Helgaas
  1 sibling, 1 reply; 30+ messages in thread
From: Raj, Ashok @ 2021-07-21 22:32 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Kevin Tian, Bjorn Helgaas, linux-pci,
	David S. Miller, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
> The specification states:
> 
>     For MSI-X, a function is permitted to cache Address and Data values
>     from unmasked MSI-X Table entries. However, anytime software unmasks a
>     currently masked MSI-X Table entry either by clearing its Mask bit or
>     by clearing the Function Mask bit, the function must update any Address
>     or Data values that it cached from that entry. If software changes the
>     Address or Data value of an entry while the entry is unmasked, the
>     result is undefined.
> 
> The Linux kernel's MSI-X support never enforced that the entry is masked
> before the entry is modified hence the Fixes tag refers to a commit in:
>       git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> Enforce the entry to be masked across the update.
> 
> There is no point in enforcing this to be handled at all possible call
> sites as this is just pointless code duplication and the common update
> function is the obvious place to enforce this.
> 
> Reported-by: Kevin Tian <kevin.tian@intel.com>
> Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/msi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -289,13 +289,28 @@ void __pci_write_msi_msg(struct msi_desc
>  		/* Don't touch the hardware now */
>  	} else if (entry->msi_attrib.is_msix) {
>  		void __iomem *base = pci_msix_desc_addr(entry);
> +		bool unmasked = !(entry->masked & PCI_MSIX_ENTRY_CTRL_MASKBIT);
>  
>  		if (!base)
>  			goto skip;
>  
> +		/*
> +		 * The specification mandates that the entry is masked
> +		 * when the message is modified:
> +		 *
> +		 * "If software changes the Address or Data value of an
> +		 * entry while the entry is unmasked, the result is
> +		 * undefined."
> +		 */
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
> +

Is there any locking needs here? say during cpu hotplug and some user-space
setting affinity?

>  		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
>  		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
>  		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
> +
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, 0);
>  	} else {
>  		int pos = dev->msi_cap;
>  		u16 msgctl;
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
  2021-07-21 21:10 ` [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Raj, Ashok
@ 2021-07-21 22:39   ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 22:39 UTC (permalink / raw)
  To: Raj, Ashok
  Cc: LKML, Alex Williamson, David S. Miller, Bjorn Helgaas, linux-pci,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

On Wed, Jul 21 2021 at 14:10, Ashok Raj wrote:

> On Wed, Jul 21, 2021 at 09:11:26PM +0200, Thomas Gleixner wrote:
>> A recent discussion about the PCI/MSI management for virtio unearthed a
>
> nit:
> virtio or VFIO? :-)

VFIO of course. 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 1/8] PCI/MSI: Enable and mask MSIX early
  2021-07-21 21:38   ` Raj, Ashok
@ 2021-07-21 22:51     ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 22:51 UTC (permalink / raw)
  To: Raj, Ashok
  Cc: LKML, Alex Williamson, David S. Miller, Bjorn Helgaas, linux-pci,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

On Wed, Jul 21 2021 at 14:38, Ashok Raj wrote:

> On Wed, Jul 21, 2021 at 09:11:27PM +0200, Thomas Gleixner wrote:
>> The ordering of MSI-X enable in hardware is disfunctional:
>> 
>>  1) MSI-X is disabled in the control register
>>  2) Various setup functions
>>  3) pci_msi_setup_msi_irqs() is invoked which ends up accessing
>>     the MSI-X table entries
>>  4) MSI-X is enabled and masked in the control register with the
>>     comment that enabling is required for some hardware to access
>>     the MSI-X table
>> 
>> #4 obviously contradicts #3. The history of this is an issue with the NIU
>> hardware. When #4 was introduced the table access actually happened in
>> msix_program_entries() which was invoked after enabling and masking MSI-X.
>> 
>> This was changed in commit d71d6432e105 ("PCI/MSI: Kill redundant call of
>> irq_set_msi_desc() for MSI-X interrupts") which removed the table write
>> from msix_program_entries().
>> 
>> Interestingly enough nobody noticed and either NIU still works or it did
>> not get any testing with a kernel 3.19 or later.
>> 
>> Nevertheless this is inconsistent and there is no reason why MSI-X can't be
>> enabled and masked in the control register early on, i.e. move #4 above to
>
> Does the above comment also apply to legacy MSI when it support per-vector
> masking capability? Probably not interesting since without IR, we only give
> 1 vector to MSI. 

No MSI is completely different as the MSI configuration is purely in PCI
config space while the MSI-X table is separately mapped.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-21 22:23   ` Raj, Ashok
@ 2021-07-21 22:57     ` Thomas Gleixner
  2021-07-22 13:46       ` Marc Zyngier
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 22:57 UTC (permalink / raw)
  To: Raj, Ashok
  Cc: LKML, Alex Williamson, Bjorn Helgaas, linux-pci, David S. Miller,
	Kevin Tian, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

Ashok,

On Wed, Jul 21 2021 at 15:23, Ashok Raj wrote:
> On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:
>>  
>> +		addr = pci_msix_desc_addr(entry);
>> +		if (addr)
>> +			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>
> Silly question:
> Do we have to read what the HW has to set this entry->masked? Shouldn't
> this be all masked before we start the setup?

msix_mask_all() is invoked before the msi descriptors are
allocated. msi_desc::masked is actually a misnomer because it's not like
the name suggests a boolean representing the masked state. It's caching
the content of the PCI_MSIX_ENTRY_VECTOR_CTRL part of the corresponding
table entry. Right now this is just using bit 0 (the mask bit), but is
that true forever? So we actually should rename that member to
vector_ctrl or such.

>> +static void msix_mask_all(void __iomem *base, int tsize)
>> +{
>> +	u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
>> +	int i;
>>  
>> -		msix_mask_irq(entry, 1);
>> -	}
>> +	for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
>> +		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);
>
> shouldn't we initialize entry->masked here?

How so?

>> +	/* Ensure that all table entries are masked. */
>> +	msix_mask_all(base, tsize);
>> +
>>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);

It's invoked before the msi descriptors are set up. We can of course
setup the descriptors first and then do the masking, but notice that
@nvec is not necessarily the same as the table size.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update
  2021-07-21 22:32   ` Raj, Ashok
@ 2021-07-21 22:59     ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-21 22:59 UTC (permalink / raw)
  To: Raj, Ashok
  Cc: LKML, Alex Williamson, Kevin Tian, Bjorn Helgaas, linux-pci,
	David S. Miller, Marc Zyngier, Ingo Molnar, x86, Ashok Raj

Ashok,

On Wed, Jul 21 2021 at 15:32, Ashok Raj wrote:
> On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
>> +		/*
>> +		 * The specification mandates that the entry is masked
>> +		 * when the message is modified:
>> +		 *
>> +		 * "If software changes the Address or Data value of an
>> +		 * entry while the entry is unmasked, the result is
>> +		 * undefined."
>> +		 */
>> +		if (unmasked)
>> +			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
>> +
>
> Is there any locking needs here? say during cpu hotplug and some user-space
> setting affinity?

No. Except for suspend/resume this is always serialized by irq_desc::lock().

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-21 22:57     ` Thomas Gleixner
@ 2021-07-22 13:46       ` Marc Zyngier
  2021-07-28 10:04         ` Thomas Gleixner
  0 siblings, 1 reply; 30+ messages in thread
From: Marc Zyngier @ 2021-07-22 13:46 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Raj, Ashok, LKML, Alex Williamson, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Ingo Molnar, x86

On Wed, 21 Jul 2021 23:57:55 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> Ashok,
> 
> On Wed, Jul 21 2021 at 15:23, Ashok Raj wrote:
> > On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:
> >>  
> >> +		addr = pci_msix_desc_addr(entry);
> >> +		if (addr)
> >> +			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> >
> > Silly question:
> > Do we have to read what the HW has to set this entry->masked? Shouldn't
> > this be all masked before we start the setup?
> 
> msix_mask_all() is invoked before the msi descriptors are
> allocated. msi_desc::masked is actually a misnomer because it's not like
> the name suggests a boolean representing the masked state. It's caching
> the content of the PCI_MSIX_ENTRY_VECTOR_CTRL part of the corresponding
> table entry. Right now this is just using bit 0 (the mask bit), but is
> that true forever? So we actually should rename that member to
> vector_ctrl or such.

To follow-up with this forward looking statement, should we only keep
bit 0 when reading PCI_MSIX_ENTRY_VECTOR_CTRL? I.e.:

	entry->masked = (readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL) &
			 PCI_MSIX_ENTRY_CTRL_MASKBIT);

Or do we want to cache the whole register? In which case I'm all for
the suggesting renaming (though 'masked' is shared with the old-school
multi-MSI).

Otherwise:

Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP
  2021-07-21 19:11 ` [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP Thomas Gleixner
@ 2021-07-22 15:12   ` Marc Zyngier
  2021-07-28 10:40     ` Thomas Gleixner
  0 siblings, 1 reply; 30+ messages in thread
From: Marc Zyngier @ 2021-07-22 15:12 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, Ingo Molnar, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, x86

On Wed, 21 Jul 2021 20:11:32 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> X86 IO/APIC and MSI interrupts (when used without interrupts remapping)
> require that the affinity setup on startup is done before the interrupt is
> enabled for the first time as the non-remapped operation mode cannot safely
> migrate enabled interrupts from arbitrary contexts. Provide a new irq chip
> flag which allows affected hardware to request this.
> 
> This has to be opt-in because there have been reports in the past that some
> interrupt chips cannot handle affinity setting before startup.
> 
> Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/irq.h |    2 ++
>  kernel/irq/chip.c   |    5 ++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -569,6 +569,7 @@ struct irq_chip {
>   * IRQCHIP_SUPPORTS_NMI:              Chip can deliver NMIs, only for root irqchips
>   * IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND:  Invokes __enable_irq()/__disable_irq() for wake irqs
>   *                                    in the suspend path if they are in disabled state
> + * IRQCHIP_AFFINITY_PRE_STARTUP:      Default affinity update before startup
>   */
>  enum {
>  	IRQCHIP_SET_TYPE_MASKED			= (1 <<  0),
> @@ -581,6 +582,7 @@ enum {
>  	IRQCHIP_SUPPORTS_LEVEL_MSI		= (1 <<  7),
>  	IRQCHIP_SUPPORTS_NMI			= (1 <<  8),
>  	IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND	= (1 <<  9),
> +	IRQCHIP_AFFINITY_PRE_STARTUP		= (1 << 10),
>  };
>  
>  #include <linux/irqdesc.h>
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b
>  	} else {
>  		switch (__irq_startup_managed(desc, aff, force)) {
>  		case IRQ_STARTUP_NORMAL:
> +			if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)
> +				irq_setup_affinity(desc);

How about moving this to activate instead? We already special-case the
activation of MSIs for PCI (MSI_FLAG_ACTIVATE_EARLY), and this
wouldn't look completely out of place. The startup mode could be an
issue though...

>  			ret = __irq_startup(desc);
> -			irq_setup_affinity(desc);
> +			if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP))
> +				irq_setup_affinity(desc);
>  			break;
>  		case IRQ_STARTUP_MANAGED:
>  			irq_do_set_affinity(d, aff, false);

Otherwise, looks good.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (8 preceding siblings ...)
  2021-07-21 21:10 ` [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Raj, Ashok
@ 2021-07-22 15:17 ` Marc Zyngier
  2021-07-22 21:43 ` Bjorn Helgaas
  10 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-07-22 15:17 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, Ingo Molnar, x86

On Wed, 21 Jul 2021 20:11:26 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> A recent discussion about the PCI/MSI management for virtio unearthed a
> violation of the MSI-X specification vs. writing the MSI-X message: under
> certain circumstances the entry is written without being masked.
> 
> While looking at that and the related violation of the x86 non-remapped
> interrupt affinity mechanism a few other issues were discovered by
> inspection.
> 
> The following series addresses these.
> 
> Note this does not fix the virtio issue, but while staring at the above
> problems I came up with a plan to address this. I'm still trying to
> convince myself that I can get away without sprinkling locking all over the
> place, so don't hold your breath that this will materialize tomorrow.
> 
> The series is also available from git:
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi
> 
> Thanks,
> 
> 	tglx
> ---
>  arch/x86/kernel/apic/io_apic.c |    6 +-
>  arch/x86/kernel/apic/msi.c     |   11 +++-
>  arch/x86/kernel/hpet.c         |    2 
>  drivers/pci/msi.c              |   98 +++++++++++++++++++++++++++--------------
>  include/linux/irq.h            |    2 
>  kernel/irq/chip.c              |    5 +-
>  6 files changed, 85 insertions(+), 39 deletions(-)

The couple of nits I mentioned notwithstanding, this looks good to me.
I've taken it for a short ride on an arm64 box (both bare metal and
guests), and nothing exploded. Must be good!

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 1/8] PCI/MSI: Enable and mask MSIX early
  2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
  2021-07-21 21:38   ` Raj, Ashok
@ 2021-07-22 21:43   ` Bjorn Helgaas
  2021-07-27 20:33     ` Thomas Gleixner
  1 sibling, 1 reply; 30+ messages in thread
From: Bjorn Helgaas @ 2021-07-22 21:43 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar,
	x86

s/MSIX/MSI-X/ in subject

On Wed, Jul 21, 2021 at 09:11:27PM +0200, Thomas Gleixner wrote:
> The ordering of MSI-X enable in hardware is disfunctional:

s/disfunctional/dysfunctional/, isn't English wonderful ;)

>  1) MSI-X is disabled in the control register
>  2) Various setup functions
>  3) pci_msi_setup_msi_irqs() is invoked which ends up accessing
>     the MSI-X table entries
>  4) MSI-X is enabled and masked in the control register with the
>     comment that enabling is required for some hardware to access
>     the MSI-X table
> 
> #4 obviously contradicts #3. The history of this is an issue with the NIU

Annoyingly, if you "git rebase" and reword this commit log, it drops
this line and the one a few lines below because they start with "#".
Should be obvious, but took me a few iterations to see what was
happening.

> hardware. When #4 was introduced the table access actually happened in
> msix_program_entries() which was invoked after enabling and masking MSI-X.
> 
> This was changed in commit d71d6432e105 ("PCI/MSI: Kill redundant call of
> irq_set_msi_desc() for MSI-X interrupts") which removed the table write
> from msix_program_entries().
> 
> Interestingly enough nobody noticed and either NIU still works or it did
> not get any testing with a kernel 3.19 or later.
> 
> Nevertheless this is inconsistent and there is no reason why MSI-X can't be
> enabled and masked in the control register early on, i.e. move #4 above to
> #1. This preserves the NIU workaround and has no side effects on other
> hardware.
>
> Fixes: d71d6432e105 ("PCI/MSI: Kill redundant call of irq_set_msi_desc() for MSI-X interrupts")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/msi.c |   28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -772,18 +772,25 @@ static int msix_capability_init(struct p
>  	u16 control;
>  	void __iomem *base;
>  
> -	/* Ensure MSI-X is disabled while it is set up */
> -	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> +	/*
> +	 * Some devices require MSI-X to be enabled before the MSI-X
> +	 * registers can be accessed.  Mask all the vectors to prevent
> +	 * interrupts coming in before they're fully set up.
> +	 */
> +	pci_msix_clear_and_set_ctrl(dev, 0, PCI_MSIX_FLAGS_MASKALL |
> +				    PCI_MSIX_FLAGS_ENABLE);
>  
>  	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
>  	/* Request & Map MSI-X table region */
>  	base = msix_map_region(dev, msix_table_size(control));
> -	if (!base)
> -		return -ENOMEM;
> +	if (!base) {
> +		ret = -ENOMEM;
> +		goto out_disable;
> +	}
>  
>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);
>  	if (ret)
> -		return ret;
> +		goto out_disable;
>  
>  	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
>  	if (ret)
> @@ -794,14 +801,6 @@ static int msix_capability_init(struct p
>  	if (ret)
>  		goto out_free;
>  
> -	/*
> -	 * Some devices require MSI-X to be enabled before we can touch the
> -	 * MSI-X registers.  We need to mask all the vectors to prevent
> -	 * interrupts coming in before they're fully set up.
> -	 */
> -	pci_msix_clear_and_set_ctrl(dev, 0,
> -				PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
> -
>  	msix_program_entries(dev, entries);
>  
>  	ret = populate_msi_sysfs(dev);
> @@ -836,6 +835,9 @@ static int msix_capability_init(struct p
>  out_free:
>  	free_msi_irqs(dev);
>  
> +out_disable:
> +	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> +
>  	return ret;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
  2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
                   ` (9 preceding siblings ...)
  2021-07-22 15:17 ` Marc Zyngier
@ 2021-07-22 21:43 ` Bjorn Helgaas
  2021-07-27 20:38   ` Thomas Gleixner
  10 siblings, 1 reply; 30+ messages in thread
From: Bjorn Helgaas @ 2021-07-22 21:43 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar,
	x86

On Wed, Jul 21, 2021 at 09:11:26PM +0200, Thomas Gleixner wrote:
> A recent discussion about the PCI/MSI management for virtio unearthed a
> violation of the MSI-X specification vs. writing the MSI-X message: under
> certain circumstances the entry is written without being masked.
> 
> While looking at that and the related violation of the x86 non-remapped
> interrupt affinity mechanism a few other issues were discovered by
> inspection.
> 
> The following series addresses these.
> 
> Note this does not fix the virtio issue, but while staring at the above
> problems I came up with a plan to address this. I'm still trying to
> convince myself that I can get away without sprinkling locking all over the
> place, so don't hold your breath that this will materialize tomorrow.
> 
> The series is also available from git:
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi
> 
> Thanks,
> 
> 	tglx
> ---
>  arch/x86/kernel/apic/io_apic.c |    6 +-
>  arch/x86/kernel/apic/msi.c     |   11 +++-
>  arch/x86/kernel/hpet.c         |    2 
>  drivers/pci/msi.c              |   98 +++++++++++++++++++++++++++--------------
>  include/linux/irq.h            |    2 
>  kernel/irq/chip.c              |    5 +-
>  6 files changed, 85 insertions(+), 39 deletions(-)

Acked-by: Bjorn Helgaas <bhelgaas@google.com> for the PCI pieces.

I'm happy to take the whole series via PCI, given an x86 ack.  Or you
can merge with my ack.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-21 19:11 ` [patch 2/8] PCI/MSI: Mask all unused MSI-X entries Thomas Gleixner
  2021-07-21 22:23   ` Raj, Ashok
@ 2021-07-22 21:45   ` Bjorn Helgaas
  1 sibling, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2021-07-22 21:45 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:
> When MSI-X is enabled the ordering of calls is:
> 
>   msix_map_region();
>   msix_setup_entries();
>   pci_msi_setup_msi_irqs();
>   msix_program_entries();
> 
> This has a few interesting issues:
> 
>  1) msix_setup_entries() allocates the msi descriptors and initializes them

s/msi/MSI/ (one or two more below)

>     except for the msi_desc:masked member which is left zero initialized.
> 
>  2) pci_msi_setup_msi_irqs() allocates the interrupt descriptors and sets
>     up the MSI interrupts which ends up in pci_write_msi_msg() unless the
>     interrupt chip provides it's own irq_write_msi_msg() function.

s/it's/its/

>  3) msix_program_entries() does not do what the name suggests. It solely
>     updates the entries array (if not NULL) and initializes the masked
>     member for each msi descriptor by reading the hardware state and then
>     masks the entry.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update
  2021-07-21 19:11 ` [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update Thomas Gleixner
  2021-07-21 22:32   ` Raj, Ashok
@ 2021-07-22 21:46   ` Bjorn Helgaas
  1 sibling, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2021-07-22 21:46 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, Kevin Tian, Bjorn Helgaas,
	linux-pci, David S. Miller, Marc Zyngier, Ingo Molnar, x86

On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
> The specification states:

Maybe add "(PCIe r5.0, sec 6.1.4.5)"

>     For MSI-X, a function is permitted to cache Address and Data values
>     from unmasked MSI-X Table entries. However, anytime software unmasks a
>     currently masked MSI-X Table entry either by clearing its Mask bit or
>     by clearing the Function Mask bit, the function must update any Address
>     or Data values that it cached from that entry. If software changes the
>     Address or Data value of an entry while the entry is unmasked, the
>     result is undefined.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible
  2021-07-21 19:11 ` [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible Thomas Gleixner
@ 2021-07-22 21:48   ` Bjorn Helgaas
       [not found]     ` <CAHp75VdNi4rMuRz8UrW9Haf_Ge8KmNJ0w9ykheqkVhmpXHTUyg@mail.gmail.com>
  0 siblings, 1 reply; 30+ messages in thread
From: Bjorn Helgaas @ 2021-07-22 21:48 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Alex Williamson, Raj, Ashok, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Marc Zyngier, Ingo Molnar, x86

On Wed, Jul 21, 2021 at 09:11:30PM +0200, Thomas Gleixner wrote:
> Nothing enforces the posted writes to be visible when the function
> returns. Flush them even if the flush might be redundant when the entry is
> masked already as the unmask will flush as well. This is either setup or a
> rare affinity change event so the extra flush is not the end of the world.
> 
> While this is more a theoretical issue especially the X86 MSI affinity
> stter mechanism relies on the assumption that the update has reached the

stter?

> hardware when the function returns.
> 
> Again, as this never has been enfocred the Fixes tag refers to a commit in:

s/enfocred/enforced/

>    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible
       [not found]     ` <CAHp75VdNi4rMuRz8UrW9Haf_Ge8KmNJ0w9ykheqkVhmpXHTUyg@mail.gmail.com>
@ 2021-07-23  8:14       ` Marc Zyngier
  0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-07-23  8:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bjorn Helgaas, Thomas Gleixner, LKML, Alex Williamson, Raj,
	Ashok, Bjorn Helgaas, linux-pci, David S. Miller, Kevin Tian,
	Ingo Molnar, x86

On Thu, 22 Jul 2021 22:54:48 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> On Friday, July 23, 2021, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Wed, Jul 21, 2021 at 09:11:30PM +0200, Thomas Gleixner wrote:
> > > Nothing enforces the posted writes to be visible when the function
> > > returns. Flush them even if the flush might be redundant when the entry
> > is
> > > masked already as the unmask will flush as well. This is either setup or
> > a
> > > rare affinity change event so the extra flush is not the end of the
> > world.
> > >
> > > While this is more a theoretical issue especially the X86 MSI affinity
> > > stter mechanism relies on the assumption that the update has reached the
> >
> > stter?
> 
> 
> Setter I suppose

My bet is on 'steer', given that this is about affinity management.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 1/8] PCI/MSI: Enable and mask MSIX early
  2021-07-22 21:43   ` Bjorn Helgaas
@ 2021-07-27 20:33     ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-27 20:33 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: LKML, Alex Williamson, Raj, Ashok, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar,
	x86

On Thu, Jul 22 2021 at 16:43, Bjorn Helgaas wrote:
> s/MSIX/MSI-X/ in subject

Sure.

> On Wed, Jul 21, 2021 at 09:11:27PM +0200, Thomas Gleixner wrote:
>> The ordering of MSI-X enable in hardware is disfunctional:
>
> s/disfunctional/dysfunctional/, isn't English wonderful ;)

Yes and I'm never going to master it.

>>  1) MSI-X is disabled in the control register
>>  2) Various setup functions
>>  3) pci_msi_setup_msi_irqs() is invoked which ends up accessing
>>     the MSI-X table entries
>>  4) MSI-X is enabled and masked in the control register with the
>>     comment that enabling is required for some hardware to access
>>     the MSI-X table
>> 
>> #4 obviously contradicts #3. The history of this is an issue with the NIU
>
> Annoyingly, if you "git rebase" and reword this commit log, it drops
> this line and the one a few lines below because they start with "#".
> Should be obvious, but took me a few iterations to see what was
> happening.

Cute.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies
  2021-07-22 21:43 ` Bjorn Helgaas
@ 2021-07-27 20:38   ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-27 20:38 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: LKML, Alex Williamson, Raj, Ashok, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, Marc Zyngier, Ingo Molnar,
	x86

Bjorn,

On Thu, Jul 22 2021 at 16:43, Bjorn Helgaas wrote:
> On Wed, Jul 21, 2021 at 09:11:26PM +0200, Thomas Gleixner wrote:
>> A recent discussion about the PCI/MSI management for virtio unearthed a
>> violation of the MSI-X specification vs. writing the MSI-X message: under
>> certain circumstances the entry is written without being masked.
>> 
>> While looking at that and the related violation of the x86 non-remapped
>> interrupt affinity mechanism a few other issues were discovered by
>> inspection.
>> 
>> The following series addresses these.
>> 
>> Note this does not fix the virtio issue, but while staring at the above
>> problems I came up with a plan to address this. I'm still trying to
>> convince myself that I can get away without sprinkling locking all over the
>> place, so don't hold your breath that this will materialize tomorrow.
>> 
>> The series is also available from git:
>> 
>>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irq/msi
>> 
>> Thanks,
>> 
>> 	tglx
>> ---
>>  arch/x86/kernel/apic/io_apic.c |    6 +-
>>  arch/x86/kernel/apic/msi.c     |   11 +++-
>>  arch/x86/kernel/hpet.c         |    2 
>>  drivers/pci/msi.c              |   98 +++++++++++++++++++++++++++--------------
>>  include/linux/irq.h            |    2 
>>  kernel/irq/chip.c              |    5 +-
>>  6 files changed, 85 insertions(+), 39 deletions(-)
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com> for the PCI pieces.
>
> I'm happy to take the whole series via PCI, given an x86 ack.  Or you
> can merge with my ack.

Let me repost it first with the various review comments fixed. I'm also
having a fix for the VFIO muck in the pipeline which will be based on
this and also requires changes to the irq core. Let me think about the
best way to get this routed.

Thanks,

        tglx



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries
  2021-07-22 13:46       ` Marc Zyngier
@ 2021-07-28 10:04         ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-28 10:04 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Raj, Ashok, LKML, Alex Williamson, Bjorn Helgaas, linux-pci,
	David S. Miller, Kevin Tian, Ingo Molnar, x86

On Thu, Jul 22 2021 at 14:46, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 23:57:55 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> msix_mask_all() is invoked before the msi descriptors are
>> allocated. msi_desc::masked is actually a misnomer because it's not like
>> the name suggests a boolean representing the masked state. It's caching
>> the content of the PCI_MSIX_ENTRY_VECTOR_CTRL part of the corresponding
>> table entry. Right now this is just using bit 0 (the mask bit), but is
>> that true forever? So we actually should rename that member to
>> vector_ctrl or such.
>
> To follow-up with this forward looking statement, should we only keep
> bit 0 when reading PCI_MSIX_ENTRY_VECTOR_CTRL? I.e.:
>
> 	entry->masked = (readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL) &
> 			 PCI_MSIX_ENTRY_CTRL_MASKBIT);
>
> Or do we want to cache the whole register? In which case I'm all for
> the suggesting renaming (though 'masked' is shared with the old-school
> multi-MSI).

We want to cache the whole register because that's what we need to write
when the mask bit is toggled.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP
  2021-07-22 15:12   ` Marc Zyngier
@ 2021-07-28 10:40     ` Thomas Gleixner
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2021-07-28 10:40 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: LKML, Alex Williamson, Raj, Ashok, Ingo Molnar, David S. Miller,
	Bjorn Helgaas, linux-pci, Kevin Tian, x86

On Thu, Jul 22 2021 at 16:12, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 20:11:32 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>>  #include <linux/irqdesc.h>
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b
>>  	} else {
>>  		switch (__irq_startup_managed(desc, aff, force)) {
>>  		case IRQ_STARTUP_NORMAL:
>> +			if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)
>> +				irq_setup_affinity(desc);
>
> How about moving this to activate instead? We already special-case the
> activation of MSIs for PCI (MSI_FLAG_ACTIVATE_EARLY), and this
> wouldn't look completely out of place. The startup mode could be an
> issue though...

Yes, I thought about that, but the ordering here is:

setup()
  early_activate()

early activation just needs to program a valid message. Now later we
have request_irq() invoking:

     activate()
     startup()

So, yes. We could do that in activate, but then we still have the post
startup variant in irq_startup() which makes the code hard to follow.

There is another practical issue. Assume the irq is requested with
IRQF_NOAUTOEN, then irq_startup() will be invoked when the driver calls
enable_irq(), which might be way later and then the affinity setting
might be completely different already. So I rather keep it there.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-07-28 10:40 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 19:11 [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Thomas Gleixner
2021-07-21 19:11 ` [patch 1/8] PCI/MSI: Enable and mask MSIX early Thomas Gleixner
2021-07-21 21:38   ` Raj, Ashok
2021-07-21 22:51     ` Thomas Gleixner
2021-07-22 21:43   ` Bjorn Helgaas
2021-07-27 20:33     ` Thomas Gleixner
2021-07-21 19:11 ` [patch 2/8] PCI/MSI: Mask all unused MSI-X entries Thomas Gleixner
2021-07-21 22:23   ` Raj, Ashok
2021-07-21 22:57     ` Thomas Gleixner
2021-07-22 13:46       ` Marc Zyngier
2021-07-28 10:04         ` Thomas Gleixner
2021-07-22 21:45   ` Bjorn Helgaas
2021-07-21 19:11 ` [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update Thomas Gleixner
2021-07-21 22:32   ` Raj, Ashok
2021-07-21 22:59     ` Thomas Gleixner
2021-07-22 21:46   ` Bjorn Helgaas
2021-07-21 19:11 ` [patch 4/8] PCI/MSI: Enforce MSI[X] entry updates to be visible Thomas Gleixner
2021-07-22 21:48   ` Bjorn Helgaas
     [not found]     ` <CAHp75VdNi4rMuRz8UrW9Haf_Ge8KmNJ0w9ykheqkVhmpXHTUyg@mail.gmail.com>
2021-07-23  8:14       ` Marc Zyngier
2021-07-21 19:11 ` [patch 5/8] PCI/MSI: Simplify msi_verify_entries() Thomas Gleixner
2021-07-21 19:11 ` [patch 6/8] genirq: Provide IRQCHIP_AFFINITY_PRE_STARTUP Thomas Gleixner
2021-07-22 15:12   ` Marc Zyngier
2021-07-28 10:40     ` Thomas Gleixner
2021-07-21 19:11 ` [patch 7/8] x86/ioapic: Force affinity setup before startup Thomas Gleixner
2021-07-21 19:11 ` [patch 8/8] x86/msi: " Thomas Gleixner
2021-07-21 21:10 ` [patch 0/8] PCI/MSI, x86: Cure a couple of inconsistencies Raj, Ashok
2021-07-21 22:39   ` Thomas Gleixner
2021-07-22 15:17 ` Marc Zyngier
2021-07-22 21:43 ` Bjorn Helgaas
2021-07-27 20:38   ` Thomas Gleixner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.