Re: [PATCH] iommu/amd: Enable swiotlb if any device supports iommu v2 and uses identity mapping

2021-09-09 Thread Kai-Heng Feng
On Wed, Jul 14, 2021 at 6:25 PM Joerg Roedel  wrote:
>
> On Tue, Jul 13, 2021 at 07:57:40PM -0400, Konrad Rzeszutek Wilk wrote:
> > The SWIOTLB does have support to do late initialization (xen-pcifront
> > does that for example - so if you add devices that can't do 64-bit it
> > will allocate something like 4MB).
>
> That sounds like a way to evaluate. I suggest to allocate the SWIOTLB
> memory at boot and when the IOMMUs are initialized we re-evaluate what
> we ended up with and free the SWIOTLB memory if its not needed.
>
> If that turns out to be wrong during runtime (e.g. because a device is
> switched to a passthrough default domain at runtime), we allocate a
> small aperture for this device like the above mentioned 4MB.

I am currently working on this but I found that 4MB is not enough,
16MB is the minimal size to make the device work.
How do I know the right SWIOTLB size for each device?

>
> (A boot option to always keep the aperture around might also be helpful
>  for some setups)

OK, will also implement this in next iteration.

Kai-Heng

>
> Regards,
>
> Joerg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH] iommu/amd: Enable swiotlb if any device supports iommu v2 and uses identity mapping

2021-07-13 Thread Kai-Heng Feng
On Wed, Jul 14, 2021 at 7:57 AM Konrad Rzeszutek Wilk  wrote:
>
> On Thu, Jul 08, 2021 at 03:43:42PM +0100, Robin Murphy wrote:
> > On 2021-07-08 14:57, Kai-Heng Feng wrote:
> > > On Thu, Jul 8, 2021 at 6:18 PM Robin Murphy  wrote:
> > > >
> > > > On 2021-07-08 10:28, Joerg Roedel wrote:
> > > > > On Thu, Jul 08, 2021 at 03:42:32PM +0800, Kai-Heng Feng wrote:
> > > > > > @@ -344,6 +344,9 @@ static int iommu_init_device(struct device *dev)
> > > > > >
> > > > > >   iommu = amd_iommu_rlookup_table[dev_data->devid];
> > > > > >   dev_data->iommu_v2 = iommu->is_iommu_v2;
> > > > > > +
> > > > > > +if (dev_data->iommu_v2)
> > > > > > +swiotlb = 1;
> > > > >
> > > > > This looks like the big hammer, as it will affect all other systems
> > > > > where the AMD GPUs are in their own group.
> > > > >
> > > > > What is needed here is an explicit check whether a non-iommu-v2 device
> > > > > is direct-mapped because it shares a group with the GPU, and only 
> > > > > enable
> > > > > swiotlb in this case.
> > > >
> > > > Right, it's basically about whether any DMA-limited device might at any
> > > > time end up in an IOMMU_DOMAIN_IDENTITY domain. And given the
> > > > possibility of device hotplug and the user being silly with the sysfs
> > > > interface, I don't think we can categorically determine that at boot 
> > > > time.
> > > >
> > > > Also note that Intel systems are likely to be similarly affected (in
> > > > fact intel-iommu doesn't even have the iommu_default_passthough() check
> > > > so it's probably even easier to blow up).
> > >
> > > swiotlb is enabled by pci_swiotlb_detect_4gb() and intel-iommu doesn't
> > > disable it.
> >
> > Oh, right... I did say I found this dance hard to follow. Clearly I
> > shouldn't have trusted what I thought I remembered from looking at it
> > yesterday :)
> >
> > Also not helped by the fact that it sets iommu_detected which *does* disable
> > SWIOTLB, but only on IA-64.
> >
> > > I wonder if we can take the same approach in amd-iommu?
> >
> > Certainly if there's a precedent for leaving SWIOTLB enabled even if it
> > *might* be redundant, that seems like the easiest option (it's what we do on
> > arm64 too, but then we have system topologies where some devices may not be
> > behind IOMMUs even when others are). More fun would be to try to bring it up
> > at the first sign of IOMMU_DOMAIN_IDENTITY if it was disabled previously,
> > but I don't have the highest hope of that being practical.
>
> 
> It is kind of silly to enable SWIOTLB which will just eat 64MB of memory
> "just in case".
>
> The SWIOTLB does have support to do late initialization (xen-pcifront
> does that for example - so if you add devices that can't do 64-bit it
> will allocate something like 4MB).
>
> Would that be a better choice going forward - that is allocate this
> under those conditions?

But how to practically do swiotlb late init on 32-bit capable devices?
On the first DMA map requested by the driver?

Kai-Heng

> >
> > Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v2] iommu/amd: Keep swiotlb enabled to ensure devices with 32bit DMA still work

2021-07-08 Thread Kai-Heng Feng
We are seeing kernel panic on rtw88 probe routine because swiotlb isn't
set:
[  252.036773] rtw_8821ce :06:00.0: enabling device ( -> 0003)
[  252.037084] Kernel panic - not syncing: Can not allocate SWIOTLB buffer 
earlier and can't now provide you with the DMA bounce buffer
[  252.037146] CPU: 7 PID: 1174 Comm: modprobe Not tainted 5.13.0+ #39
[  252.037175] Hardware name: HP HP ProDesk 405 G6 Small Form Factor PC/8835, 
BIOS S05 Ver. 02.04.00 06/03/2021
[  252.037218] Call Trace:
[  252.037231]  dump_stack_lvl+0x4a/0x5f
[  252.037251]  dump_stack+0x10/0x12
[  252.037267]  panic+0x101/0x2e3
[  252.037284]  swiotlb_tbl_map_single.cold+0xc/0x73
[  252.037305]  ? __mod_lruvec_page_state+0x95/0xb0
[  252.037329]  ? kmalloc_large_node+0x8c/0xb0
[  252.037348]  ? __netdev_alloc_skb+0x44/0x160
[  252.037370]  swiotlb_map+0x61/0x240
[  252.037387]  ? __alloc_skb+0xed/0x1e0
[  252.037404]  dma_map_page_attrs+0x12c/0x1f0
[  252.037422]  ? __netdev_alloc_skb+0x44/0x160
[  252.037443]  rtw_pci_probe+0x30f/0x872 [rtw88_pci]
[  252.037467]  local_pci_probe+0x48/0x80
[  252.037487]  pci_device_probe+0x105/0x1c0
[  252.037506]  really_probe+0x1fe/0x3f0
[  252.037524]  __driver_probe_device+0x109/0x180
[  252.037545]  driver_probe_device+0x23/0x90
[  252.037564]  __driver_attach+0xac/0x1b0
[  252.037582]  ? __device_attach_driver+0xe0/0xe0
[  252.037602]  bus_for_each_dev+0x7e/0xc0
[  252.037620]  driver_attach+0x1e/0x20
[  252.037637]  bus_add_driver+0x135/0x1f0
[  252.037654]  driver_register+0x95/0xf0
[  252.037672]  ? 0xc0fa
[  252.037687]  __pci_register_driver+0x68/0x70
[  252.037707]  rtw_8821ce_driver_init+0x23/0x1000 [rtw88_8821ce]
[  252.037734]  do_one_initcall+0x48/0x1d0
[  252.037752]  ? __cond_resched+0x1a/0x50
[  252.037771]  ? kmem_cache_alloc_trace+0x29d/0x3c0
[  252.037792]  do_init_module+0x62/0x280
[  252.037810]  load_module+0x2577/0x27c0
[  252.037862]  __do_sys_finit_module+0xbf/0x120
[  252.037877]  __x64_sys_finit_module+0x1a/0x20
[  252.037893]  do_syscall_64+0x3b/0xc0
[  252.037907]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  252.037925] RIP: 0033:0x7ff5a2f9408d
[  252.037938] Code: 27 0d 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 
f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 
f0 ff ff 73 01 c3 48 8b 0d ab dd 0c 00 f7 d8 64 89 01 48
[  252.037993] RSP: 002b:7fffaa89dce8 EFLAGS: 0246 ORIG_RAX: 
0139
[  252.038017] RAX: ffda RBX: 55fd4f881080 RCX: 7ff5a2f9408d
[  252.038039] RDX:  RSI: 55fd4f63ec02 RDI: 0009
[  252.038063] RBP: 0004 R08:  R09: 55fd4f8885b0
[  252.038085] R10: 0009 R11: 0246 R12: 55fd4f63ec02
[  252.038107] R13: 55fd4f881120 R14:  R15: 55fd4f88e350
[  252.038293] Kernel Offset: 0x3060 from 0x8100 (relocation 
range: 0x8000-0xbfff)

Because the Realtek WiFi (PCI 06:00.0) is in the same IOMMU group as AMD
graphics (PCI 01:00.0),
[1.326166] pci :01:00.0: Adding to iommu group 0
...
[1.326268] pci :06:00.0: Adding to iommu group 0

And the AMD graphics supports iommu_v2, so the group uses intentity
mapping based on the query from amd_iommu_def_domain_type().

However, the Realtek WiFi only supports 32bit DMA, so we need to
make sure swiotlb is enabled.

The swiotlb is enabled by pci_swiotlb_detect_4gb() to support legacy
devices, but it gets disabled later by amd_iommu_init_dma_ops(). Keep
swiotlb enabled to resolve the issue.

