[PATCH] iommu: Add config option to set passthrough as default
This allows the default behavior to be controlled by a kernel config
option instead of changing the commandline for the kernel to include
"iommu.passthrough=on" or "iommu=pt" on machines where this is desired.
Likewise, for machines where this config option is enabled, it can be
disabled at boot time with "iommu.passthrough=off" or "iommu=nopt".
Also corrected iommu=pt documentation for IA-64, since it has no code that
parses iommu= at all.
Signed-off-by: Olof Johansson
---
Chances since v1:
- Added analogous behavior for Intel/AMD
- Added iommu=nopt for x86 and updated docs
Documentation/admin-guide/kernel-parameters.txt | 3 ++-
arch/x86/kernel/pci-dma.c | 8
drivers/iommu/Kconfig | 11 +++
drivers/iommu/iommu.c | 4
4 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index c9dbdf1009f1..4c822aa50f13 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1744,7 +1744,8 @@
merge
nomerge
soft
- pt [x86, IA-64]
+ pt [x86]
+ nopt[x86]
nobypass[PPC/POWERNV]
Disable IOMMU bypass, using IOMMU for PCI devices.
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index ab5d9dd668d2..0acb135de7fb 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -40,8 +40,14 @@ int iommu_detected __read_mostly = 0;
* devices and allow every device to access to whole physical memory. This is
* useful if a user wants to use an IOMMU only for KVM device assignment to
* guests and not for driver dma translation.
+ * It is also possible to disable by default in kernel config, and enable with
+ * iommu=nopt at boot time.
*/
+#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
+int iommu_pass_through __read_mostly = 1;
+#else
int iommu_pass_through __read_mostly;
+#endif
extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
@@ -135,6 +141,8 @@ static __init int iommu_setup(char *p)
#endif
if (!strncmp(p, "pt", 2))
iommu_pass_through = 1;
+ if (!strncmp(p, "nopt", 4))
+ iommu_pass_through = 0;
gart_parse_options(p);
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 568ae81b0e99..1813319c8342 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -70,6 +70,17 @@ config IOMMU_DEBUGFS
debug/iommu directory, and then populate a subdirectory with
entries as required.
+config IOMMU_DEFAULT_PASSTHROUGH
+ bool "IOMMU passthrough by default"
+ depends on IOMMU_API
+help
+ Enable passthrough by default, removing the need to pass in
+ iommu.passthrough=on or iommu=pt through command line. If this
+ is enabled, you can still disable with iommu.passthrough=off
+ or iommu=nopt depending on the architecture.
+
+ If unsure, say N here.
+
config IOMMU_IOVA
tristate
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d227b864a109..6f8f59684def 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -36,7 +36,11 @@
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
+#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
+static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
+#else
static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
+#endif
struct iommu_callback_data {
const struct iommu_ops *ops;
--
2.11.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu: Add config option to set passthrough as default
On Fri, Jul 20, 2018 at 5:16 AM, Joerg Roedel wrote: > Hi Olof, > > On Wed, Jul 11, 2018 at 01:59:35PM -0700, Olof Johansson wrote: >> +config IOMMU_DEFAULT_PASSTHROUGH >> + bool "IOMMU passthrough by default" >> + depends on IOMMU_API >> +help >> + Enable passthrough by default (removing the need to pass in >> + iommu.passthrough=on through command line). If this is enabled, >> + you can still disable with iommu.passthrough=off >> + >> + If unsure, say N here. >> + > > The patch is a good start, but the description above indicates that it > affects all IOMMU driver, which it does not. Please make the Intel and > AMD IOMMU drivers also take this option into account. It looks like it should make the AMD driver should honor it, since it uses the generic infrastructure for domain types? But it also shares iommu_pass_through variable usage with Intel, so if I change it over there it'll be covered for sure. One unfortunate thing here is the divergence in command line options between arm64 and x86. I'll add a 'iommu=nopt' on x86 so it can be turned off at runtime if enabled in config, but it'd be nice to also have it adhere to the .passthrough options. That's a larger topic than just this specific patch though. Posting new patch shortly. -Olof ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/ipmmu-vmsa: Fix allocation in atomic context
When attaching a device to an IOMMU group with
CONFIG_DEBUG_ATOMIC_SLEEP=y:
BUG: sleeping function called from invalid context at mm/slab.h:421
in_atomic(): 1, irqs_disabled(): 128, pid: 61, name: kworker/1:1
...
Call trace:
...
arm_lpae_alloc_pgtable+0x114/0x184
arm_64_lpae_alloc_pgtable_s1+0x2c/0x128
arm_32_lpae_alloc_pgtable_s1+0x40/0x6c
alloc_io_pgtable_ops+0x60/0x88
ipmmu_attach_device+0x140/0x334
ipmmu_attach_device() takes a spinlock, while arm_lpae_alloc_pgtable()
allocates memory using GFP_KERNEL. Originally, the ipmmu-vmsa driver
had its own custom page table allocation implementation using
GFP_ATOMIC, hence the spinlock was fine.
Fix this by replacing the spinlock by a mutex, like the arm-smmu driver
does.
Fixes: f20ed39f53145e45 ("iommu/ipmmu-vmsa: Use the ARM LPAE page table
allocator")
Signed-off-by: Geert Uytterhoeven
---
drivers/iommu/ipmmu-vmsa.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 6a0e7142f41bf667..8f54f25404456035 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -73,7 +73,7 @@ struct ipmmu_vmsa_domain {
struct io_pgtable_ops *iop;
unsigned int context_id;
- spinlock_t lock;/* Protects mappings */
+ struct mutex mutex; /* Protects mappings */
};
static struct ipmmu_vmsa_domain *to_vmsa_domain(struct iommu_domain *dom)
@@ -599,7 +599,7 @@ static struct iommu_domain *__ipmmu_domain_alloc(unsigned
type)
if (!domain)
return NULL;
- spin_lock_init(>lock);
+ mutex_init(>mutex);
return >io_domain;
}
@@ -645,7 +645,6 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
struct iommu_fwspec *fwspec = dev->iommu_fwspec;
struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
- unsigned long flags;
unsigned int i;
int ret = 0;
@@ -654,7 +653,7 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
return -ENXIO;
}
- spin_lock_irqsave(>lock, flags);
+ mutex_lock(>mutex);
if (!domain->mmu) {
/* The domain hasn't been used yet, initialize it. */
@@ -678,7 +677,7 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
} else
dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
- spin_unlock_irqrestore(>lock, flags);
+ mutex_unlock(>mutex);
if (ret < 0)
return ret;
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] Docs: dt: arm-smmu: Add optional clock parameter
On Fri, Jul 13, 2018 at 11:27:56AM -0500, thor.tha...@linux.intel.com wrote: > From: Thor Thayer > > Add a clock to the SMMU node bindings. > > Signed-off-by: Thor Thayer > --- > Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > index 8a6ffce12af5..356fd9f41e1b 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > @@ -71,6 +71,8 @@ conditions. >or using stream matching with #iommu-cells = <2>, and >may be ignored if present in such cases. > > +- clock: clock provider specifier > + The TRM says there is a TCU clock and clock per TBU. Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[git pull] IOMMU Fixes for Linux v4.18-rc5
Hi Linus, The following changes since commit 9d3cce1e8b8561fed5f383d22a4d6949db4eadbe: Linux 4.18-rc5 (2018-07-15 12:49:31 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git tags/iommu-fixes-v4.18-rc5 for you to fetch changes up to 2db1581e1f432ac6b4efe152c57fdfb4de85c154: Revert "iommu/vt-d: Clean up pasid quirk for pre-production devices" (2018-07-20 13:55:56 +0200) IOMMU Fixes for Linux v4.18-rc5: Only one revert: * Revert an Intel VT-d patch that caused issues with the i915 GPU driver Lu Baolu (1): Revert "iommu/vt-d: Clean up pasid quirk for pre-production devices" drivers/iommu/intel-iommu.c | 32 ++-- include/linux/intel-iommu.h | 1 + 2 files changed, 31 insertions(+), 2 deletions(-) Please pull. Thanks, Joerg signature.asc Description: Digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: usb HC busted?
On Fri, 20 Jul 2018, Mathias Nyman wrote: > >> But we need to fix this properly as well. > >> xhci needs to be more in sync with usb core in usb_set_interface(), > >> currently xhci > >> has the altssetting up and running when usb core hasn't event started > >> flushing endpoints. > > > > Absolutely. The core tries to be compatible with host controller > > drivers that either allocate bandwidth as it is requested or else > > allocate bandwidth all at once when an altsetting is installed. > > > > xhci-hcd falls into the second category. However, this approach > > requires the bandwidth verification for the new altsetting to be > > performed before the old altsetting has been disabled, and the xHCI > > hardware can't do this. > > > > We may need to change the core so that the old endpoints are disabled > > before the bandwidth check is done, instead of after. Of course, this > > leads to an awkward situation if the check fails -- we'd probably have > > to go back and re-install the old altsetting. > > That would help xhci a lot. > > If we want to avoid the awkward altsetting re-install after bandwidth failure > then adding a extra endpoint flush before checking the bandwidth would > already help a lot. > > The endpoint disabling can then be remain after bandwidth checking. > Does that work for other host controllers? As far as I know, the other host controller drivers don't really care how this is done. xHCI is the only technology where the hardware has to verify the bandwidth requirements. (Maybe some other SuperSpeed controller design also cares, but if so then this change is unlikely to hurt.) Alan Stern ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: use the generic dma-noncoherent code for microblaze
On Fri, Jul 20, 2018 at 02:33:44PM +0200, Michal Simek wrote: > Hi, > > On 19.7.2018 14:54, Christoph Hellwig wrote: > > Hi Michal, > > > > can you review these patches to switch microblaze to use the generic > > dma-noncoherent code? All the requirements are in mainline already > > and we've switched various architectures over to it already. > > > > I can't see any issue with them. > Both applied to next. ! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: usb HC busted?
Hi Mathias, On Fri, Jul 20, 2018 at 02:10:58PM +0300, Mathias Nyman wrote: > On 19.07.2018 20:32, Sudip Mukherjee wrote: > > Hi Mathias, > > > > On Thu, Jul 19, 2018 at 06:42:19PM +0300, Mathias Nyman wrote: > > > > > As first aid I could try to implement checks that make sure the > > > > > flushed URBs > > > > > trb pointers really are on the current endpoint ring, and also add > > > > > some warning > > > > > if we are we are dropping endpoints with URBs still queued. > > > > > > > > Yes, please. I think your first-aid will be a much better option than > > > > the hacky patch I am using atm. > > > > > > > > So poison is overwritten at e5acda58 with almost its own address, (reading > backwards) e5 ac da 60, twice. > looks like something (32bit?)is pointing to itself twice, maybe a linked list > node next and prev pointer > being set to point to itself as last item was removed from list. > > The cancelled_td_list is part of struct xhci_virt_ep, so that should be fine. > But td_list is part of struct xhci_ring, which was freed. and we removed the > URBs tds from the td_list when > flushing the ring after ring was freed > > I changed the patch (attached) to make sure it doesn't touch the td_list when > canceling a URB after > ring is freed. > > How about this one, any improvements? Yes, it worked. :D So, cycle-1 = no change, just to make sure I can still reproduce the error. cycle-2 and cycle-3 with your patch, and there was no problem, slub debug was also happy. I am starting an autotest with this patch now, and I will have almost 50 cycles tested by tomorrow morning. -- Regards Sudip ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 0/9] iommu/vt-d: Improve PASID id and table management
On Sat, Jul 14, 2018 at 03:46:53PM +0800, Lu Baolu wrote: > Lu Baolu (9): > iommu/vt-d: Global PASID name space > iommu/vt-d: Avoid using idr_for_each_entry() > iommu/vt-d: Apply global PASID in SVA > iommu/vt-d: Move device_domain_info to header > iommu/vt-d: Add for_each_device_domain() helper > iommu/vt-d: Per PCI device pasid table interfaces > iommu/vt-d: Allocate and free pasid table > iommu/vt-d: Apply per pci device pasid table in SVA > iommu/vt-d: Remove the obsolete per iommu pasid tables Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Remove redundant WARN_ON()
On Fri, Jul 20, 2018 at 10:45:45AM +0200, Anna-Maria Gleixner wrote:
> The WARN_ON() was introduced in commit 272e4f99e966 ("iommu/amd: WARN
> when __[attach|detach]_device are called with irqs enabled") to ensure
> that the domain->lock is taken in proper irqs disabled context. This
> is required, because the domain->lock is taken as well in irq
> context.
>
> The proper context check by the WARN_ON() is redundant, because it is
> already covered by LOCKDEP. When working with locks and changing
> context, a run with LOCKDEP is required anyway and would detect the
> wrong lock context.
>
> Furthermore all callers for those functions are within the same file
> and all callers acquire another lock which already disables interrupts.
>
> Signed-off-by: Anna-Maria Gleixner
> ---
> drivers/iommu/amd_iommu.c | 12
> 1 file changed, 12 deletions(-)
Applied, thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: use the generic dma-noncoherent code for microblaze
Hi, On 19.7.2018 14:54, Christoph Hellwig wrote: > Hi Michal, > > can you review these patches to switch microblaze to use the generic > dma-noncoherent code? All the requirements are in mainline already > and we've switched various architectures over to it already. > I can't see any issue with them. Both applied to next. Thanks, Michal ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: AMD-Vi: Decrease time of enabling lazy IO/TLB flushing
On Tue, Jul 17, 2018 at 06:07:07PM +0200, Paul Menzel wrote: > On a MSI B350M MORTAR with AMD Ryzen 3 2200g (Raven) with Linux 4.18-rc5+ > and Debian Sid/unstable `pci_iommu_init` takes 40 ms according to > `initcall_debug`. 40ms is a lot indeed, but IOMMU initialization is also a complex process which includes scanning ACPI tables, allocating memory, flushing hardware caches (which probably accounts for a fair amount of the time needed). Would be good if someone can come up with patches to improve the situation here, as I don't know when I will find the time to look into that. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: MSI B350M MORTAR: `AMD-Vi: Unable to write to IOMMU perf counter.` and `pci 0000:00:00.2: can't derive routing for PCI INT A`
Hi Paul, On Tue, Jul 17, 2018 at 06:02:07PM +0200, Paul Menzel wrote: > $ dmesg > […] > [0.145696] calling pci_iommu_init+0x0/0x3f @ 1 > [0.145719] AMD-Vi: Unable to write to IOMMU perf counter. This is likely a firmware issue. Either the IVRS ACPI table is incorrect or the BIOS did not enable the performance counter feature in the IOMMU hardware. Are you running on the latest BIOS? Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu: Add config option to set passthrough as default
Hi Olof, On Wed, Jul 11, 2018 at 01:59:35PM -0700, Olof Johansson wrote: > +config IOMMU_DEFAULT_PASSTHROUGH > + bool "IOMMU passthrough by default" > + depends on IOMMU_API > +help > + Enable passthrough by default (removing the need to pass in > + iommu.passthrough=on through command line). If this is enabled, > + you can still disable with iommu.passthrough=off > + > + If unsure, say N here. > + The patch is a good start, but the description above indicates that it affects all IOMMU driver, which it does not. Please make the Intel and AMD IOMMU drivers also take this option into account. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/ipmmu-vmsa: IMUCTRn.TTSEL needs a special usage on R-Car Gen3
On Mon, Jul 09, 2018 at 11:53:31AM +0900, Yoshihiro Shimoda wrote: > The TTSEL bit of IMUCTRn register of R-Car Gen3 needs to be set > unused MMU context number even if uTLBs are disabled > (The MMUEN bit of IMUCTRn register = 0). > Since initial values of IMUCTRn.TTSEL on all IPMMU-domains are 0, > this patch adds a new feature "reserved_context" to reserve IPMMU > context number 0 as the unused MMU context. > > Signed-off-by: Yoshihiro Shimoda > --- > drivers/iommu/ipmmu-vmsa.c | 8 > 1 file changed, 8 insertions(+) Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] Revert "iommu/vt-d: Clean up pasid quirk for pre-production devices"
On Sun, Jul 08, 2018 at 02:23:21PM +0800, Lu Baolu wrote:
> This reverts commit ab96746aaa344fb720a198245a837e266fad3b62.
>
> The commit ab96746aaa34 ("iommu/vt-d: Clean up pasid quirk for
> pre-production devices") triggers ECS mode on some platforms
> which have broken ECS support. As the result, graphic device
> will be inoperable on boot.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107017
>
> Cc: Ashok Raj
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/intel-iommu.c | 32 ++--
> include/linux/intel-iommu.h | 1 +
> 2 files changed, 31 insertions(+), 2 deletions(-)
Applied to iommu/fixes.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: usb HC busted?
On 19.07.2018 17:57, Alan Stern wrote: On Thu, 19 Jul 2018, Mathias Nyman wrote: xhci driver will set up all the endpoints for the new altsetting already in usb_hcd_alloc_bandwidth(). New endpoints will be ready and rings running after this. I don't know the exact history behind this, but I assume it is because xhci does all of the steps to drop/add, disable/enable endpoints and check bandwidth in a single configure endpoint command, that will return errors if there is not enough bandwidth. That's right; Sarah and I spent some time going over this while she was working on it. But it looks like the approach isn't adequate. This command is issued in hcd->driver->check_bandwidth() This means that xhci doesn't really do much in hcd->driver->endpoint_disable or hcd->driver->endpoint_enable It also means that xhci driver assumes rings are empty when hcd->driver->check_bandwidth is called. It will bluntly free dropped rings. If there are URBs left on a endpoint ring that was dropped+added (freed+reallocated) then those URBs will contain pointers to freed ring, causing issues when usb_hcd_flush_endpoint() cancels those URBs. usb_set_interface() usb_hcd_alloc_bandwidth() hcd->driver->drop_endpoint() hcd->driver->add_endpoint() // allocates new rings hcd->driver->check_bandwidth() // issues configure endpoint command, free rings. usb_disable_interface(iface, true) usb_disable_endpoint() usb_hcd_flush_endpoint() // will access freed ring if URBs found!! usb_hcd_disable_endpoint() hcd->driver->endpoint_disable() // xhci does nothing usb_enable_interface(iface, true) usb_enable_endpoint(ep_addrss, true) // not really doing much on xhci side. As first aid I could try to implement checks that make sure the flushed URBs trb pointers really are on the current endpoint ring, and also add some warning if we are we are dropping endpoints with URBs still queued. But we need to fix this properly as well. xhci needs to be more in sync with usb core in usb_set_interface(), currently xhci has the altssetting up and running when usb core hasn't event started flushing endpoints. Absolutely. The core tries to be compatible with host controller drivers that either allocate bandwidth as it is requested or else allocate bandwidth all at once when an altsetting is installed. xhci-hcd falls into the second category. However, this approach requires the bandwidth verification for the new altsetting to be performed before the old altsetting has been disabled, and the xHCI hardware can't do this. We may need to change the core so that the old endpoints are disabled before the bandwidth check is done, instead of after. Of course, this leads to an awkward situation if the check fails -- we'd probably have to go back and re-install the old altsetting. That would help xhci a lot. If we want to avoid the awkward altsetting re-install after bandwidth failure then adding a extra endpoint flush before checking the bandwidth would already help a lot. The endpoint disabling can then be remain after bandwidth checking. Does that work for other host controllers? -Mathias ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: usb HC busted?
On 19.07.2018 20:32, Sudip Mukherjee wrote:
Hi Mathias,
On Thu, Jul 19, 2018 at 06:42:19PM +0300, Mathias Nyman wrote:
As first aid I could try to implement checks that make sure the flushed URBs
trb pointers really are on the current endpoint ring, and also add some warning
if we are we are dropping endpoints with URBs still queued.
Yes, please. I think your first-aid will be a much better option than
the hacky patch I am using atm.
Attached a patch that checks canceled URB td/trb pointers.
I haven't tested it at all (well compiles and boots, but new code never
exercised)
Does it work for you?
No, not exactly. :(
I can see your message getting printed.
[ 249.518394] xhci_hcd :00:14.0: Canceled URB td not found on endpoint ring
[ 249.518431] xhci_hcd :00:14.0: Canceled URB td not found on endpoint ring
But I can see the message from slub debug again:
[ 348.279986]
=
[ 348.279993] BUG kmalloc-96 (Tainted: G U O ): Poison overwritten
[ 348.279995]
-
[ 348.279997] Disabling lock debugging due to kernel taint
[ 348.28] INFO: 0xe5acda60-0xe5acda67. First byte 0x60 instead of 0x6b
[ 348.280012] INFO: Allocated in xhci_ring_alloc.constprop.14+0x31/0x125
[xhci_hcd] age=129264 cpu=0 pid=33
...
[ 348.280095] INFO: Freed in xhci_ring_free+0xa7/0xc6 [xhci_hcd] age=98722
cpu=0 pid=33
...
[ 348.280158] INFO: Slab 0xf46e0fe0 objects=29 used=29 fp=0x (null)
flags=0x40008100
[ 348.280160] INFO: Object 0xe5acda48 @offset=6728 fp=0xe5acd700
[ 348.280164] Redzone e5acda40: bb bb bb bb bb bb bb bb
[ 348.280167] Object e5acda48: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
[ 348.280169] Object e5acda58: 6b 6b 6b 6b 6b 6b 6b 6b 60 da ac e5 60 da ac e5
`...`...
So poison is overwritten at e5acda58 with almost its own address, (reading
backwards) e5 ac da 60, twice.
looks like something (32bit?)is pointing to itself twice, maybe a linked list
node next and prev pointer
being set to point to itself as last item was removed from list.
The cancelled_td_list is part of struct xhci_virt_ep, so that should be fine.
But td_list is part of struct xhci_ring, which was freed. and we removed the
URBs tds from the td_list when
flushing the ring after ring was freed
I changed the patch (attached) to make sure it doesn't touch the td_list when
canceling a URB after
ring is freed.
How about this one, any improvements?
-Mathias
>From ee48d9f9c2d82058489dcdc38faa34a3cbdb08d1 Mon Sep 17 00:00:00 2001
From: Mathias Nyman
Date: Thu, 19 Jul 2018 18:06:18 +0300
Subject: [PATCH v2] xhci: when dequeing a URB make sure it exists on the
current endpoint ring.
If the endpoint ring has been reallocated since the URB was enqueued,
then URB may contain TD and TRB pointers to a already freed ring.
If this the case then manuallt return the URB without touching any of the
freed ring structure data.
Don't try to stop the ring. It would be useless.
This can happened if endpoint is not flushed before it is dropped and
re-added, which is the case in usb_set_interface() as xhci does
things in an odd order.
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 711da33..7093341 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -37,6 +37,21 @@ static unsigned int quirks;
module_param(quirks, uint, S_IRUGO);
MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
+static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring)
+{
+ struct xhci_segment *seg = ring->first_seg;
+
+ if (!td || !td->start_seg)
+ return false;
+ do {
+ if (seg == td->start_seg)
+ return true;
+ seg = seg->next;
+ } while (seg && seg != ring->first_seg);
+
+ return false;
+}
+
/* TODO: copied from ehci-hcd.c - can this be refactored? */
/*
* xhci_handshake - spin reading hc until handshake completes or fails
@@ -1467,6 +1482,21 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
goto done;
}
+ /*
+ * check ring is not re-allocated since URB was enqueued. If it is, then
+ * make sure none of the ring related pointers in this URB private data
+ * are touched, such as td_list, otherwise we overwrite freed data
+ */
+ if (!td_on_ring(_priv->td[0], ep_ring)) {
+ xhci_err(xhci, "Canceled URB td not found on endpoint ring");
+ for (i = urb_priv->num_tds_done; i < urb_priv->num_tds; i++) {
+ td = _priv->td[i];
+ if (!list_empty(>cancelled_td_list))
+list_del_init(>cancelled_td_list);
+ }
+ goto err_giveback;
+ }
+
if (xhci->xhc_state & XHCI_STATE_HALTED) {
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"HC halted, freeing TD manually.");
--
Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled
On July 20, 2018 12:55:04 PM GMT+03:00, lijiang wrote:> >Thanks for your advice, I will rewrite the log and send them again. Do not send them again - explain the problem properly first! -- Sent from a small device: formatting sux and brevity is inevitable. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled
在 2018年07月20日 15:32, Borislav Petkov 写道: > On Fri, Jul 20, 2018 at 01:23:04PM +0800, Dave Young wrote: >>> Here, it doesn't need to dump MMIO space of the previous kernel, when the >>> kdump kernel boot, the MMIO address will be remapped in decryption manners, >>> but the MMIO address don't belong to the range of the crash reserved memory, >>> for the kdump kernel, the MMIO space(address) and IOMMU device >>> table(address) >>> are outside address, whereas, the IOMMU device table is encrypted in the >>> first >>> kernel, the kdump kernel will need to copy the content of IOMMU device table >>> from the first kernel when the kdump kernel boot, so the IOMMU device table >>> will >>> be remapped in encryption manners. > > -ENOFCKINGPARSE > > I believe you're the only one who understands that humongous sentence.Sorry > for this. > How about using a fullstop from time to time. And WTF is "encryption > manners"? > >>> So some of them require to be remapped in encryption manners, and >>> some(address) >>> require to be remapped in decryption manners. > >> There could be some misunderstanding here. > > Hell yeah there's a misunderstanding! > > Can you folks first relax, sit down and explain the whole problem in > *plain* English using *simple* sentences. *Not* like the unparseable > mess above. Use simple, declaratory sentences and don't even try to > sound fancy: > > "The first kernel boots. It's memory is encrypted... Now, the second > kernel boots. It must do A because of B. In order to do A, it needs to > do C. Because D..." > > And so on. Explain what the problem is first. Then explain the proposed > solution. Explain *why* it needs to be done this way. > > When you've written your explanation, try to read it as someone who > doesn't know kdump and *think* hard whether your explanation makes > sense. If it doesn't, fix it and read it again. Rinse and repeat. Until > it is clear to unenlightened readers too. > Thanks for your advice, I will rewrite the log and send them again. > It is about time this hurried throwing of half-baked patches at > maintainers and seeing what sticks, stops! > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: Remove redundant WARN_ON()
The WARN_ON() was introduced in commit 272e4f99e966 ("iommu/amd: WARN
when __[attach|detach]_device are called with irqs enabled") to ensure
that the domain->lock is taken in proper irqs disabled context. This
is required, because the domain->lock is taken as well in irq
context.
The proper context check by the WARN_ON() is redundant, because it is
already covered by LOCKDEP. When working with locks and changing
context, a run with LOCKDEP is required anyway and would detect the
wrong lock context.
Furthermore all callers for those functions are within the same file
and all callers acquire another lock which already disables interrupts.
Signed-off-by: Anna-Maria Gleixner
---
drivers/iommu/amd_iommu.c | 12
1 file changed, 12 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..b5f39bffd684 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1944,12 +1944,6 @@ static int __attach_device(struct iommu_dev_data
*dev_data,
{
int ret;
- /*
-* Must be called with IRQs disabled. Warn here to detect early
-* when its not.
-*/
- WARN_ON(!irqs_disabled());
-
/* lock domain */
spin_lock(>lock);
@@ -2115,12 +2109,6 @@ static void __detach_device(struct iommu_dev_data
*dev_data)
{
struct protection_domain *domain;
- /*
-* Must be called with IRQs disabled. Warn here to detect early
-* when its not.
-*/
- WARN_ON(!irqs_disabled());
-
domain = dev_data->domain;
spin_lock(>lock);
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] dts: arm64/sdm845: Add node for arm,mmu-500
Hi Rob,
On Fri, Jul 20, 2018 at 4:38 AM, Rob Herring wrote:
> On Thu, Jul 19, 2018 at 11:54 AM Vivek Gautam
> wrote:
>>
>> Add device node for arm,mmu-500 available on sdm845.
>> This MMU-500 with single TCU and multiple TBU architecture
>> is shared among all the peripherals except gpu on sdm845.
>>
>> Signed-off-by: Vivek Gautam
>> ---
>> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 4 ++
>> arch/arm64/boot/dts/qcom/sdm845.dtsi| 73
>> +
>> 2 files changed, 77 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> index 6d651f314193..13b50dff440f 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> @@ -58,3 +58,7 @@
>> bias-pull-up;
>> };
>> };
>> +
>> +_smmu {
>> + status = "okay";
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 00722b533a92..70ca18ae6cb3 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -980,6 +980,79 @@
>> cell-index = <0>;
>> };
>>
>> + apps_smmu: arm,smmu@1500 {
>
> iommu@...
Thanks for the review.
Sure, will modify this.
>
>> + compatible = "arm,mmu-500";
>
> Really unmodified by QCom? Would be better to have SoC specific compatible.
Yes, at this point we are able to use the driver as is on 845.
But, will add a SoC specific compatible as suggested to make bindings
future proof.
Best regards
Vivek
>
> Rob
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled
On Fri, Jul 20, 2018 at 01:23:04PM +0800, Dave Young wrote: > > Here, it doesn't need to dump MMIO space of the previous kernel, when the > > kdump kernel boot, the MMIO address will be remapped in decryption manners, > > but the MMIO address don't belong to the range of the crash reserved memory, > > for the kdump kernel, the MMIO space(address) and IOMMU device > > table(address) > > are outside address, whereas, the IOMMU device table is encrypted in the > > first > > kernel, the kdump kernel will need to copy the content of IOMMU device table > > from the first kernel when the kdump kernel boot, so the IOMMU device table > > will > > be remapped in encryption manners. -ENOFCKINGPARSE I believe you're the only one who understands that humongous sentence. How about using a fullstop from time to time. And WTF is "encryption manners"? > > So some of them require to be remapped in encryption manners, and > > some(address) > > require to be remapped in decryption manners. > There could be some misunderstanding here. Hell yeah there's a misunderstanding! Can you folks first relax, sit down and explain the whole problem in *plain* English using *simple* sentences. *Not* like the unparseable mess above. Use simple, declaratory sentences and don't even try to sound fancy: "The first kernel boots. It's memory is encrypted... Now, the second kernel boots. It must do A because of B. In order to do A, it needs to do C. Because D..." And so on. Explain what the problem is first. Then explain the proposed solution. Explain *why* it needs to be done this way. When you've written your explanation, try to read it as someone who doesn't know kdump and *think* hard whether your explanation makes sense. If it doesn't, fix it and read it again. Rinse and repeat. Until it is clear to unenlightened readers too. It is about time this hurried throwing of half-baked patches at maintainers and seeing what sticks, stops! -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
