Ard and myself have just spent quite some time lately trying to pin
down an issue in the DMA code which was taking the form of a PCIe USB3
controller issuing a DMA access at some bizarre address, and being
caught red-handed by the IOMMU.
After much head scratching and most of a week-end spent on tracing the
damn thing, I'm now convinced that the DMA code is fine, the XHCI
driver is correct, but that the HW (a Renesas uPD720202 chip) is a
nasty piece of work.
The issue is as follow:
- EFI initializes the controller using physical addresses above the
4GB limit (this is on an arm64 box where the memory starts at
0x80_00000000...).
- The kernel takes over, sends a XHCI reset to the controller, and
because we have an IOMMU sitting between the controller and memory,
provides *virtual* addresses. Trying to make things a bit faster for
our controller, it issues IOVAs in the low 4GB range).
- Low and behold, the controller is now issuing transactions with a
0x80 prefix in front of our IOVA. Yes, the same prefix that was
programmed during the EFI configuration. IOMMU fault, not happy.
If the kernel is hacked to only generate IOVAs that are more than
32bit wide, the HW behaves correctly. The only way I can explain this
behaviour is that the HW latches the top 32bit of the ERST (it is
always the ERST IOVA that appears in my traces) in some internal
register, and that the XHCI reset fails to clear it. Writing zero in
the top bits is not enough to clear it either.
So far, the only solution we have for this lovely piece of kit is to
force a PCI reset at probe time, which puts it right. The patches are
pretty ugly, but that's the best I could come up with so far.
Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
uPD720202 controllers.
Marc Zyngier (2):
PCI: Implement pci_reset_function_locked
usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
controller
drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
drivers/usb/host/pci-quirks.h | 1 +
drivers/usb/host/xhci-pci.c | 7 +++++++
include/linux/pci.h | 1 +
5 files changed, 64 insertions(+)
--
2.11.0
The Renesas uPD72020x XHCI controller seems to suffer from a
really annoying bug, where it may retain some of its DMA programming
across a XHCI reset, and despite the driver correctly programming new
DMA addresses. This is visible if the device has been using 64bit DMA
addresses, and is then switched to using 32bit DMA addresses. The top
32bits of the address (now zero) are ignored are replaced by the 32bit
from the *previous* programming. Sticking with 64bit DMA always works,
but doesn't seem very appropriate.
A PCI reset of the device restores the normal functionnality, which
is done at probe time. Unfortunately, this has to be done before
any quirk has been discovered, hence the intrusive nature of the fix.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
drivers/usb/host/pci-quirks.h | 1 +
drivers/usb/host/xhci-pci.c | 7 +++++++
3 files changed, 28 insertions(+)
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c40480..5bed002c7dd3 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1096,3 +1096,23 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
}
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
+
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
+{
+ /*
+ * Our dear uPD72020{1,2} friend only partially resets when
+ * asked to via the XHCI interface, and may end-up doing DMA
+ * at the wrong addresses, as it keeps the top 32bit of some
+ * addresses from its previous programming under obscure
+ * circumstances.
+ * Give it a good wack at probe time. Unfortunately, this
+ * needs to happen before we've had a chance to discover any
+ * quirk, or the system will be in a rather bad state.
+ */
+ if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+ (pdev->device == 0x0014 || pdev->device == 0x0015))
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 0222195bd5b0..dcbe4b097b33 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -14,6 +14,7 @@ void usb_amd_quirk_pll_enable(void);
void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
void sb800_prefetch(struct device *dev, int on);
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
#else
struct pci_dev;
static inline void usb_amd_quirk_pll_disable(void) {}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1bcf971141c0..3831705c2817 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -267,6 +267,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
driver = (struct hc_driver *)id->driver_data;
+ /* For some HW implementation, a XHCI reset is just not enough... */
+ if (usb_xhci_needs_pci_reset(dev)) {
+ dev_info(&dev->dev, "Resetting\n");
+ if (pci_reset_function_locked(dev))
+ dev_warn(&dev->dev, "Reset failed");
+ }
+
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(&dev->dev);
--
2.11.0
The implementation of PCI workarounds may require that the device
is reset from its probe function. This implies that the PCI device
lock is already held, and makes calling pci_reset_function impossible
(since it will itself try to take that lock).
This patch introduces pci_reset_function_locked, which is the equivalent
of pci_reset_function, except that it requires the PCI device lock to
be already held by the caller.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 36 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..a2c3e8b94f65 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4287,6 +4287,41 @@ int pci_reset_function(struct pci_dev *dev)
EXPORT_SYMBOL_GPL(pci_reset_function);
/**
+ * pci_reset_function_locked - quiesce and reset a PCI device function
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device. The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * This function does not just reset the PCI portion of a device, but
+ * clears all the state associated with the device. This function differs
+ * from __pci_reset_function in that it saves and restores device state
+ * over the reset. it also differs from pci_reset_function in that it
+ * requires the PCI device lock to be held.
+ *
+ * Returns 0 if the device function was successfully reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_reset_function_locked(struct pci_dev *dev)
+{
+ int rc;
+
+ rc = pci_dev_reset(dev, 1);
+ if (rc)
+ return rc;
+
+ pci_dev_save_and_disable(dev);
+
+ rc = __pci_dev_reset(dev, 0);
+
+ pci_dev_restore(dev);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(pci_reset_function_locked);
+
+/**
* pci_try_reset_function - quiesce and reset a PCI device function
* @dev: PCI device to reset
*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..16be18678ca1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1049,6 +1049,7 @@ void pcie_flr(struct pci_dev *dev);
int __pci_reset_function(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
+int pci_reset_function_locked(struct pci_dev *dev);
int pci_try_reset_function(struct pci_dev *dev);
int pci_probe_reset_slot(struct pci_slot *slot);
int pci_reset_slot(struct pci_slot *slot);
--
2.11.0
On 10 July 2017 at 16:52, Marc Zyngier <[email protected]> wrote:
> Ard and myself have just spent quite some time lately trying to pin
> down an issue in the DMA code which was taking the form of a PCIe USB3
> controller issuing a DMA access at some bizarre address, and being
> caught red-handed by the IOMMU.
>
> After much head scratching and most of a week-end spent on tracing the
> damn thing, I'm now convinced that the DMA code is fine, the XHCI
> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> nasty piece of work.
>
> The issue is as follow:
>
> - EFI initializes the controller using physical addresses above the
> 4GB limit (this is on an arm64 box where the memory starts at
> 0x80_00000000...).
>
> - The kernel takes over, sends a XHCI reset to the controller, and
> because we have an IOMMU sitting between the controller and memory,
> provides *virtual* addresses. Trying to make things a bit faster for
> our controller, it issues IOVAs in the low 4GB range).
>
> - Low and behold, the controller is now issuing transactions with a
> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> programmed during the EFI configuration. IOMMU fault, not happy.
>
> If the kernel is hacked to only generate IOVAs that are more than
> 32bit wide, the HW behaves correctly. The only way I can explain this
> behaviour is that the HW latches the top 32bit of the ERST (it is
> always the ERST IOVA that appears in my traces) in some internal
> register, and that the XHCI reset fails to clear it. Writing zero in
> the top bits is not enough to clear it either.
>
To clarify, this seems to be an issue in the internal DMA logic of the
controller. The ESRT base address register *is* cleared by the XHCI
reset, i.e., it reads back as all zeroes. However, any 32-bit value we
write there is extended with the high word written by the UEFI in the
actual DMA transactions that take place.
> So far, the only solution we have for this lovely piece of kit is to
> force a PCI reset at probe time, which puts it right. The patches are
> pretty ugly, but that's the best I could come up with so far.
>
> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> uPD720202 controllers.
>
> Marc Zyngier (2):
> PCI: Implement pci_reset_function_locked
> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> controller
>
> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> drivers/usb/host/pci-quirks.h | 1 +
> drivers/usb/host/xhci-pci.c | 7 +++++++
> include/linux/pci.h | 1 +
> 5 files changed, 64 insertions(+)
>
This issue was uncovered by commit 122fac030e91 ("iommu/dma: Implement
PCI allocation optimisation"), which appeared in v4.11. So if this
approach is considered appropriate, it would be nice if we could tag
it for v4.11-stable as well.
Thanks,
Ard.
On 10 July 2017 at 18:21, Ard Biesheuvel <[email protected]> wrote:
> On 10 July 2017 at 16:52, Marc Zyngier <[email protected]> wrote:
>> Ard and myself have just spent quite some time lately trying to pin
>> down an issue in the DMA code which was taking the form of a PCIe USB3
>> controller issuing a DMA access at some bizarre address, and being
>> caught red-handed by the IOMMU.
>>
>> After much head scratching and most of a week-end spent on tracing the
>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>> nasty piece of work.
>>
>> The issue is as follow:
>>
>> - EFI initializes the controller using physical addresses above the
>> 4GB limit (this is on an arm64 box where the memory starts at
>> 0x80_00000000...).
>>
>> - The kernel takes over, sends a XHCI reset to the controller, and
>> because we have an IOMMU sitting between the controller and memory,
>> provides *virtual* addresses. Trying to make things a bit faster for
>> our controller, it issues IOVAs in the low 4GB range).
>>
>> - Low and behold, the controller is now issuing transactions with a
>> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
>> programmed during the EFI configuration. IOMMU fault, not happy.
>>
>> If the kernel is hacked to only generate IOVAs that are more than
>> 32bit wide, the HW behaves correctly. The only way I can explain this
>> behaviour is that the HW latches the top 32bit of the ERST (it is
>> always the ERST IOVA that appears in my traces) in some internal
>> register, and that the XHCI reset fails to clear it. Writing zero in
>> the top bits is not enough to clear it either.
>>
>
> To clarify, this seems to be an issue in the internal DMA logic of the
> controller. The ESRT base address register *is* cleared by the XHCI
> reset, i.e., it reads back as all zeroes. However, any 32-bit value we
> write there is extended with the high word written by the UEFI in the
> actual DMA transactions that take place.
>
>> So far, the only solution we have for this lovely piece of kit is to
>> force a PCI reset at probe time, which puts it right. The patches are
>> pretty ugly, but that's the best I could come up with so far.
>>
>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>> uPD720202 controllers.
>>
>> Marc Zyngier (2):
>> PCI: Implement pci_reset_function_locked
>> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>> controller
>>
Tested-by: Ard Biesheuvel <[email protected]>
On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> Ard and myself have just spent quite some time lately trying to pin
> down an issue in the DMA code which was taking the form of a PCIe USB3
> controller issuing a DMA access at some bizarre address, and being
> caught red-handed by the IOMMU.
>
> After much head scratching and most of a week-end spent on tracing the
> damn thing, I'm now convinced that the DMA code is fine, the XHCI
> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> nasty piece of work.
>
> The issue is as follow:
>
> - EFI initializes the controller using physical addresses above the
> 4GB limit (this is on an arm64 box where the memory starts at
> 0x80_00000000...).
>
> - The kernel takes over, sends a XHCI reset to the controller, and
> because we have an IOMMU sitting between the controller and memory,
> provides *virtual* addresses. Trying to make things a bit faster for
> our controller, it issues IOVAs in the low 4GB range).
>
> - Low and behold, the controller is now issuing transactions with a
> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> programmed during the EFI configuration. IOMMU fault, not happy.
>
> If the kernel is hacked to only generate IOVAs that are more than
> 32bit wide, the HW behaves correctly. The only way I can explain this
> behaviour is that the HW latches the top 32bit of the ERST (it is
> always the ERST IOVA that appears in my traces) in some internal
> register, and that the XHCI reset fails to clear it. Writing zero in
> the top bits is not enough to clear it either.
>
> So far, the only solution we have for this lovely piece of kit is to
> force a PCI reset at probe time, which puts it right. The patches are
> pretty ugly, but that's the best I could come up with so far.
>
> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> uPD720202 controllers.
>
> Marc Zyngier (2):
> PCI: Implement pci_reset_function_locked
> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> controller
>
> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> drivers/usb/host/pci-quirks.h | 1 +
> drivers/usb/host/xhci-pci.c | 7 +++++++
> include/linux/pci.h | 1 +
> 5 files changed, 64 insertions(+)
I provisionally applied this to pci/virtualization. I'd like to have an
XHCI ack before going further, though.
I assume this only affects boxes where the firmware uses addresses above
4GB, i.e., not very many? So this is v4.14 material? Or do you think it's
important for v4.13?
Bjorn
On 13 July 2017 at 04:12, Bjorn Helgaas <[email protected]> wrote:
> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>> Ard and myself have just spent quite some time lately trying to pin
>> down an issue in the DMA code which was taking the form of a PCIe USB3
>> controller issuing a DMA access at some bizarre address, and being
>> caught red-handed by the IOMMU.
>>
>> After much head scratching and most of a week-end spent on tracing the
>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>> nasty piece of work.
>>
>> The issue is as follow:
>>
>> - EFI initializes the controller using physical addresses above the
>> 4GB limit (this is on an arm64 box where the memory starts at
>> 0x80_00000000...).
>>
>> - The kernel takes over, sends a XHCI reset to the controller, and
>> because we have an IOMMU sitting between the controller and memory,
>> provides *virtual* addresses. Trying to make things a bit faster for
>> our controller, it issues IOVAs in the low 4GB range).
>>
>> - Low and behold, the controller is now issuing transactions with a
>> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
>> programmed during the EFI configuration. IOMMU fault, not happy.
>>
>> If the kernel is hacked to only generate IOVAs that are more than
>> 32bit wide, the HW behaves correctly. The only way I can explain this
>> behaviour is that the HW latches the top 32bit of the ERST (it is
>> always the ERST IOVA that appears in my traces) in some internal
>> register, and that the XHCI reset fails to clear it. Writing zero in
>> the top bits is not enough to clear it either.
>>
>> So far, the only solution we have for this lovely piece of kit is to
>> force a PCI reset at probe time, which puts it right. The patches are
>> pretty ugly, but that's the best I could come up with so far.
>>
>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>> uPD720202 controllers.
>>
>> Marc Zyngier (2):
>> PCI: Implement pci_reset_function_locked
>> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>> controller
>>
>> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
>> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
>> drivers/usb/host/pci-quirks.h | 1 +
>> drivers/usb/host/xhci-pci.c | 7 +++++++
>> include/linux/pci.h | 1 +
>> 5 files changed, 64 insertions(+)
>
> I provisionally applied this to pci/virtualization. I'd like to have an
> XHCI ack before going further, though.
>
> I assume this only affects boxes where the firmware uses addresses above
> 4GB, i.e., not very many? So this is v4.14 material? Or do you think it's
> important for v4.13?
>
As I mentioned, it would be nice if this could at least go into v4.11
and later, given that v4.11 contains a patch that switches all PCI
devices to 32-bit addressing only when the IOMMU is involved in DMA,
and this is what triggered the issue on arm64 boards with such a PCI
card and no DRAM below 4 GB.
Thanks,
Ard.
On 13/07/17 07:48, Ard Biesheuvel wrote:
> On 13 July 2017 at 04:12, Bjorn Helgaas <[email protected]> wrote:
>> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>>> Ard and myself have just spent quite some time lately trying to pin
>>> down an issue in the DMA code which was taking the form of a PCIe USB3
>>> controller issuing a DMA access at some bizarre address, and being
>>> caught red-handed by the IOMMU.
>>>
>>> After much head scratching and most of a week-end spent on tracing the
>>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>>> nasty piece of work.
>>>
>>> The issue is as follow:
>>>
>>> - EFI initializes the controller using physical addresses above the
>>> 4GB limit (this is on an arm64 box where the memory starts at
>>> 0x80_00000000...).
>>>
>>> - The kernel takes over, sends a XHCI reset to the controller, and
>>> because we have an IOMMU sitting between the controller and memory,
>>> provides *virtual* addresses. Trying to make things a bit faster for
>>> our controller, it issues IOVAs in the low 4GB range).
>>>
>>> - Low and behold, the controller is now issuing transactions with a
>>> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
>>> programmed during the EFI configuration. IOMMU fault, not happy.
>>>
>>> If the kernel is hacked to only generate IOVAs that are more than
>>> 32bit wide, the HW behaves correctly. The only way I can explain this
>>> behaviour is that the HW latches the top 32bit of the ERST (it is
>>> always the ERST IOVA that appears in my traces) in some internal
>>> register, and that the XHCI reset fails to clear it. Writing zero in
>>> the top bits is not enough to clear it either.
>>>
>>> So far, the only solution we have for this lovely piece of kit is to
>>> force a PCI reset at probe time, which puts it right. The patches are
>>> pretty ugly, but that's the best I could come up with so far.
>>>
>>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>>> uPD720202 controllers.
>>>
>>> Marc Zyngier (2):
>>> PCI: Implement pci_reset_function_locked
>>> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>>> controller
>>>
>>> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
>>> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
>>> drivers/usb/host/pci-quirks.h | 1 +
>>> drivers/usb/host/xhci-pci.c | 7 +++++++
>>> include/linux/pci.h | 1 +
>>> 5 files changed, 64 insertions(+)
>>
>> I provisionally applied this to pci/virtualization. I'd like to have an
>> XHCI ack before going further, though.
>>
>> I assume this only affects boxes where the firmware uses addresses above
>> 4GB, i.e., not very many? So this is v4.14 material? Or do you think it's
>> important for v4.13?
>>
>
> As I mentioned, it would be nice if this could at least go into v4.11
> and later, given that v4.11 contains a patch that switches all PCI
> devices to 32-bit addressing only when the IOMMU is involved in DMA,
> and this is what triggered the issue on arm64 boards with such a PCI
> card and no DRAM below 4 GB.
Agreed. It is likely that the issue will trigger on any 64bit->32bit
IOVA transition, not only EFI->kernel, such as a kexec from a 4.10 to a
4.11 kernel.
More importantly, this could have a dramatic effect on a system where
both the 32bit and 64bit address ranges are valid. In my case, I was
saved by the IOMMU blocking the DMA access, but imagine for a second the
device was using PAs... I'm not sure that this is completely
hypothetical, nor arm64 specific.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Wed, Jul 12, 2017 at 10:12:34PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> > Ard and myself have just spent quite some time lately trying to pin
> > down an issue in the DMA code which was taking the form of a PCIe USB3
> > controller issuing a DMA access at some bizarre address, and being
> > caught red-handed by the IOMMU.
> >
> > After much head scratching and most of a week-end spent on tracing the
> > damn thing, I'm now convinced that the DMA code is fine, the XHCI
> > driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> > nasty piece of work.
> >
> > The issue is as follow:
> >
> > - EFI initializes the controller using physical addresses above the
> > 4GB limit (this is on an arm64 box where the memory starts at
> > 0x80_00000000...).
> >
> > - The kernel takes over, sends a XHCI reset to the controller, and
> > because we have an IOMMU sitting between the controller and memory,
> > provides *virtual* addresses. Trying to make things a bit faster for
> > our controller, it issues IOVAs in the low 4GB range).
> >
> > - Low and behold, the controller is now issuing transactions with a
> > 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> > programmed during the EFI configuration. IOMMU fault, not happy.
> >
> > If the kernel is hacked to only generate IOVAs that are more than
> > 32bit wide, the HW behaves correctly. The only way I can explain this
> > behaviour is that the HW latches the top 32bit of the ERST (it is
> > always the ERST IOVA that appears in my traces) in some internal
> > register, and that the XHCI reset fails to clear it. Writing zero in
> > the top bits is not enough to clear it either.
> >
> > So far, the only solution we have for this lovely piece of kit is to
> > force a PCI reset at probe time, which puts it right. The patches are
> > pretty ugly, but that's the best I could come up with so far.
> >
> > Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> > uPD720202 controllers.
> >
> > Marc Zyngier (2):
> > PCI: Implement pci_reset_function_locked
> > usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> > controller
> >
> > drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> > drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> > drivers/usb/host/pci-quirks.h | 1 +
> > drivers/usb/host/xhci-pci.c | 7 +++++++
> > include/linux/pci.h | 1 +
> > 5 files changed, 64 insertions(+)
>
> I provisionally applied this to pci/virtualization. I'd like to have an
> XHCI ack before going further, though.
The xhci maintainer is on vacation, let's wait a week for him to get
back to get this. Given the long time that this has been broken on this
hardware, I think we can wait another week just fine :)
thanks,
greg k-h
On Thu, Jul 13, 2017 at 08:46:45AM +0100, Marc Zyngier wrote:
> On 13/07/17 07:48, Ard Biesheuvel wrote:
> > On 13 July 2017 at 04:12, Bjorn Helgaas <[email protected]> wrote:
> >> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> >>> Ard and myself have just spent quite some time lately trying to pin
> >>> down an issue in the DMA code which was taking the form of a PCIe USB3
> >>> controller issuing a DMA access at some bizarre address, and being
> >>> caught red-handed by the IOMMU.
> >>>
> >>> After much head scratching and most of a week-end spent on tracing the
> >>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
> >>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> >>> nasty piece of work.
> >>>
> >>> The issue is as follow:
> >>>
> >>> - EFI initializes the controller using physical addresses above the
> >>> 4GB limit (this is on an arm64 box where the memory starts at
> >>> 0x80_00000000...).
> >>>
> >>> - The kernel takes over, sends a XHCI reset to the controller, and
> >>> because we have an IOMMU sitting between the controller and memory,
> >>> provides *virtual* addresses. Trying to make things a bit faster for
> >>> our controller, it issues IOVAs in the low 4GB range).
> >>>
> >>> - Low and behold, the controller is now issuing transactions with a
> >>> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> >>> programmed during the EFI configuration. IOMMU fault, not happy.
> >>>
> >>> If the kernel is hacked to only generate IOVAs that are more than
> >>> 32bit wide, the HW behaves correctly. The only way I can explain this
> >>> behaviour is that the HW latches the top 32bit of the ERST (it is
> >>> always the ERST IOVA that appears in my traces) in some internal
> >>> register, and that the XHCI reset fails to clear it. Writing zero in
> >>> the top bits is not enough to clear it either.
> >>>
> >>> So far, the only solution we have for this lovely piece of kit is to
> >>> force a PCI reset at probe time, which puts it right. The patches are
> >>> pretty ugly, but that's the best I could come up with so far.
> >>>
> >>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> >>> uPD720202 controllers.
> >>>
> >>> Marc Zyngier (2):
> >>> PCI: Implement pci_reset_function_locked
> >>> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> >>> controller
> >>>
> >>> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> >>> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> >>> drivers/usb/host/pci-quirks.h | 1 +
> >>> drivers/usb/host/xhci-pci.c | 7 +++++++
> >>> include/linux/pci.h | 1 +
> >>> 5 files changed, 64 insertions(+)
> >>
> >> I provisionally applied this to pci/virtualization. I'd like to have an
> >> XHCI ack before going further, though.
> >>
> >> I assume this only affects boxes where the firmware uses addresses above
> >> 4GB, i.e., not very many? So this is v4.14 material? Or do you think it's
> >> important for v4.13?
> >>
> >
> > As I mentioned, it would be nice if this could at least go into v4.11
> > and later, given that v4.11 contains a patch that switches all PCI
> > devices to 32-bit addressing only when the IOMMU is involved in DMA,
> > and this is what triggered the issue on arm64 boards with such a PCI
> > card and no DRAM below 4 GB.
>
> Agreed. It is likely that the issue will trigger on any 64bit->32bit
> IOVA transition, not only EFI->kernel, such as a kexec from a 4.10 to a
> 4.11 kernel.
>
> More importantly, this could have a dramatic effect on a system where
> both the 32bit and 64bit address ranges are valid. In my case, I was
> saved by the IOMMU blocking the DMA access, but imagine for a second the
> device was using PAs... I'm not sure that this is completely
> hypothetical, nor arm64 specific.
I did add a v4.11+ stable tag on your behalf, so it will get
backported to eventually. But your responses don't exactly answer my
question about whether you want to start with this in v4.13 or v4.14.
On Thu, Jul 13, 2017 at 10:26:40AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 12, 2017 at 10:12:34PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> > > Ard and myself have just spent quite some time lately trying to pin
> > > down an issue in the DMA code which was taking the form of a PCIe USB3
> > > controller issuing a DMA access at some bizarre address, and being
> > > caught red-handed by the IOMMU.
> > >
> > > After much head scratching and most of a week-end spent on tracing the
> > > damn thing, I'm now convinced that the DMA code is fine, the XHCI
> > > driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> > > nasty piece of work.
> > >
> > > The issue is as follow:
> > >
> > > - EFI initializes the controller using physical addresses above the
> > > 4GB limit (this is on an arm64 box where the memory starts at
> > > 0x80_00000000...).
> > >
> > > - The kernel takes over, sends a XHCI reset to the controller, and
> > > because we have an IOMMU sitting between the controller and memory,
> > > provides *virtual* addresses. Trying to make things a bit faster for
> > > our controller, it issues IOVAs in the low 4GB range).
> > >
> > > - Low and behold, the controller is now issuing transactions with a
> > > 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> > > programmed during the EFI configuration. IOMMU fault, not happy.
> > >
> > > If the kernel is hacked to only generate IOVAs that are more than
> > > 32bit wide, the HW behaves correctly. The only way I can explain this
> > > behaviour is that the HW latches the top 32bit of the ERST (it is
> > > always the ERST IOVA that appears in my traces) in some internal
> > > register, and that the XHCI reset fails to clear it. Writing zero in
> > > the top bits is not enough to clear it either.
> > >
> > > So far, the only solution we have for this lovely piece of kit is to
> > > force a PCI reset at probe time, which puts it right. The patches are
> > > pretty ugly, but that's the best I could come up with so far.
> > >
> > > Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> > > uPD720202 controllers.
> > >
> > > Marc Zyngier (2):
> > > PCI: Implement pci_reset_function_locked
> > > usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> > > controller
> > >
> > > drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> > > drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> > > drivers/usb/host/pci-quirks.h | 1 +
> > > drivers/usb/host/xhci-pci.c | 7 +++++++
> > > include/linux/pci.h | 1 +
> > > 5 files changed, 64 insertions(+)
> >
> > I provisionally applied this to pci/virtualization. I'd like to have an
> > XHCI ack before going further, though.
>
> The xhci maintainer is on vacation, let's wait a week for him to get
> back to get this. Given the long time that this has been broken on this
> hardware, I think we can wait another week just fine :)
I'll be on vacation myself until the beginning of August, so no problem
with that.
On 13/07/17 12:36, Bjorn Helgaas wrote:
> On Thu, Jul 13, 2017 at 08:46:45AM +0100, Marc Zyngier wrote:
>> On 13/07/17 07:48, Ard Biesheuvel wrote:
>>> On 13 July 2017 at 04:12, Bjorn Helgaas <[email protected]> wrote:
>>>> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>>>>> Ard and myself have just spent quite some time lately trying to pin
>>>>> down an issue in the DMA code which was taking the form of a PCIe USB3
>>>>> controller issuing a DMA access at some bizarre address, and being
>>>>> caught red-handed by the IOMMU.
>>>>>
>>>>> After much head scratching and most of a week-end spent on tracing the
>>>>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>>>>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>>>>> nasty piece of work.
>>>>>
>>>>> The issue is as follow:
>>>>>
>>>>> - EFI initializes the controller using physical addresses above the
>>>>> 4GB limit (this is on an arm64 box where the memory starts at
>>>>> 0x80_00000000...).
>>>>>
>>>>> - The kernel takes over, sends a XHCI reset to the controller, and
>>>>> because we have an IOMMU sitting between the controller and memory,
>>>>> provides *virtual* addresses. Trying to make things a bit faster for
>>>>> our controller, it issues IOVAs in the low 4GB range).
>>>>>
>>>>> - Low and behold, the controller is now issuing transactions with a
>>>>> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
>>>>> programmed during the EFI configuration. IOMMU fault, not happy.
>>>>>
>>>>> If the kernel is hacked to only generate IOVAs that are more than
>>>>> 32bit wide, the HW behaves correctly. The only way I can explain this
>>>>> behaviour is that the HW latches the top 32bit of the ERST (it is
>>>>> always the ERST IOVA that appears in my traces) in some internal
>>>>> register, and that the XHCI reset fails to clear it. Writing zero in
>>>>> the top bits is not enough to clear it either.
>>>>>
>>>>> So far, the only solution we have for this lovely piece of kit is to
>>>>> force a PCI reset at probe time, which puts it right. The patches are
>>>>> pretty ugly, but that's the best I could come up with so far.
>>>>>
>>>>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>>>>> uPD720202 controllers.
>>>>>
>>>>> Marc Zyngier (2):
>>>>> PCI: Implement pci_reset_function_locked
>>>>> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>>>>> controller
>>>>>
>>>>> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
>>>>> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
>>>>> drivers/usb/host/pci-quirks.h | 1 +
>>>>> drivers/usb/host/xhci-pci.c | 7 +++++++
>>>>> include/linux/pci.h | 1 +
>>>>> 5 files changed, 64 insertions(+)
>>>>
>>>> I provisionally applied this to pci/virtualization. I'd like to have an
>>>> XHCI ack before going further, though.
>>>>
>>>> I assume this only affects boxes where the firmware uses addresses above
>>>> 4GB, i.e., not very many? So this is v4.14 material? Or do you think it's
>>>> important for v4.13?
>>>>
>>>
>>> As I mentioned, it would be nice if this could at least go into v4.11
>>> and later, given that v4.11 contains a patch that switches all PCI
>>> devices to 32-bit addressing only when the IOMMU is involved in DMA,
>>> and this is what triggered the issue on arm64 boards with such a PCI
>>> card and no DRAM below 4 GB.
>>
>> Agreed. It is likely that the issue will trigger on any 64bit->32bit
>> IOVA transition, not only EFI->kernel, such as a kexec from a 4.10 to a
>> 4.11 kernel.
>>
>> More importantly, this could have a dramatic effect on a system where
>> both the 32bit and 64bit address ranges are valid. In my case, I was
>> saved by the IOMMU blocking the DMA access, but imagine for a second the
>> device was using PAs... I'm not sure that this is completely
>> hypothetical, nor arm64 specific.
>
> I did add a v4.11+ stable tag on your behalf, so it will get
> backported to eventually. But your responses don't exactly answer my
> question about whether you want to start with this in v4.13 or v4.14.
Thanks for that. The sooner the better, so I'd be tempted to say 4.13,
assuming we can have an Ack from the XHCI maintainer in a reasonable
time frame.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 10.07.2017 18:52, Marc Zyngier wrote:
> The Renesas uPD72020x XHCI controller seems to suffer from a
> really annoying bug, where it may retain some of its DMA programming
> across a XHCI reset, and despite the driver correctly programming new
> DMA addresses. This is visible if the device has been using 64bit DMA
> addresses, and is then switched to using 32bit DMA addresses. The top
> 32bits of the address (now zero) are ignored are replaced by the 32bit
> from the *previous* programming. Sticking with 64bit DMA always works,
> but doesn't seem very appropriate.
>
> A PCI reset of the device restores the normal functionnality, which
> is done at probe time. Unfortunately, this has to be done before
> any quirk has been discovered, hence the intrusive nature of the fix.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> drivers/usb/host/pci-quirks.h | 1 +
> drivers/usb/host/xhci-pci.c | 7 +++++++
> 3 files changed, 28 insertions(+)
>
Acked-by: Mathias Nyman <[email protected]>
On Thu, Jul 13, 2017 at 10:26:40AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 12, 2017 at 10:12:34PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> > > Ard and myself have just spent quite some time lately trying to pin
> > > down an issue in the DMA code which was taking the form of a PCIe USB3
> > > controller issuing a DMA access at some bizarre address, and being
> > > caught red-handed by the IOMMU.
> > >
> > > After much head scratching and most of a week-end spent on tracing the
> > > damn thing, I'm now convinced that the DMA code is fine, the XHCI
> > > driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> > > nasty piece of work.
> > >
> > > The issue is as follow:
> > >
> > > - EFI initializes the controller using physical addresses above the
> > > 4GB limit (this is on an arm64 box where the memory starts at
> > > 0x80_00000000...).
> > >
> > > - The kernel takes over, sends a XHCI reset to the controller, and
> > > because we have an IOMMU sitting between the controller and memory,
> > > provides *virtual* addresses. Trying to make things a bit faster for
> > > our controller, it issues IOVAs in the low 4GB range).
> > >
> > > - Low and behold, the controller is now issuing transactions with a
> > > 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> > > programmed during the EFI configuration. IOMMU fault, not happy.
> > >
> > > If the kernel is hacked to only generate IOVAs that are more than
> > > 32bit wide, the HW behaves correctly. The only way I can explain this
> > > behaviour is that the HW latches the top 32bit of the ERST (it is
> > > always the ERST IOVA that appears in my traces) in some internal
> > > register, and that the XHCI reset fails to clear it. Writing zero in
> > > the top bits is not enough to clear it either.
> > >
> > > So far, the only solution we have for this lovely piece of kit is to
> > > force a PCI reset at probe time, which puts it right. The patches are
> > > pretty ugly, but that's the best I could come up with so far.
> > >
> > > Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> > > uPD720202 controllers.
> > >
> > > Marc Zyngier (2):
> > > PCI: Implement pci_reset_function_locked
> > > usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> > > controller
> > >
> > > drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> > > drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> > > drivers/usb/host/pci-quirks.h | 1 +
> > > drivers/usb/host/xhci-pci.c | 7 +++++++
> > > include/linux/pci.h | 1 +
> > > 5 files changed, 64 insertions(+)
> >
> > I provisionally applied this to pci/virtualization. I'd like to have an
> > XHCI ack before going further, though.
>
> The xhci maintainer is on vacation, let's wait a week for him to get
> back to get this. Given the long time that this has been broken on this
> hardware, I think we can wait another week just fine :)
Ping, in case Mathias is back from vacation :)
On 1 August 2017 at 22:44, Bjorn Helgaas <[email protected]> wrote:
> On Thu, Jul 13, 2017 at 10:26:40AM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Jul 12, 2017 at 10:12:34PM -0500, Bjorn Helgaas wrote:
>> > On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>> > > Ard and myself have just spent quite some time lately trying to pin
>> > > down an issue in the DMA code which was taking the form of a PCIe USB3
>> > > controller issuing a DMA access at some bizarre address, and being
>> > > caught red-handed by the IOMMU.
>> > >
>> > > After much head scratching and most of a week-end spent on tracing the
>> > > damn thing, I'm now convinced that the DMA code is fine, the XHCI
>> > > driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>> > > nasty piece of work.
>> > >
>> > > The issue is as follow:
>> > >
>> > > - EFI initializes the controller using physical addresses above the
>> > > 4GB limit (this is on an arm64 box where the memory starts at
>> > > 0x80_00000000...).
>> > >
>> > > - The kernel takes over, sends a XHCI reset to the controller, and
>> > > because we have an IOMMU sitting between the controller and memory,
>> > > provides *virtual* addresses. Trying to make things a bit faster for
>> > > our controller, it issues IOVAs in the low 4GB range).
>> > >
>> > > - Low and behold, the controller is now issuing transactions with a
>> > > 0x80 prefix in front of our IOVA. Yes, the same prefix that was
>> > > programmed during the EFI configuration. IOMMU fault, not happy.
>> > >
>> > > If the kernel is hacked to only generate IOVAs that are more than
>> > > 32bit wide, the HW behaves correctly. The only way I can explain this
>> > > behaviour is that the HW latches the top 32bit of the ERST (it is
>> > > always the ERST IOVA that appears in my traces) in some internal
>> > > register, and that the XHCI reset fails to clear it. Writing zero in
>> > > the top bits is not enough to clear it either.
>> > >
>> > > So far, the only solution we have for this lovely piece of kit is to
>> > > force a PCI reset at probe time, which puts it right. The patches are
>> > > pretty ugly, but that's the best I could come up with so far.
>> > >
>> > > Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>> > > uPD720202 controllers.
>> > >
>> > > Marc Zyngier (2):
>> > > PCI: Implement pci_reset_function_locked
>> > > usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>> > > controller
>> > >
>> > > drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
>> > > drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
>> > > drivers/usb/host/pci-quirks.h | 1 +
>> > > drivers/usb/host/xhci-pci.c | 7 +++++++
>> > > include/linux/pci.h | 1 +
>> > > 5 files changed, 64 insertions(+)
>> >
>> > I provisionally applied this to pci/virtualization. I'd like to have an
>> > XHCI ack before going further, though.
>>
>> The xhci maintainer is on vacation, let's wait a week for him to get
>> back to get this. Given the long time that this has been broken on this
>> hardware, I think we can wait another week just fine :)
>
> Ping, in case Mathias is back from vacation :)
Actually, Mathias already acked these changes, in reply to 2/2
On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
> Ard and myself have just spent quite some time lately trying to pin
> down an issue in the DMA code which was taking the form of a PCIe USB3
> controller issuing a DMA access at some bizarre address, and being
> caught red-handed by the IOMMU.
>
> After much head scratching and most of a week-end spent on tracing the
> damn thing, I'm now convinced that the DMA code is fine, the XHCI
> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
> nasty piece of work.
>
> The issue is as follow:
>
> - EFI initializes the controller using physical addresses above the
> 4GB limit (this is on an arm64 box where the memory starts at
> 0x80_00000000...).
>
> - The kernel takes over, sends a XHCI reset to the controller, and
> because we have an IOMMU sitting between the controller and memory,
> provides *virtual* addresses. Trying to make things a bit faster for
> our controller, it issues IOVAs in the low 4GB range).
>
> - Low and behold, the controller is now issuing transactions with a
> 0x80 prefix in front of our IOVA. Yes, the same prefix that was
> programmed during the EFI configuration. IOMMU fault, not happy.
>
> If the kernel is hacked to only generate IOVAs that are more than
> 32bit wide, the HW behaves correctly. The only way I can explain this
> behaviour is that the HW latches the top 32bit of the ERST (it is
> always the ERST IOVA that appears in my traces) in some internal
> register, and that the XHCI reset fails to clear it. Writing zero in
> the top bits is not enough to clear it either.
>
> So far, the only solution we have for this lovely piece of kit is to
> force a PCI reset at probe time, which puts it right. The patches are
> pretty ugly, but that's the best I could come up with so far.
>
> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
> uPD720202 controllers.
>
> Marc Zyngier (2):
> PCI: Implement pci_reset_function_locked
> usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
> controller
>
> drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
> drivers/usb/host/pci-quirks.c | 20 ++++++++++++++++++++
> drivers/usb/host/pci-quirks.h | 1 +
> drivers/usb/host/xhci-pci.c | 7 +++++++
> include/linux/pci.h | 1 +
> 5 files changed, 64 insertions(+)
Applied with Mathias' ack on the XHCI change to for-linus for v4.13,
thanks!