Cc: Robin Murphy 
Signed-off-by: Kai-Heng Feng 
---
v2:
 - Keep swiotlb enabled if it's already set.
 - Some wording change.

 drivers/iommu/amd/iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 811a49a95d04..a893a6b6aeba 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1774,7 +1774,8 @@ void amd_iommu_domain_update(struct protection_domain 
*domain)
 
 static void __init amd_iommu_init_dma_ops(void)
 {
-   swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
+   if (!swiotlb)
+   swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
 
if (amd_iommu_unmap_flush)
pr_info("IO/TLB flush on unmap enabled\n");
-- 
2.31.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH] iommu/amd: Enable swiotlb if any device supports iommu v2 and uses identity mapping

2021-07-08 Thread Kai-Heng Feng
On Thu, Jul 8, 2021 at 6:18 PM Robin Murphy  wrote:
>
> On 2021-07-08 10:28, Joerg Roedel wrote:
> > On Thu, Jul 08, 2021 at 03:42:32PM +0800, Kai-Heng Feng wrote:
> >> @@ -344,6 +344,9 @@ static int iommu_init_device(struct device *dev)
> >>
> >>  iommu = amd_iommu_rlookup_table[dev_data->devid];
> >>  dev_data->iommu_v2 = iommu->is_iommu_v2;
> >> +
> >> +if (dev_data->iommu_v2)
> >> +swiotlb = 1;
> >
> > This looks like the big hammer, as it will affect all other systems
> > where the AMD GPUs are in their own group.
> >
> > What is needed here is an explicit check whether a non-iommu-v2 device
> > is direct-mapped because it shares a group with the GPU, and only enable
> > swiotlb in this case.
>
> Right, it's basically about whether any DMA-limited device might at any
> time end up in an IOMMU_DOMAIN_IDENTITY domain. And given the
> possibility of device hotplug and the user being silly with the sysfs
> interface, I don't think we can categorically determine that at boot time.
>
> Also note that Intel systems are likely to be similarly affected (in
> fact intel-iommu doesn't even have the iommu_default_passthough() check
> so it's probably even easier to blow up).

swiotlb is enabled by pci_swiotlb_detect_4gb() and intel-iommu doesn't
disable it.

I wonder if we can take the same approach in amd-iommu?

Kai-Heng

>
> Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH] iommu/amd: Enable swiotlb if any device supports iommu v2 and uses identity mapping

2021-07-08 Thread Kai-Heng Feng
We are seeing kernel panic on rtw88 probe routine because swiotlb isn't
set:
[  252.036773] rtw_8821ce :06:00.0: enabling device ( -> 0003)
[  252.037084] Kernel panic - not syncing: Can not allocate SWIOTLB buffer 
earlier and can't now provide you with the DMA bounce buffer
[  252.037146] CPU: 7 PID: 1174 Comm: modprobe Not tainted 5.13.0+ #39
[  252.037175] Hardware name: HP HP ProDesk 405 G6 Small Form Factor PC/8835, 
BIOS S05 Ver. 02.04.00 06/03/2021
[  252.037218] Call Trace:
[  252.037231]  dump_stack_lvl+0x4a/0x5f
[  252.037251]  dump_stack+0x10/0x12
[  252.037267]  panic+0x101/0x2e3
[  252.037284]  swiotlb_tbl_map_single.cold+0xc/0x73
[  252.037305]  ? __mod_lruvec_page_state+0x95/0xb0
[  252.037329]  ? kmalloc_large_node+0x8c/0xb0
[  252.037348]  ? __netdev_alloc_skb+0x44/0x160
[  252.037370]  swiotlb_map+0x61/0x240
[  252.037387]  ? __alloc_skb+0xed/0x1e0
[  252.037404]  dma_map_page_attrs+0x12c/0x1f0
[  252.037422]  ? __netdev_alloc_skb+0x44/0x160
[  252.037443]  rtw_pci_probe+0x30f/0x872 [rtw88_pci]
[  252.037467]  local_pci_probe+0x48/0x80
[  252.037487]  pci_device_probe+0x105/0x1c0
[  252.037506]  really_probe+0x1fe/0x3f0
[  252.037524]  __driver_probe_device+0x109/0x180
[  252.037545]  driver_probe_device+0x23/0x90
[  252.037564]  __driver_attach+0xac/0x1b0
[  252.037582]  ? __device_attach_driver+0xe0/0xe0
[  252.037602]  bus_for_each_dev+0x7e/0xc0
[  252.037620]  driver_attach+0x1e/0x20
[  252.037637]  bus_add_driver+0x135/0x1f0
[  252.037654]  driver_register+0x95/0xf0
[  252.037672]  ? 0xc0fa
[  252.037687]  __pci_register_driver+0x68/0x70
[  252.037707]  rtw_8821ce_driver_init+0x23/0x1000 [rtw88_8821ce]
[  252.037734]  do_one_initcall+0x48/0x1d0
[  252.037752]  ? __cond_resched+0x1a/0x50
[  252.037771]  ? kmem_cache_alloc_trace+0x29d/0x3c0
[  252.037792]  do_init_module+0x62/0x280
[  252.037810]  load_module+0x2577/0x27c0
[  252.037862]  __do_sys_finit_module+0xbf/0x120
[  252.037877]  __x64_sys_finit_module+0x1a/0x20
[  252.037893]  do_syscall_64+0x3b/0xc0
[  252.037907]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  252.037925] RIP: 0033:0x7ff5a2f9408d
[  252.037938] Code: 27 0d 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 
f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 
f0 ff ff 73 01 c3 48 8b 0d ab dd 0c 00 f7 d8 64 89 01 48
[  252.037993] RSP: 002b:7fffaa89dce8 EFLAGS: 0246 ORIG_RAX: 
0139
[  252.038017] RAX: ffda RBX: 55fd4f881080 RCX: 7ff5a2f9408d
[  252.038039] RDX:  RSI: 55fd4f63ec02 RDI: 0009
[  252.038063] RBP: 0004 R08:  R09: 55fd4f8885b0
[  252.038085] R10: 0009 R11: 0246 R12: 55fd4f63ec02
[  252.038107] R13: 55fd4f881120 R14:  R15: 55fd4f88e350
[  252.038293] Kernel Offset: 0x3060 from 0x8100 (relocation 
range: 0x8000-0xbfff)

Because the Realtek WiFi (PCI 06:00.0) is in the same IOMMU group as AMD
graphics (PCI 01:00.0),
[1.326166] pci :01:00.0: Adding to iommu group 0
...
[1.326268] pci :06:00.0: Adding to iommu group 0

And the AMD graphics supports iommu v2, so the group uses intentity
mapping based on the query from amd_iommu_def_domain_type().

However, the Realtek WiFi doesn't support 64bit DMA, so we need to
enable swiotlb, which was disabled by amd_iommu_init_dma_ops(), to make
remapping work.

Cc: Robin Murphy 
Signed-off-by: Kai-Heng Feng 
---
 drivers/iommu/amd/iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 811a49a95d04..7c5111ed5c97 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -344,6 +344,9 @@ static int iommu_init_device(struct device *dev)
 
iommu = amd_iommu_rlookup_table[dev_data->devid];
dev_data->iommu_v2 = iommu->is_iommu_v2;
+
+   if (dev_data->iommu_v2)
+   swiotlb = 1;
}
 
dev_iommu_priv_set(dev, dev_data);
-- 
2.31.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0

2021-07-07 Thread Kai-Heng Feng
On Wed, Jul 7, 2021 at 2:03 AM Robin Murphy  wrote:
>
> On 2021-07-06 17:21, Kai-Heng Feng wrote:
> > On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy  wrote:
> >>
> >> On 2021-07-06 07:51, Kai-Heng Feng wrote:
> >>> Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted
> >>> device into core") not only moved the check for untrusted device to
> >>> IOMMU core, it also introduced a behavioral change by returning
> >>> def_domain_type() directly without checking its return value. That makes
> >>> many devices no longer use the default IOMMU setting.
> >>>
> >>> So revert back to the old behavior which defaults to
> >>> iommu_def_domain_type when driver callback returns 0.
> >>>
> >>> Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted 
> >>> device into core")
> >>
> >> Are you sure about that? From that same commit:
> >>
> >> @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct
> >> iommu_group *group,
> >>   if (group->default_domain)
> >>   return 0;
> >>
> >> -   type = iommu_get_def_domain_type(dev);
> >> +   type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
> >>
> >>   return iommu_group_alloc_default_domain(dev->bus, group, type);
> >>}
> >>
> >> AFAICS the other two callers should also handle 0 correctly. Have you
> >> seen a problem in practice?
> >
> > Thanks for pointing out how the return value is being handled by the 
> > callers.
> > However, the same check is missing in probe_get_default_domain_type():
> > static int probe_get_default_domain_type(struct device *dev, void *data)
> > {
> >  struct __group_domain_type *gtype = data;
> >  unsigned int type = iommu_get_def_domain_type(dev);
> > ...
> > }
>
> I'm still not following - the next line right after that is "if (type)",
> which means it won't touch gtype, and if that happens for every
> iteration, probe_alloc_default_domain() subsequently hits its "if
> (!gtype.type)" condition and still ends up with iommu_def_domain_type.
> This *was* one of the other two callers I was talking about (the second
> being iommu_change_dev_def_domain()), and in fact on second look I think
> your proposed change will actually break this logic, since it's
> necessary to differentiate between a specific type being requested for
> the given device, and a "don't care" response which only implies to use
> the global default type if it's still standing after *all* the
> appropriate devices have been queried.

