From: Rajat Jain <rajatja@google.com>
To: Evan Green <evgreen@chromium.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
linux-pci <linux-pci@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] PCI/MSI: Avoid torn updates to MSI pairs
Date: Wed, 22 Jan 2020 03:25:52 -0800 [thread overview]
Message-ID: <CACK8Z6Ft95qj4e_fsA32r_bcz2SsHOW1xxqZJt3_DBAJw=NMGA@mail.gmail.com> (raw)
In-Reply-To: <20200117162444.v2.1.I9c7e72144ef639cc135ea33ef332852a6b33730f@changeid>
On Fri, Jan 17, 2020 at 4:26 PM Evan Green <evgreen@chromium.org> wrote:
>
> __pci_write_msi_msg() updates three registers in the device: address
> high, address low, and data. On x86 systems, address low contains
> CPU targeting info, and data contains the vector. The order of writes
> is address, then data.
>
> This is problematic if an interrupt comes in after address has
> been written, but before data is updated, and both the SMP affinity
> and target vector are being changed. In this case, the interrupt targets
> the wrong vector on the new CPU.
>
> This case is pretty easy to stumble into using xhci and CPU hotplugging.
> Create a script that repeatedly targets interrupts at a set of cores and
> then offlines those cores. Put some stress on USB, and then watch xhci
> lose an interrupt and die.
Do I understand it right, that even with this patch, the driver might
still miss the same interrupt (because we are disabling the interrupt
for that time) - the improvement this patch brings is that it will at
least not be delivered to the wrong CPU or via a wrong vector?
Thanks,
Rajat
>
> Avoid this by disabling MSIs during the update.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
> - Also mask msi-x interrupts during the update
> - Restore the enable/mask bit to its previous value, rather than
> unconditionally enabling interrupts
>
>
> Bjorn,
> I was unsure whether disabling MSIs temporarily is actually an okay
> thing to do. I considered using the mask bit, but got the impression
> that not all devices support the mask bit. Let me know if this going to
> cause problems or there's a better way. I can include the repro
> script I used to cause mayhem if needed.
>
> ---
> drivers/pci/msi.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6b43a5455c7af..bb21a7739fa2c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -311,6 +311,7 @@ void __pci_read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> {
> struct pci_dev *dev = msi_desc_to_pci_dev(entry);
> + u16 msgctl;
>
> if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
> /* Don't touch the hardware now */
> @@ -320,15 +321,25 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> if (!base)
> goto skip;
>
> + pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> + &msgctl);
> +
> + pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> + msgctl | PCI_MSIX_FLAGS_MASKALL);
> +
> 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);
> + pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> + msgctl);
> +
> } else {
> int pos = dev->msi_cap;
> - u16 msgctl;
> + u16 enabled;
>
> pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
> - msgctl &= ~PCI_MSI_FLAGS_QSIZE;
> + enabled = msgctl & PCI_MSI_FLAGS_ENABLE;
> + msgctl &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
> msgctl |= entry->msi_attrib.multiple << 4;
> pci_write_config_word(dev, pos + PCI_MSI_FLAGS, msgctl);
>
> @@ -343,6 +354,9 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> pci_write_config_word(dev, pos + PCI_MSI_DATA_32,
> msg->data);
> }
> +
> + msgctl |= enabled;
> + pci_write_config_word(dev, pos + PCI_MSI_FLAGS, msgctl);
> }
>
> skip:
> --
> 2.24.1
>
next prev parent reply other threads:[~2020-01-22 11:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-18 0:25 [PATCH v2] PCI/MSI: Avoid torn updates to MSI pairs Evan Green
2020-01-22 11:25 ` Rajat Jain [this message]
2020-01-22 18:00 ` Evan Green
2020-01-23 8:49 ` Thomas Gleixner
2020-01-23 18:16 ` Thomas Gleixner
[not found] ` <CAE=gft6YiM5S1A7iJYJTd5zmaAa8=nhLE3B94JtWa+XW-qVSqQ@mail.gmail.com>
2020-01-23 22:59 ` Evan Green
2020-01-24 0:29 ` Evan Green
2020-01-24 14:34 ` Thomas Gleixner
2020-01-24 21:53 ` Evan Green
2020-01-24 22:50 ` Thomas Gleixner
2020-01-28 14:38 ` Thomas Gleixner
2020-01-28 22:22 ` Evan Green
2020-01-28 22:48 ` Thomas Gleixner
2020-01-29 18:00 ` Evan Green
2020-01-29 21:00 ` Thomas Gleixner
2020-01-29 22:53 ` Evan Green
2020-01-29 23:16 ` Thomas Gleixner
2020-01-29 23:48 ` Evan Green
2020-01-31 11:27 ` [PATCH] x86/apic/msi: Plug non-maskable MSI affinity race Thomas Gleixner
2020-01-31 14:26 ` [PATCH V2] " Thomas Gleixner
2020-01-31 20:32 ` Evan Green
2020-01-31 21:45 ` Thomas Gleixner
[not found] ` <20200205144509.7004C21D7D@mail.kernel.org>
2020-02-05 14:58 ` [PATCH] " Thomas Gleixner
2020-02-05 20:18 ` Sasha Levin
2020-01-24 0:50 ` [PATCH v2] PCI/MSI: Avoid torn updates to MSI pairs Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACK8Z6Ft95qj4e_fsA32r_bcz2SsHOW1xxqZJt3_DBAJw=NMGA@mail.gmail.com' \
--to=rajatja@google.com \
--cc=bhelgaas@google.com \
--cc=evgreen@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).