diff mbox series

[2/8] PCI/MSI: Mask all unused MSI-X entries

Message ID 20210721192650.268814107@linutronix.de (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI/MSI, x86: Cure a couple of inconsistencies | expand

Commit Message

Thomas Gleixner July 21, 2021, 7:11 p.m. UTC 
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(-)

Comments

Ashok Raj July 21, 2021, 10:23 p.m. UTC | #1 
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)
>
Thomas Gleixner July 21, 2021, 10:57 p.m. UTC | #2 
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
Marc Zyngier July 22, 2021, 1:46 p.m. UTC | #3 
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.
Bjorn Helgaas July 22, 2021, 9:45 p.m. UTC | #4 
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.
Thomas Gleixner July 28, 2021, 10:04 a.m. UTC | #5 
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
diff mbox series

Patch

--- 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)