You are right, what I am seeing here is that the AMD GFX is using
IOMMU_DOMAIN_IDENTITY, and makes other devices in the same group,
including the Realtek WiFi, to use IOMMU_DOMAIN_IDENTITY as well.

>
> > I personally prefer the old way instead of open coding with ternary
> > operator, so I'll do that in v2.
> >
> > In practice, this causes a kernel panic when probing Realtek WiFi.
> > Because of the bug, dma_ops isn't set by probe_finalize(),
> > dma_map_single() falls back to swiotlb which isn't set and caused a
> > kernel panic.
>
> Hmm, but if that's the case, wouldn't it still be a problem anyway if
> the end result was IOMMU_DOMAIN_IDENTITY? I can't claim to fully
> understand the x86 swiotlb and no_iommu dance, but nothing really stands
> out to give me confidence that it handles the passthrough options correctly.

Yes, I think AMD IOMMU driver needs more thourough check on
static void __init amd_iommu_init_dma_ops(void)
{
swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
...
}

Since swiotlb gets disabled by the check, but the Realtek WiFi is only
capable of doing 32bit DMA, the kernel panics because
io_tlb_default_mem is NULL.

Thanks again for pointing to the right direction, this patch is indeed
incorrect.

Kai-Heng

>
> Robin.
>
> > I didn't attach the panic log because the system simply is frozen at
> > that point so the message is not logged to the storage.
> > I'll see if I can find another way to collect the log and attach it in v2.
> >
> > Kai-Heng
> >
> >>
> >> Robin.
> >>
> >>> Signed-off-by: Kai-Heng Feng 
> >>> ---
> >>>drivers/iommu/iommu.c | 5 +++--
> >>>1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >>> index 5419c4b9f27a..faac4f795025 10064

Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0

2021-07-06 Thread Kai-Heng Feng
On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy  wrote:
>
> On 2021-07-06 07:51, Kai-Heng Feng wrote:
> > Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted
> > device into core") not only moved the check for untrusted device to
> > IOMMU core, it also introduced a behavioral change by returning
> > def_domain_type() directly without checking its return value. That makes
> > many devices no longer use the default IOMMU setting.
> >
> > So revert back to the old behavior which defaults to
> > iommu_def_domain_type when driver callback returns 0.
> >
> > Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted 
> > device into core")
>
> Are you sure about that? From that same commit:
>
> @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct
> iommu_group *group,
>  if (group->default_domain)
>  return 0;
>
> -   type = iommu_get_def_domain_type(dev);
> +   type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
>
>  return iommu_group_alloc_default_domain(dev->bus, group, type);
>   }
>
> AFAICS the other two callers should also handle 0 correctly. Have you
> seen a problem in practice?

Thanks for pointing out how the return value is being handled by the callers.
However, the same check is missing in probe_get_default_domain_type():
static int probe_get_default_domain_type(struct device *dev, void *data)
{
struct __group_domain_type *gtype = data;
unsigned int type = iommu_get_def_domain_type(dev);
...
}

I personally prefer the old way instead of open coding with ternary
operator, so I'll do that in v2.

In practice, this causes a kernel panic when probing Realtek WiFi.
Because of the bug, dma_ops isn't set by probe_finalize(),
dma_map_single() falls back to swiotlb which isn't set and caused a
kernel panic.
I didn't attach the panic log because the system simply is frozen at
that point so the message is not logged to the storage.
I'll see if I can find another way to collect the log and attach it in v2.

Kai-Heng

