Message ID | 20210721192650.268814107@linutronix.de |
---|---|
State | New |
Headers | show |
Series | PCI/MSI, x86: Cure a couple of inconsistencies | expand |
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) >
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
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.
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.
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
--- 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)
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(-)