>
> Robin.
>
> > Signed-off-by: Kai-Heng Feng 
> > ---
> >   drivers/iommu/iommu.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 5419c4b9f27a..faac4f795025 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
> >   static int iommu_get_def_domain_type(struct device *dev)
> >   {
> >   const struct iommu_ops *ops = dev->bus->iommu_ops;
> > + unsigned int type = 0;
> >
> >   if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
> >   return IOMMU_DOMAIN_DMA;
> >
> >   if (ops->def_domain_type)
> > - return ops->def_domain_type(dev);
> > + type = ops->def_domain_type(dev);
> >
> > - return 0;
> > + return (type == 0) ? iommu_def_domain_type : type;
> >   }
> >
> >   static int iommu_group_alloc_default_domain(struct bus_type *bus,
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0

2021-07-06 Thread Kai-Heng Feng
Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted
device into core") not only moved the check for untrusted device to
IOMMU core, it also introduced a behavioral change by returning
def_domain_type() directly without checking its return value. That makes
many devices no longer use the default IOMMU setting.

So revert back to the old behavior which defaults to
iommu_def_domain_type when driver callback returns 0.

Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device 
into core")
Signed-off-by: Kai-Heng Feng 
---
 drivers/iommu/iommu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5419c4b9f27a..faac4f795025 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 static int iommu_get_def_domain_type(struct device *dev)
 {
const struct iommu_ops *ops = dev->bus->iommu_ops;
+   unsigned int type = 0;
 
if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
return IOMMU_DOMAIN_DMA;
 
if (ops->def_domain_type)
-   return ops->def_domain_type(dev);
+   type = ops->def_domain_type(dev);
 
-   return 0;
+   return (type == 0) ? iommu_def_domain_type : type;
 }
 
 static int iommu_group_alloc_default_domain(struct bus_type *bus,
-- 
2.31.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [bugzilla-dae...@bugzilla.kernel.org: [Bug 209149] New: "iommu/vt-d: Enable PCI ACS for platform opt in hint" makes NVMe config space not accessible after S3]

2020-09-25 Thread Kai-Heng Feng
Raj,

> On Sep 25, 2020, at 03:44, Raj, Ashok  wrote:
> 
> Hi Alex
> 
>>> Apparently it also requires to disable RR, and I'm not able to confirm if
>>> CML requires that as well. 
>>> 
>>> pci_quirk_disable_intel_spt_pch_acs_redir() also seems to consult the same
>>> table, so i'm not sure why we need the other patch in bugzilla is required.
>> 
>> If we're talking about the Intel bug where PCH root ports implement
>> the ACS capability and control registers as dword rather than word
>> registers, then how is ACS getting enabled in order to generate an ACS
>> violation?  The standard ACS code would write to the control register
>> word at offset 6, which is still the read-only capability register on
>> those devices.  Thanks,
> 
> 
> Right... Maybe we need header log to figure out what exatly is happening. 
> 

Please let me know what logs you need.

As Bjorn mentioned earlier, there's currently no way to dump TLP header log?

Kai-Heng
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [bugzilla-dae...@bugzilla.kernel.org: [Bug 209149] New: "iommu/vt-d: Enable PCI ACS for platform opt in hint" makes NVMe config space not accessible after S3]

2020-09-23 Thread Kai-Heng Feng
[+Cc Christoph]

> On Sep 24, 2020, at 00:03, Bjorn Helgaas  wrote:
> 
> [+cc IOMMU and NVMe folks]
> 
> Sorry, I forgot to forward this to linux-pci when it was first
> reported.
> 
> Apparently this happens with v5.9-rc3, and may be related to
> 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in hint"),
> which appeared in v5.8-rc3.
> 
> There are several dmesg logs and proposed patches in the bugzilla, but
> no analysis yet of what the problem is.  From the first dmesg
> attachment (https://bugzilla.kernel.org/attachment.cgi?id=292327):

AFAIK Intel is working on it internally.
Comet Lake probably needs ACS quirk like older generation chips.

> 
>  [   50.434945] PM: suspend entry (deep)
>  [   50.802086] nvme :01:00.0: saving config space at offset 0x0 (reading 
> 0x11e0f)
>  [   50.842775] ACPI: Preparing to enter system sleep state S3
>  [   50.858922] ACPI: Waking up from system sleep state S3
>  [   50.883622] nvme :01:00.0: can't change power state from D3hot to D0 
> (config space inaccessible)
>  [   50.947352] nvme :01:00.0: restoring config space at offset 0x0 (was 
> 0x, writing 0x11e0f)
>  [   50.947816] pcieport :00:1b.0: DPC: containment event, status:0x1f01 
> source:0x
>  [   50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error 
> detected
>  [   50.947829] pcieport :00:1b.0: PCIe Bus Error: severity=Uncorrected 
> (Non-Fatal), type=Transaction Layer, (Receiver ID)
>  [   50.947830] pcieport :00:1b.0:   device [8086:06ac] error 
> status/mask=0020/0001
>  [   50.947831] pcieport :00:1b.0:[21] ACSViol(First)
>  [   50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
>  [   50.947843] nvme nvme0: frozen state error detected, reset controller
> 
> I suspect the nvme "can't change power state" and restore config space
> errors are a consequence of the DPC event.  If DPC disables the link,
> the device is inaccessible.
> 
> I don't know what caused the ACS Violation.  The AER TLP Header Log
> might have a clue, but unfortunately we didn't print it.
> 
> Tangent:
> 
>  The fact that we didn't print the AER TLP Header log looks like
>  a bug in itself.  PCIe r5.0, sec 6.2.7, table 6-5, says many
>  errors, including ACS Violation, should log the TLP header.  But
>  aer_get_device_error_info() only reads the log for error bits in
>  AER_LOG_TLP_MASKS, which doesn't include PCI_ERR_UNC_ACSV.
> 
>  I don't think there's a "TLP Header Log Valid" bit, and it's ugly to
>  have to update AER_LOG_TLP_MASKS if new errors are added.  I think
>  maybe we should always print the header log.

I can attach TLP Header if there's a patch...

Kai-Heng

> 
> - Forwarded message from bugzilla-dae...@bugzilla.kernel.org -
> 
> Date: Fri, 04 Sep 2020 14:31:20 +
> From: bugzilla-dae...@bugzilla.kernel.org
> To: bj...@helgaas.com
> Subject: [Bug 209149] New: "iommu/vt-d: Enable PCI ACS for platform opt in
>   hint" makes NVMe config space not accessible after S3
> Message-ID: 
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=209149
> 
>Bug ID: 209149
>   Summary: "iommu/vt-d: Enable PCI ACS for platform opt in hint"
>makes NVMe config space not accessible after S3
>   Product: Drivers
>   Version: 2.5
>Kernel Version: mainline
>  Hardware: All
>OS: Linux
>  Tree: Mainline
>Status: NEW
>  Severity: normal
>  Priority: P1
> Component: PCI
>  Assignee: drivers_...@kernel-bugs.osdl.org
>  Reporter: kai.heng.f...@canonical.com
>Regression: No
> 
> Here's the error:
> [   50.947816] pcieport :00:1b.0: DPC: containment event, status:0x1f01
> source:0x
> [   50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error
> detected
> [   50.947829] pcieport :00:1b.0: PCIe Bus Error: severity=Uncorrected
> (Non-Fatal), type=Transaction Layer, (Receiver ID)
> [   50.947830] pcieport :00:1b.0:   device [8086:06ac] error
> status/mask=0020/0001
> [   50.947831] pcieport :00:1b.0:[21] ACSViol(First)
> [   50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
> [   50.947843] nvme nvme0: frozen state error detected, reset controller
> 
> -- 
> You are receiving this mail because:
> You are watching the assignee of the bug.
> 
> - End forwarded message -

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [Regression] "iommu/amd: Relax locking in dma_ops path" makes tg3 ethernet transmit queue timeout

2020-08-21 Thread Kai-Heng Feng
Hi Joerg,

> On Aug 21, 2020, at 21:43, Joerg Roedel  wrote:
> 
> Hi Kai,
> 
> On Mon, Jun 29, 2020 at 08:33:22PM +0800, Kai-Heng Feng wrote:
>> I am still seeing the issue on v5.8-rc3. The issue goes away as soon
>> as "iommu=off" is added.
> 
> Can you probably help with debugging this issue? I've had no luck so far
> getting my hands on a machine with tg3 hardware and an AMD IOMMU.

Of course, I still have the system at my side.

The offending commit is 92d420ec028d ("iommu/amd: Relax locking in dma_ops 
path"), however be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the 
dma-iommu api") removed .map_page entirely so I don't know where to start.

Kai-Heng

> 
> Regards,
> 
>   Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[BUG] "Pre-boot DMA Protection" makes AMDGPU stop working

2020-07-02 Thread Kai-Heng Feng
Hi,

A more detailed bug report can be found at [1].

I have a AMD Renoir system that can't enter graphical session because there are 
many IOMMU splat.

Alex suggested to disable "Pre-boot DMA Protection", I can confirm once it's 
disabled, AMDGPU starts working with IOMMU enabled.
So raise the issue here because I have no knowledge on how to reset the IOMMU.

[1] https://gitlab.freedesktop.org/drm/amd/-/issues/1204

Kai-Heng
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [Regression] "iommu/amd: Relax locking in dma_ops path" makes tg3 ethernet transmit queue timeout

2020-06-29 Thread Kai-Heng Feng


> On May 18, 2020, at 23:32, Kai-Heng Feng  wrote:
> 
> 
> 
>> On May 18, 2020, at 22:05, Kai-Heng Feng  wrote:
>> 
>> 
>> 
>>> On May 18, 2020, at 21:32, Joerg Roedel  wrote:
>>> 
>>> On Mon, May 18, 2020 at 05:06:45PM +0800, Kai-Heng Feng wrote:
>>>> Particularly, as soon as the spinlock is removed, the issue can be 
>>>> reproduced.
>>>> Function domain_flush_complete() doesn't seem to affect the status.
>>>> 
>>>> However, the .map_page callback was removed by be62dbf554c5
>>>> ("iommu/amd: Convert AMD iommu driver to the dma-iommu api"), so
>>>> there's no easy revert for this issue.
>>>> 
>>>> This is still reproducible as of today's mainline kernel, v5.7-rc6.
>>> 
>>> Is there any error message from the IOMMU driver?
>>> 
>> 
>> As of mainline kernel, there's no error message from IOMMU driver.
>> There are some complains from v4.15-rc1:
>> https://pastebin.ubuntu.com/p/qn4TXkFxsc/
> 
> Just tested v5.7-rc6, the issue disappears as soon as kernel boots with 
> "iommu=off".

I am still seeing the issue on v5.8-rc3. The issue goes away as soon as 
"iommu=off" is added.

Kai-Heng

> 
> Kai-Heng
> 
>> 
>> Kai-Heng
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [Regression] "iommu/amd: Relax locking in dma_ops path" makes tg3 ethernet transmit queue timeout

2020-05-18 Thread Kai-Heng Feng



> On May 18, 2020, at 22:05, Kai-Heng Feng  wrote:
> 
> 
> 
>> On May 18, 2020, at 21:32, Joerg Roedel  wrote:
>> 
>> On Mon, May 18, 2020 at 05:06:45PM +0800, Kai-Heng Feng wrote:
>>> Particularly, as soon as the spinlock is removed, the issue can be 
>>> reproduced.
>>> Function domain_flush_complete() doesn't seem to affect the status.
>>> 
>>> However, the .map_page callback was removed by be62dbf554c5
>>> ("iommu/amd: Convert AMD iommu driver to the dma-iommu api"), so
>>> there's no easy revert for this issue.
>>> 
>>> This is still reproducible as of today's mainline kernel, v5.7-rc6.
>> 
>> Is there any error message from the IOMMU driver?
>> 
> 
> As of mainline kernel, there's no error message from IOMMU driver.
> There are some complains from v4.15-rc1:
> https://pastebin.ubuntu.com/p/qn4TXkFxsc/

Just tested v5.7-rc6, the issue disappears as soon as kernel boots with 
"iommu=off".

Kai-Heng

> 
> Kai-Heng

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [Regression] "iommu/amd: Relax locking in dma_ops path" makes tg3 ethernet transmit queue timeout

2020-05-18 Thread Kai-Heng Feng



> On May 18, 2020, at 21:32, Joerg Roedel  wrote:
> 
> On Mon, May 18, 2020 at 05:06:45PM +0800, Kai-Heng Feng wrote:
>> Particularly, as soon as the spinlock is removed, the issue can be 
>> reproduced.
>> Function domain_flush_complete() doesn't seem to affect the status.
>> 
>> However, the .map_page callback was removed by be62dbf554c5
>> ("iommu/amd: Convert AMD iommu driver to the dma-iommu api"), so
>> there's no easy revert for this issue.
>> 
>> This is still reproducible as of today's mainline kernel, v5.7-rc6.
> 
> Is there any error message from the IOMMU driver?
> 

As of mainline kernel, there's no error message from IOMMU driver.
There are some complains from v4.15-rc1:
https://pastebin.ubuntu.com/p/qn4TXkFxsc/

Kai-Heng
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[Regression] "iommu/amd: Relax locking in dma_ops path" makes tg3 ethernet transmit queue timeout

2020-05-18 Thread Kai-Heng Feng
Hi,

Broadcom ethernet tg3 unusable after commit 92d420ec028d ("iommu/amd: Relax 
locking in dma_ops path").
After a short period it stops:
[  122.717144] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:303 
dev_watchdog+0x237/0x240()
[  122.717152] NETDEV WATCHDOG: enp3s0 (tg3): transmit queue 0 timed out

After testing the patch section by section, this is the part that caused the 
regression:

@@ -2578,19 +2580,8 @@ static dma_addr_t map_page(struct device *dev, struct 
page *page,
 
dma_mask = *dev->dma_mask;
 
-   spin_lock_irqsave(>lock, flags);
-
-   addr = __map_single(dev, domain->priv, paddr, size, dir, false,
+   return __map_single(dev, domain->priv, paddr, size, dir, false,
dma_mask);
-   if (addr == DMA_ERROR_CODE)
-   goto out;
-
-   domain_flush_complete(domain);
-
-out:
-   spin_unlock_irqrestore(>lock, flags);
-
-   return addr;
 }

Particularly, as soon as the spinlock is removed, the issue can be reproduced.
Function domain_flush_complete() doesn't seem to affect the status.

However, the .map_page callback was removed by be62dbf554c5 ("iommu/amd: 
Convert AMD iommu driver to the dma-iommu api"), so there's no easy revert for 
this issue.

This is still reproducible as of today's mainline kernel, v5.7-rc6.

Kai-Heng
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2020-02-05 Thread Kai-Heng Feng
Hi Joerg,

> On Jan 6, 2020, at 16:37, Kai-Heng Feng  wrote:
> 
> 
> 
>> On Dec 20, 2019, at 10:13, Kai-Heng Feng  wrote:
>> 
>> 
>> 
>>> On Dec 20, 2019, at 03:15, Deucher, Alexander  
>>> wrote:
>>> 
>>>> -Original Message-
>>>> From: Kai-Heng Feng 
>>>> Sent: Wednesday, December 18, 2019 12:45 PM
>>>> To: Joerg Roedel 
>>>> Cc: Christoph Hellwig ; Deucher, Alexander
>>>> ; iommu@lists.linux-foundation.org; Kernel
>>>> development list 
>>>> Subject: Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge
>>>> systems
>>>> 
>>>> 
>>>> 
>>>>> On Dec 17, 2019, at 17:53, Joerg Roedel  wrote:
>>>>> 
>>>>> On Fri, Dec 06, 2019 at 01:57:41PM +0800, Kai-Heng Feng wrote:
>>>>>> Hi Joerg,
>>>>>> 
>>>>>>> On Dec 3, 2019, at 01:00, Christoph Hellwig  wrote:
>>>>>>> 
>>>>>>> On Fri, Nov 29, 2019 at 10:21:54PM +0800, Kai-Heng Feng wrote:
>>>>>>>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>>>>>>>> 
>>>>>>>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's
>>>>>>>> do the same here to avoid screen flickering on 4K monitor.
>>>>>>> 
>>>>>>> Disabling the IOMMU entirely seem pretty severe.  Isn't it enough to
>>>>>>> identity map the GPU device?
>>>>>> 
>>>>>> Ok, there's set_device_exclusion_range() to exclude the device from
>>>> IOMMU.
>>>>>> However I don't know how to generate range_start and range_length,
>>>> which are read from ACPI.
>>>>> 
>>>>> set_device_exclusion_range() is not the solution here. The best is if
>>>>> the GPU device is put into a passthrough domain at boot, in which it
>>>>> will be identity mapped. DMA still goes through the IOMMU in this
>>>>> case, but it only needs to lookup the device-table, page-table walks
>>>>> will not be done anymore.
>>>>> 
>>>>> The best way to implement this is to put it into the
>>>>> amd_iommu_add_device() in drivers/iommu/amd_iommu.c. There is this
>>>>> check:
>>>>> 
>>>>> if (dev_data->iommu_v2)
>>>>>   iommu_request_dm_for_dev(dev);
>>>>> 
>>>>> The iommu_request_dm_for_dev() function causes the device to be
>>>>> identity mapped. The check can be extended to also check for a device
>>>>> white-list for devices that need identity mapping.
>>>> 
>>>> My patch looks like this but the original behavior (4K screen flickering) 
>>>> is still
>>>> the same:
>>> 
>>> Does reverting the patch to disable ATS along with this patch help?
>> 
>> Unfortunately it doesn't help.
> 
> Any further suggestion to let me try?

Since using identity mapping with ATS doesn't help,
Is it possible to merge this patch as is?

Kai-Heng

> 
> Kai-Heng
> 
>> 
>> Kai-Heng
>> 
>>> 
>>> Alex
>>> 
>>>> 
>>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>>> index bd25674ee4db..f913a25c9e92 100644
>>>> --- a/drivers/iommu/amd_iommu.c
>>>> +++ b/drivers/iommu/amd_iommu.c
>>>> @@ -42,6 +42,7 @@
>>>> #include 
>>>> #include 
>>>> #include 
>>>> +#include 
>>>> 
>>>> #include "amd_iommu_proto.h"
>>>> #include "amd_iommu_types.h"
>>>> @@ -2159,6 +2160,8 @@ static int amd_iommu_add_device(struct device
>>>> *dev)
>>>>  struct iommu_domain *domain;
>>>>  struct amd_iommu *iommu;
>>>>  int ret, devid;
>>>> +   bool need_identity_mapping = false;
>>>> +   u32 header;
>>>> 
>>>>  if (!check_device(dev) || get_dev_data(dev))
>>>>  return 0;
>>>> @@ -2184,7 +2187,11 @@ static int amd_iommu_add_device(struct device
>>>> *dev)
>>>> 
>>>>  BUG_ON(!dev_data);
>>>> 
>>>> -   if (dev_data->iommu_v2)
>>>> +   header = read_pci_config(0, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>>>> PCI_FUNC(devid));
>>>> +   if ((header & 0x) == 0x1002 && (header >> 16) == 0x98e4)
>>>> +   need_identity_mapping = true;
>>>> +
>>>> +   if (dev_data->iommu_v2 || need_identity_mapping)
>>>>  iommu_request_dm_for_dev(dev);
>>>> 
>>>>  /* Domains are initialized for this device - have a look what we 
>>>> ended up
>>>> with */
>>>> 
>>>> 
>>>> $ dmesg | grep -i direct
>>>> [0.011446] Using GB pages for direct mapping
>>>> [0.703369] pci :00:01.0: Using iommu direct mapping
>>>> [0.703830] pci :00:08.0: Using iommu direct mapping
>>>> 
>>>> So the graphics device (pci :00:01.0:) is using direct mapping after 
>>>> the
>>>> change.
>>>> 
>>>> Kai-Heng
>>>> 
>>>>> 
>>>>> HTH,
>>>>> 
>>>>>   Joerg
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2020-01-06 Thread Kai-Heng Feng



> On Dec 20, 2019, at 10:13, Kai-Heng Feng  wrote:
> 
> 
> 
>> On Dec 20, 2019, at 03:15, Deucher, Alexander  
>> wrote:
>> 
>>> -----Original Message-
>>> From: Kai-Heng Feng 
>>> Sent: Wednesday, December 18, 2019 12:45 PM
>>> To: Joerg Roedel 
>>> Cc: Christoph Hellwig ; Deucher, Alexander
>>> ; iommu@lists.linux-foundation.org; Kernel
>>> development list 
>>> Subject: Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge
>>> systems
>>> 
>>> 
>>> 
>>>> On Dec 17, 2019, at 17:53, Joerg Roedel  wrote:
>>>> 
>>>> On Fri, Dec 06, 2019 at 01:57:41PM +0800, Kai-Heng Feng wrote:
>>>>> Hi Joerg,
>>>>> 
>>>>>> On Dec 3, 2019, at 01:00, Christoph Hellwig  wrote:
>>>>>> 
>>>>>> On Fri, Nov 29, 2019 at 10:21:54PM +0800, Kai-Heng Feng wrote:
>>>>>>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>>>>>>> 
>>>>>>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's
>>>>>>> do the same here to avoid screen flickering on 4K monitor.
>>>>>> 
>>>>>> Disabling the IOMMU entirely seem pretty severe.  Isn't it enough to
>>>>>> identity map the GPU device?
>>>>> 
>>>>> Ok, there's set_device_exclusion_range() to exclude the device from
>>> IOMMU.
>>>>> However I don't know how to generate range_start and range_length,
>>> which are read from ACPI.
>>>> 
>>>> set_device_exclusion_range() is not the solution here. The best is if
>>>> the GPU device is put into a passthrough domain at boot, in which it
>>>> will be identity mapped. DMA still goes through the IOMMU in this
>>>> case, but it only needs to lookup the device-table, page-table walks
>>>> will not be done anymore.
>>>> 
>>>> The best way to implement this is to put it into the
>>>> amd_iommu_add_device() in drivers/iommu/amd_iommu.c. There is this
>>>> check:
>>>> 
>>>>  if (dev_data->iommu_v2)
>>>>iommu_request_dm_for_dev(dev);
>>>> 
>>>> The iommu_request_dm_for_dev() function causes the device to be
>>>> identity mapped. The check can be extended to also check for a device
>>>> white-list for devices that need identity mapping.
>>> 
>>> My patch looks like this but the original behavior (4K screen flickering) 
>>> is still
>>> the same:
>> 
>> Does reverting the patch to disable ATS along with this patch help?
> 
> Unfortunately it doesn't help.

Any further suggestion to let me try?

Kai-Heng

> 
> Kai-Heng
> 
>> 
>> Alex
>> 
>>> 
>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>> index bd25674ee4db..f913a25c9e92 100644
>>> --- a/drivers/iommu/amd_iommu.c
>>> +++ b/drivers/iommu/amd_iommu.c
>>> @@ -42,6 +42,7 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>> 
>>> #include "amd_iommu_proto.h"
>>> #include "amd_iommu_types.h"
>>> @@ -2159,6 +2160,8 @@ static int amd_iommu_add_device(struct device
>>> *dev)
>>>   struct iommu_domain *domain;
>>>   struct amd_iommu *iommu;
>>>   int ret, devid;
>>> +   bool need_identity_mapping = false;
>>> +   u32 header;
>>> 
>>>   if (!check_device(dev) || get_dev_data(dev))
>>>   return 0;
>>> @@ -2184,7 +2187,11 @@ static int amd_iommu_add_device(struct device
>>> *dev)
>>> 
>>>   BUG_ON(!dev_data);
>>> 
>>> -   if (dev_data->iommu_v2)
>>> +   header = read_pci_config(0, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>>> PCI_FUNC(devid));
>>> +   if ((header & 0x) == 0x1002 && (header >> 16) == 0x98e4)
>>> +   need_identity_mapping = true;
>>> +
>>> +   if (dev_data->iommu_v2 || need_identity_mapping)
>>>   iommu_request_dm_for_dev(dev);
>>> 
>>>   /* Domains are initialized for this device - have a look what we 
>>> ended up
>>> with */
>>> 
>>> 
>>> $ dmesg | grep -i direct
>>> [0.011446] Using GB pages for direct mapping
>>> [0.703369] pci :00:01.0: Using iommu direct mapping
>>> [0.703830] pci :00:08.0: Using iommu direct mapping
>>> 
>>> So the graphics device (pci :00:01.0:) is using direct mapping after the
>>> change.
>>> 
>>> Kai-Heng
>>> 
>>>> 
>>>> HTH,
>>>> 
>>>>Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2019-12-19 Thread Kai-Heng Feng



> On Dec 20, 2019, at 03:15, Deucher, Alexander  
> wrote:
> 
>> -Original Message-----
>> From: Kai-Heng Feng 
>> Sent: Wednesday, December 18, 2019 12:45 PM
>> To: Joerg Roedel 
>> Cc: Christoph Hellwig ; Deucher, Alexander
>> ; iommu@lists.linux-foundation.org; Kernel
>> development list 
>> Subject: Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge
>> systems
>> 
>> 
>> 
>>> On Dec 17, 2019, at 17:53, Joerg Roedel  wrote:
>>> 
>>> On Fri, Dec 06, 2019 at 01:57:41PM +0800, Kai-Heng Feng wrote:
>>>> Hi Joerg,
>>>> 
>>>>> On Dec 3, 2019, at 01:00, Christoph Hellwig  wrote:
>>>>> 
>>>>> On Fri, Nov 29, 2019 at 10:21:54PM +0800, Kai-Heng Feng wrote:
>>>>>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>>>>>> 
>>>>>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's
>>>>>> do the same here to avoid screen flickering on 4K monitor.
>>>>> 
>>>>> Disabling the IOMMU entirely seem pretty severe.  Isn't it enough to
>>>>> identity map the GPU device?
>>>> 
>>>> Ok, there's set_device_exclusion_range() to exclude the device from
>> IOMMU.
>>>> However I don't know how to generate range_start and range_length,
>> which are read from ACPI.
>>> 
>>> set_device_exclusion_range() is not the solution here. The best is if
>>> the GPU device is put into a passthrough domain at boot, in which it
>>> will be identity mapped. DMA still goes through the IOMMU in this
>>> case, but it only needs to lookup the device-table, page-table walks
>>> will not be done anymore.
>>> 
>>> The best way to implement this is to put it into the
>>> amd_iommu_add_device() in drivers/iommu/amd_iommu.c. There is this
>>> check:
>>> 
>>>   if (dev_data->iommu_v2)
>>> iommu_request_dm_for_dev(dev);
>>> 
>>> The iommu_request_dm_for_dev() function causes the device to be
>>> identity mapped. The check can be extended to also check for a device
>>> white-list for devices that need identity mapping.
>> 
>> My patch looks like this but the original behavior (4K screen flickering) is 
>> still
>> the same:
> 
> Does reverting the patch to disable ATS along with this patch help?

Unfortunately it doesn't help.

Kai-Heng

> 
> Alex
> 
>> 
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index bd25674ee4db..f913a25c9e92 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -42,6 +42,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> 
>> #include "amd_iommu_proto.h"
>> #include "amd_iommu_types.h"
>> @@ -2159,6 +2160,8 @@ static int amd_iommu_add_device(struct device
>> *dev)
>>struct iommu_domain *domain;
>>struct amd_iommu *iommu;
>>int ret, devid;
>> +   bool need_identity_mapping = false;
>> +   u32 header;
>> 
>>if (!check_device(dev) || get_dev_data(dev))
>>return 0;
>> @@ -2184,7 +2187,11 @@ static int amd_iommu_add_device(struct device
>> *dev)
>> 
>>BUG_ON(!dev_data);
>> 
>> -   if (dev_data->iommu_v2)
>> +   header = read_pci_config(0, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>> PCI_FUNC(devid));
>> +   if ((header & 0x) == 0x1002 && (header >> 16) == 0x98e4)
>> +   need_identity_mapping = true;
>> +
>> +   if (dev_data->iommu_v2 || need_identity_mapping)
>>iommu_request_dm_for_dev(dev);
>> 
>>/* Domains are initialized for this device - have a look what we 
>> ended up
>> with */
>> 
>> 
>> $ dmesg | grep -i direct
>> [0.011446] Using GB pages for direct mapping
>> [0.703369] pci :00:01.0: Using iommu direct mapping
>> [0.703830] pci :00:08.0: Using iommu direct mapping
>> 
>> So the graphics device (pci :00:01.0:) is using direct mapping after the
>> change.
>> 
>> Kai-Heng
>> 
>>> 
>>> HTH,
>>> 
>>> Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2019-12-18 Thread Kai-Heng Feng



> On Dec 17, 2019, at 17:53, Joerg Roedel  wrote:
> 
> On Fri, Dec 06, 2019 at 01:57:41PM +0800, Kai-Heng Feng wrote:
>> Hi Joerg,
>> 
>>> On Dec 3, 2019, at 01:00, Christoph Hellwig  wrote:
>>> 
>>> On Fri, Nov 29, 2019 at 10:21:54PM +0800, Kai-Heng Feng wrote:
>>>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>>>> 
>>>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's do
>>>> the same here to avoid screen flickering on 4K monitor.
>>> 
>>> Disabling the IOMMU entirely seem pretty severe.  Isn't it enough to
>>> identity map the GPU device?
>> 
>> Ok, there's set_device_exclusion_range() to exclude the device from IOMMU.
>> However I don't know how to generate range_start and range_length, which are 
>> read from ACPI.
> 
> set_device_exclusion_range() is not the solution here. The best is if
> the GPU device is put into a passthrough domain at boot, in which it
> will be identity mapped. DMA still goes through the IOMMU in this case,
> but it only needs to lookup the device-table, page-table walks will not
> be done anymore.
> 
> The best way to implement this is to put it into the
> amd_iommu_add_device() in drivers/iommu/amd_iommu.c. There is this
> check:
> 
>if (dev_data->iommu_v2)
>   iommu_request_dm_for_dev(dev);
> 
> The iommu_request_dm_for_dev() function causes the device to be identity
> mapped. The check can be extended to also check for a device white-list
> for devices that need identity mapping.

My patch looks like this but the original behavior (4K screen flickering) is 
still the same:

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index bd25674ee4db..f913a25c9e92 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "amd_iommu_proto.h"
 #include "amd_iommu_types.h"
@@ -2159,6 +2160,8 @@ static int amd_iommu_add_device(struct device *dev)
struct iommu_domain *domain;
struct amd_iommu *iommu;
int ret, devid;
+   bool need_identity_mapping = false;
+   u32 header;
 
if (!check_device(dev) || get_dev_data(dev))
return 0;
@@ -2184,7 +2187,11 @@ static int amd_iommu_add_device(struct device *dev)
 
BUG_ON(!dev_data);
 
-   if (dev_data->iommu_v2)
+   header = read_pci_config(0, PCI_BUS_NUM(devid), PCI_SLOT(devid), 
PCI_FUNC(devid));
+   if ((header & 0x) == 0x1002 && (header >> 16) == 0x98e4)
+   need_identity_mapping = true;
+
+   if (dev_data->iommu_v2 || need_identity_mapping)
iommu_request_dm_for_dev(dev);
 
/* Domains are initialized for this device - have a look what we ended 
up with */


$ dmesg | grep -i direct
[0.011446] Using GB pages for direct mapping
[0.703369] pci :00:01.0: Using iommu direct mapping
[0.703830] pci :00:08.0: Using iommu direct mapping

So the graphics device (pci :00:01.0:) is using direct mapping after the 
change.

Kai-Heng

> 
> HTH,
> 
>   Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2019-12-05 Thread Kai-Heng Feng



> On Dec 5, 2019, at 00:08, Deucher, Alexander  
> wrote:
> 
>> -Original Message-
>> From: Deucher, Alexander
>> Sent: Monday, December 2, 2019 11:37 AM
>> To: Lucas Stach ; Kai-Heng Feng
>> ; j...@8bytes.org; Koenig, Christian
>> (christian.koe...@amd.com) 
>> Cc: iommu@lists.linux-foundation.org; linux-ker...@vger.kernel.org
>> Subject: RE: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge
>> systems
>> 
>>> -Original Message-----
>>> From: Lucas Stach 
>>> Sent: Sunday, December 1, 2019 7:43 AM
>>> To: Kai-Heng Feng ; j...@8bytes.org
>>> Cc: Deucher, Alexander ;
>>> iommu@lists.linux-foundation.org; linux-ker...@vger.kernel.org
>>> Subject: Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge
>>> systems
>>> 
>>> Am Freitag, den 29.11.2019, 22:21 +0800 schrieb Kai-Heng Feng:
>>>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>>>> 
>>>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's
>>>> do the same here to avoid screen flickering on 4K monitor.
>>> 
>>> This doesn't seem like a good solution, especially if there isn't a
>>> method for the user to opt-out.  Some users might prefer having the
>>> IOMMU support to 4K display output.
>>> 
>>> But before using the big hammer of disabling or breaking one of those
>>> features, we should take a look at what's the issue here. Screen
>>> flickering caused by the IOMMU being active hints to the IOMMU not
>>> being able to sustain the translation bandwidth required by the high-
>>> bandwidth isochronous transfers caused by 4K scanout, most likely due
>>> to insufficient TLB space.
>>> 
>>> As far as I know the framebuffer memory for the display buffers is
>>> located in stolen RAM, and thus contigous in memory. I don't know the
>>> details of the GPU integration on those APUs, but maybe there even is
>>> a way to bypass the IOMMU for the stolen VRAM regions?
>>> 
>>> If there isn't and all GPU traffic passes through the IOMMU when
>>> active, we should check if the stolen RAM is mapped with hugepages on
>>> the IOMMU side. All the stolen RAM can most likely be mapped with a
>>> few hugepage mappings, which should reduce IOMMU TLB demand by a
>> large margin.
>> 
>> The is no issue when we scan out of the carve out region.  The issue occurs
>> when we scan out of regular system memory (scatter/gather).  Many newer
>> laptops have very small carve out regions (e.g., 32 MB), so we have to use
>> regular system pages to support multiple high resolution displays.  The
>> problem is, the latency gets too high at some point when the IOMMU is
>> involved.  Huge pages would probably help in this case, but I'm not sure if
>> there is any way to guarantee that we get huge pages for system memory.  I
>> guess we could use CMA or something like that.
> 
> Thomas recently sent out a patch set to add huge page support to ttm:
> https://patchwork.freedesktop.org/series/70090/
> We'd still need a way to guarantee huge pages for the display buffer.

Is there an amdgpu counterpart to let me test out?

Kai-Heng

> 
> Alex
> 
>> 
>> Alex
>> 
>>> 
>>> Regards,
>>> Lucas
>>> 
>>>> Cc: Alex Deucher 
>>>> Bug:
>>>> 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
>>>> tl
>>>> 
>>> 
>> ab.freedesktop.org%2Fdrm%2Famd%2Fissues%2F961data=02%7C01%
>>> 7Calexa
>>>> 
>>> 
>> nder.deucher%40amd.com%7C30540b2bf2be417c4d9508d7765bf07f%7C3dd
>>> 8961fe4
>>>> 
>>> 
>> 884e608e11a82d994e183d%7C0%7C0%7C637108010075463266sdata=1
>>> ZIZUWos
>>>> cPiB4auOY10jlGzoFeWszYMDBQG0CtrrOO8%3Dreserved=0
>>>> Signed-off-by: Kai-Heng Feng 
>>>> ---
>>>> v2:
>>>> - Find Stoney graphics instead of host bridge.
>>>> 
>>>> drivers/iommu/amd_iommu_init.c | 13 -
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/iommu/amd_iommu_init.c
>>>> b/drivers/iommu/amd_iommu_init.c index 568c52317757..139aa6fdadda
>>>> 100644
>>>> --- a/drivers/iommu/amd_iommu_init.c
>>>> +++ b/drivers/iommu/amd_iommu_init.c
>>>> @@ -2516,6 +2516,7 @@ static int __init early_amd_iommu_init(void)
>>>>struct acpi_table_header *ivrs_base;
>&

Re: [PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2019-12-05 Thread Kai-Heng Feng
Hi Joerg,

> On Dec 3, 2019, at 01:00, Christoph Hellwig  wrote:
> 
> On Fri, Nov 29, 2019 at 10:21:54PM +0800, Kai-Heng Feng wrote:
>> Serious screen flickering when Stoney Ridge outputs to a 4K monitor.
>> 
>> According to Alex Deucher, IOMMU isn't enabled on Windows, so let's do
>> the same here to avoid screen flickering on 4K monitor.
> 
> Disabling the IOMMU entirely seem pretty severe.  Isn't it enough to
> identity map the GPU device?

Ok, there's set_device_exclusion_range() to exclude the device from IOMMU.
However I don't know how to generate range_start and range_length, which are 
read from ACPI.

Can you please give me some advice here?

Kai-Henge
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v2] iommu/amd: Disable IOMMU on Stoney Ridge systems

2019-11-29 Thread Kai-Heng Feng
Serious screen flickering when Stoney Ridge outputs to a 4K monitor.

According to Alex Deucher, IOMMU isn't enabled on Windows, so let's do
the same here to avoid screen flickering on 4K monitor.

Cc: Alex Deucher 
Bug: https://gitlab.freedesktop.org/drm/amd/issues/961
Signed-off-by: Kai-Heng Feng 
---
v2:
- Find Stoney graphics instead of host bridge.

 drivers/iommu/amd_iommu_init.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 568c52317757..139aa6fdadda 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2516,6 +2516,7 @@ static int __init early_amd_iommu_init(void)
struct acpi_table_header *ivrs_base;
acpi_status status;
int i, remap_cache_sz, ret = 0;
+   u32 pci_id;
 
if (!amd_iommu_detected)
return -ENODEV;
@@ -2603,6 +2604,16 @@ static int __init early_amd_iommu_init(void)
if (ret)
goto out;
 
+   /* Disable IOMMU if there's Stoney Ridge graphics */
+   for (i = 0; i < 32; i++) {
+   pci_id = read_pci_config(0, i, 0, 0);
+   if ((pci_id & 0x) == 0x1002 && (pci_id >> 16) == 0x98e4) {
+   pr_info("Disable IOMMU on Stoney Ridge\n");
+   amd_iommu_disabled = true;
+   break;
+   }
+   }
+
/* Disable any previously enabled IOMMUs */
if (!is_kdump_kernel() || amd_iommu_disabled)
disable_iommus();
@@ -2711,7 +2722,7 @@ static int __init state_next(void)
ret = early_amd_iommu_init();
init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {
-   pr_info("AMD IOMMU disabled on kernel command-line\n");
+   pr_info("AMD IOMMU disabled\n");
init_state = IOMMU_CMDLINE_DISABLED;
ret = -EINVAL;
}
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3] iommu/amd: Override wrong IVRS IOAPIC on Raven Ridge systems

2019-08-20 Thread Kai-Heng Feng
Raven Ridge systems may have malfunction touchpad or hang at boot if
incorrect IVRS IOAPIC is provided by BIOS.

Users already found correct "ivrs_ioapic=" values, let's put them inside
kernel to workaround buggy BIOS.

BugLink: https://bugs.launchpad.net/bugs/1795292
BugLink: https://bugs.launchpad.net/bugs/1837688
Reported-by: kbuild test robot 
Signed-off-by: Kai-Heng Feng 
---
v3:
Fix compilation error when CONFIG_DMI is not enabled.

v2:
Split the quirk to another file.

 drivers/iommu/Makefile   |  2 +-
 drivers/iommu/amd_iommu.h| 14 +
 drivers/iommu/amd_iommu_init.c   |  5 +-
 drivers/iommu/amd_iommu_quirks.c | 92 
 4 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu.h
 create mode 100644 drivers/iommu/amd_iommu_quirks.c

diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index f13f36ae1af6..c6a277e69848 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU) += of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
-obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
+obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o amd_iommu_quirks.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd_iommu_debugfs.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
diff --git a/drivers/iommu/amd_iommu.h b/drivers/iommu/amd_iommu.h
new file mode 100644
index ..12d540d9b59b
--- /dev/null
+++ b/drivers/iommu/amd_iommu.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef AMD_IOMMU_H
+#define AMD_IOMMU_H
+
+int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line);
+
+#ifdef CONFIG_DMI
+void amd_iommu_apply_ivrs_quirks(void);
+#else
+static void amd_iommu_apply_ivrs_quirks(void) { }
+#endif
+
+#endif
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 4413aa67000e..568c52317757 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -32,6 +32,7 @@
 #include 
 
 #include 
+#include "amd_iommu.h"
 #include "amd_iommu_proto.h"
 #include "amd_iommu_types.h"
 #include "irq_remapping.h"
@@ -1002,7 +1003,7 @@ static void __init set_dev_entry_from_acpi(struct 
amd_iommu *iommu,
set_iommu_for_device(iommu, devid);
 }
 
-static int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line)
+int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line)
 {
struct devid_map *entry;
struct list_head *list;
@@ -1153,6 +1154,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu 
*iommu,
if (ret)
return ret;
 
+   amd_iommu_apply_ivrs_quirks();
+
/*
 * First save the recommended feature enable bits from ACPI
 */
diff --git a/drivers/iommu/amd_iommu_quirks.c b/drivers/iommu/amd_iommu_quirks.c
new file mode 100644
index ..c235f79b7a20
--- /dev/null
+++ b/drivers/iommu/amd_iommu_quirks.c
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * Quirks for AMD IOMMU
+ *
+ * Copyright (C) 2019 Kai-Heng Feng 
+ */
+
+#ifdef CONFIG_DMI
+#include 
+
+#include "amd_iommu.h"
+
+#define IVHD_SPECIAL_IOAPIC1
+
+struct ivrs_quirk_entry {
+   u8 id;
+   u16 devid;
+};
+
+enum {
+   DELL_INSPIRON_7375 = 0,
+   DELL_LATITUDE_5495,
+   LENOVO_IDEAPAD_330S_15ARR,
+};
+
+static const struct ivrs_quirk_entry ivrs_ioapic_quirks[][3] __initconst = {
+   /* ivrs_ioapic[4]=00:14.0 ivrs_ioapic[5]=00:00.2 */
+   [DELL_INSPIRON_7375] = {
+   { .id = 4, .devid = 0xa0 },
+   { .id = 5, .devid = 0x2 },
+   {}
+   },
+   /* ivrs_ioapic[4]=00:14.0 */
+   [DELL_LATITUDE_5495] = {
+   { .id = 4, .devid = 0xa0 },
+   {}
+   },
+   /* ivrs_ioapic[32]=00:14.0 */
+   [LENOVO_IDEAPAD_330S_15ARR] = {
+   { .id = 32, .devid = 0xa0 },
+   {}
+   },
+   {}
+};
+
+static int __init ivrs_ioapic_quirk_cb(const struct dmi_system_id *d)
+{
+   const struct ivrs_quirk_entry *i;
+
+   for (i = d->driver_data; i->id != 0 && i->devid != 0; i++)
+   add_special_device(IVHD_SPECIAL_IOAPIC, i->id, (u16 
*)>devid, 0);
+
+   return 0;
+}
+
+static const struct dmi_system_id ivrs_quirks[] __initconst = {
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Dell Inspiron 7375",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7375"),
+   },
+   .driver_data = (void *)_ioapic_quirks[DELL_INSPIRON_7375],
+   },
+   {
+   

[PATCH v2] iommu/amd: Override wrong IVRS IOAPIC on Raven Ridge systems

2019-08-17 Thread Kai-Heng Feng
Raven Ridge systems may have malfunction touchpad or hang at boot if
incorrect IVRS IOAPIC is provided by BIOS.

Users already found correct "ivrs_ioapic=" values, let's put them inside
kernel to workaround buggy BIOS.

BugLink: https://bugs.launchpad.net/bugs/1795292
BugLink: https://bugs.launchpad.net/bugs/1837688
Signed-off-by: Kai-Heng Feng 
---
v2:
Split the quirk to another file.

 drivers/iommu/Makefile   |  2 +-
 drivers/iommu/amd_iommu.h| 14 +
 drivers/iommu/amd_iommu_init.c   |  5 +-
 drivers/iommu/amd_iommu_quirks.c | 90 
 4 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd_iommu.h
 create mode 100644 drivers/iommu/amd_iommu_quirks.c

diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index f13f36ae1af6..c6a277e69848 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU) += of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
-obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
+obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o amd_iommu_quirks.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd_iommu_debugfs.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
diff --git a/drivers/iommu/amd_iommu.h b/drivers/iommu/amd_iommu.h
new file mode 100644
index ..12d540d9b59b
--- /dev/null
+++ b/drivers/iommu/amd_iommu.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef AMD_IOMMU_H
+#define AMD_IOMMU_H
+
+int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line);
+
+#ifdef CONFIG_DMI
+void amd_iommu_apply_ivrs_quirks(void);
+#else
+static void amd_iommu_apply_ivrs_quirks(void) { }
+#endif
+
+#endif
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 4413aa67000e..568c52317757 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -32,6 +32,7 @@
 #include 
 
 #include 
+#include "amd_iommu.h"
 #include "amd_iommu_proto.h"
 #include "amd_iommu_types.h"
 #include "irq_remapping.h"
@@ -1002,7 +1003,7 @@ static void __init set_dev_entry_from_acpi(struct 
amd_iommu *iommu,
set_iommu_for_device(iommu, devid);
 }
 
-static int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line)
+int __init add_special_device(u8 type, u8 id, u16 *devid, bool cmd_line)
 {
struct devid_map *entry;
struct list_head *list;
@@ -1153,6 +1154,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu 
*iommu,
if (ret)
return ret;
 
+   amd_iommu_apply_ivrs_quirks();
+
/*
 * First save the recommended feature enable bits from ACPI
 */
diff --git a/drivers/iommu/amd_iommu_quirks.c b/drivers/iommu/amd_iommu_quirks.c
new file mode 100644
index ..14181f0f5c2a
--- /dev/null
+++ b/drivers/iommu/amd_iommu_quirks.c
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * Quirks for AMD IOMMU
+ *
+ * Copyright (C) 2019 Kai-Heng Feng 
+ */
+
+#include 
+
+#include "amd_iommu.h"
+
+#define IVHD_SPECIAL_IOAPIC1
+
+struct ivrs_quirk_entry {
+   u8 id;
+   u16 devid;
+};
+
+enum {
+   DELL_INSPIRON_7375 = 0,
+   DELL_LATITUDE_5495,
+   LENOVO_IDEAPAD_330S_15ARR,
+};
+
+static const struct ivrs_quirk_entry ivrs_ioapic_quirks[][3] __initconst = {
+   /* ivrs_ioapic[4]=00:14.0 ivrs_ioapic[5]=00:00.2 */
+   [DELL_INSPIRON_7375] = {
+   { .id = 4, .devid = 0xa0 },
+   { .id = 5, .devid = 0x2 },
+   {}
+   },
+   /* ivrs_ioapic[4]=00:14.0 */
+   [DELL_LATITUDE_5495] = {
+   { .id = 4, .devid = 0xa0 },
+   {}
+   },
+   /* ivrs_ioapic[32]=00:14.0 */
+   [LENOVO_IDEAPAD_330S_15ARR] = {
+   { .id = 32, .devid = 0xa0 },
+   {}
+   },
+   {}
+};
+
+static int __init ivrs_ioapic_quirk_cb(const struct dmi_system_id *d)
+{
+   const struct ivrs_quirk_entry *i;
+
+   for (i = d->driver_data; i->id != 0 && i->devid != 0; i++)
+   add_special_device(IVHD_SPECIAL_IOAPIC, i->id, (u16 
*)>devid, 0);
+
+   return 0;
+}
+
+static const struct dmi_system_id ivrs_quirks[] __initconst = {
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Dell Inspiron 7375",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7375"),
+   },
+   .driver_data = (void *)_ioapic_quirks[DELL_INSPIRON_7375],
+   },
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Dell Latitude 5495",
+  

Re: [PATCH] iommu/amd: Override wrong IVRS IOAPIC on Raven Ridge systems

2019-08-12 Thread Kai-Heng Feng

at 23:39, Joerg Roedel  wrote:


On Thu, Aug 08, 2019 at 06:17:07PM +0800, Kai-Heng Feng wrote:

Raven Ridge systems may have malfunction touchpad or hang at boot if
incorrect IVRS IOAPIC is provided by BIOS.

Users already found correct "ivrs_ioapic=" values, let's put them inside
kernel to workaround buggy BIOS.


Will that still work when a fixed BIOS for these laptops is released?


Do you mean that we should stop applying these quirks once a BIOS fix is  
confirmed?


We can modify the quirk to compare BIOS version, if there’s an unlikely  
BIOS update really fixes the issue.

Before that happens, I think it’s OK to let the quirks stay this way.

Kai-Heng




Joerg



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/amd: Override wrong IVRS IOAPIC on Raven Ridge systems

2019-08-08 Thread Kai-Heng Feng
Raven Ridge systems may have malfunction touchpad or hang at boot if
incorrect IVRS IOAPIC is provided by BIOS.

Users already found correct "ivrs_ioapic=" values, let's put them inside
kernel to workaround buggy BIOS.

BugLink: https://bugs.launchpad.net/bugs/1795292
BugLink: https://bugs.launchpad.net/bugs/1837688
Signed-off-by: Kai-Heng Feng 
---
 drivers/iommu/amd_iommu_init.c | 75 ++
 1 file changed, 75 insertions(+)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 4413aa67000e..06fd008281e5 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1109,6 +1110,78 @@ static int __init add_early_maps(void)
return 0;
 }
 
+struct quirk_entry {
+   u8 id;
+   u16 devid;
+};
+
+enum {
+   DELL_INSPIRON_7375 = 0,
+   DELL_LATITUDE_5495,
+   LENOVO_IDEAPAD_330S_15ARR,
+};
+
+static const struct quirk_entry ivrs_ioapic_quirks[][3] __initconst = {
+   /* ivrs_ioapic[4]=00:14.0 ivrs_ioapic[5]=00:00.2 */
+   [DELL_INSPIRON_7375] = {
+   { .id = 4, .devid = 0xa0 },
+   { .id = 5, .devid = 0x2 },
+   {}
+   },
+   /* ivrs_ioapic[4]=00:14.0 */
+   [DELL_LATITUDE_5495] = {
+   { .id = 4, .devid = 0xa0 },
+   {}
+   },
+   /* ivrs_ioapic[32]=00:14.0 */
+   [LENOVO_IDEAPAD_330S_15ARR] = {
+   { .id = 32, .devid = 0xa0 },
+   {}
+   },
+   {}
+};
+
+static int __init ivrs_ioapic_quirk_cb(const struct dmi_system_id *d)
+{
+   const struct quirk_entry *i;
+
+   for (i = d->driver_data; i->id != 0 && i->devid != 0; i++)
+   add_special_device(IVHD_SPECIAL_IOAPIC, i->id, >devid, 0);
+
+   return 0;
+}
+
+static const struct dmi_system_id ivrs_quirks[] __initconst = {
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Dell Inspiron 7375",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7375"),
+   },
+   .driver_data = (void *)_ioapic_quirks[DELL_INSPIRON_7375],
+   },
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Dell Latitude 5495",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5495"),
+   },
+   .driver_data = (void *)_ioapic_quirks[DELL_LATITUDE_5495],
+   },
+   {
+   .callback = ivrs_ioapic_quirk_cb,
+   .ident = "Lenovo ideapad 330S-15ARR",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+   DMI_MATCH(DMI_PRODUCT_NAME, "81FB"),
+   },
+   .driver_data = (void 
*)_ioapic_quirks[LENOVO_IDEAPAD_330S_15ARR],
+   },
+   {}
+};
+
 /*
  * Reads the device exclusion range from ACPI and initializes the IOMMU with
  * it
@@ -1153,6 +1226,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu 
*iommu,
if (ret)
return ret;
 
+   dmi_check_system(ivrs_quirks);
+
/*
 * First save the recommended feature enable bits from ACPI
 */
-- 
2.17.1