2015-12-11 23:05:48

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: [PATCH] iommu/amd: Assign default IOMMU when there is only one IOMMU

Current driver makes assumption that device with devid zero is always
included in the range of devices to be managed by IOMMU. However,
certain FW does not include devid zero in IVRS table.
This has caused IOMMU perf driver to fail to initialize.

This patch implements a workaround for this case by always assign
devid zero to be handled by the first IOMMU.

Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
drivers/iommu/amd_iommu_init.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdff..3bbad5c 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -656,6 +656,16 @@ static void __init set_iommu_for_device(struct amd_iommu *iommu, u16 devid)
amd_iommu_rlookup_table[devid] = iommu;
}

+static struct amd_iommu *get_iommu_for_device(u16 devid)
+{
+ struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+ /* Workaround: Always assign devid zero to the first IOMMU */
+ if (!iommu && !devid && amd_iommus_present)
+ iommu = amd_iommus[0];
+ return iommu;
+}
+
/*
* This function takes the device specific flags read from the ACPI
* table and sets up the device table entry with that information
@@ -751,7 +761,7 @@ static int __init add_early_maps(void)
*/
static void __init set_device_exclusion_range(u16 devid, struct ivmd_header *m)
{
- struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+ struct amd_iommu *iommu = get_iommu_for_device(devid);

if (!(m->flags & IVMD_FLAG_EXCL_RANGE))
return;
@@ -2255,7 +2265,7 @@ u8 amd_iommu_pc_get_max_banks(u16 devid)
u8 ret = 0;

/* locate the iommu governing the devid */
- iommu = amd_iommu_rlookup_table[devid];
+ iommu = get_iommu_for_device(devid);
if (iommu)
ret = iommu->max_banks;

@@ -2275,7 +2285,7 @@ u8 amd_iommu_pc_get_max_counters(u16 devid)
u8 ret = 0;

/* locate the iommu governing the devid */
- iommu = amd_iommu_rlookup_table[devid];
+ iommu = get_iommu_for_device(devid);
if (iommu)
ret = iommu->max_counters;

@@ -2295,7 +2305,7 @@ int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
return -ENODEV;

/* Locate the iommu associated with the device ID */
- iommu = amd_iommu_rlookup_table[devid];
+ iommu = get_iommu_for_device(devid);

/* Check for valid iommu and pc register indexing */
if (WARN_ON((iommu == NULL) || (fxn > 0x28) || (fxn & 7)))
--
1.9.1


2015-12-14 15:08:10

by Joerg Roedel

[permalink] [raw]
Subject: Re: [PATCH] iommu/amd: Assign default IOMMU when there is only one IOMMU

On Fri, Dec 11, 2015 at 04:54:38PM -0600, Suravee Suthikulpanit wrote:
> Current driver makes assumption that device with devid zero is always
> included in the range of devices to be managed by IOMMU. However,
> certain FW does not include devid zero in IVRS table.
> This has caused IOMMU perf driver to fail to initialize.

Hmm, this is a firmware bug. Is this bug seen in any systems that are
for sale?

> This patch implements a workaround for this case by always assign
> devid zero to be handled by the first IOMMU.

Otherwise its better to fix the firmware than to add workarounds.


Joerg

2015-12-16 06:20:16

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: Re: [PATCH] iommu/amd: Assign default IOMMU when there is only one IOMMU

Hi Joerg,

On 12/14/2015 09:08 AM, Joerg Roedel wrote:
> On Fri, Dec 11, 2015 at 04:54:38PM -0600, Suravee Suthikulpanit wrote:
>> Current driver makes assumption that device with devid zero is always
>> included in the range of devices to be managed by IOMMU. However,
>> certain FW does not include devid zero in IVRS table.
>> This has caused IOMMU perf driver to fail to initialize.
>
> Hmm, this is a firmware bug. Is this bug seen in any systems that are
> for sale?
>
>> This patch implements a workaround for this case by always assign
>> devid zero to be handled by the first IOMMU.
>
> Otherwise its better to fix the firmware than to add workarounds.
>
>
> Joerg
>

Please correct me if I am wrong. But I don't think this is necessary a
bug in the FW. From the CZ system, here are the IVHD device entry dump
that belong to this IOMMU:

[ 0.070351] AMD-Vi: device: 00:00.2 cap: 0040 seg: 0 flags: b8 info 0000
[ 0.070354] AMD-Vi: mmio-addr: 00000000feb80000
[ 0.070384] AMD-Vi: DEV_SELECT_RANGE_START devid: 00:01.0 flags: 00
[ 0.070386] AMD-Vi: DEV_RANGE_END devid: ff:1f.6
[ 0.071414] AMD-Vi: DEV_ALIAS_RANGE devid: ff:00.0 flags:
00 devid_to: 00:14.4
[ 0.071417] AMD-Vi: DEV_RANGE_END devid: ff:1f.7
[ 0.071423] AMD-Vi: DEV_SPECIAL(HPET[0]) devid: 00:14.0
[ 0.071426] AMD-Vi: DEV_SPECIAL(IOAPIC[0]) devid: 00:14.0
[ 0.071427] AMD-Vi: DEV_SPECIAL(IOAPIC[1]) devid: 00:00.1

And here is the output from lspci:

00:00.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1576
00:00.2 IOMMU: Advanced Micro Devices, Inc. [AMD] Device 1577
00:01.0 VGA compatible controller: Advanced Micro Devices, Inc.
[AMD/ATI] Device 9874 (rev c4)
00:01.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Device 9840
00:02.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 157b
00:02.4 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 157c
00:03.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 157b
00:03.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 157c
00:08.0 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1578
00:09.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 157d
00:09.2 Audio device: Advanced Micro Devices, Inc. [AMD] Device 157a
00:10.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB XHCI
Controller (rev 20)
00:11.0 SATA controller: Advanced Micro Devices, Inc. [AMD] FCH SATA
Controller [AHCI mode] (rev 49)
00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI
Controller (rev 49)
00:14.0 SMBus: Advanced Micro Devices, Inc. [AMD] FCH SMBus Controller
(rev 4a)
00:14.3 ISA bridge: Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge
(rev 11)
00:14.7 SD Host controller: Advanced Micro Devices, Inc. [AMD] FCH SD
Flash Controller (rev 01)
00:18.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1570
00:18.1 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1571
00:18.2 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1572
00:18.3 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1573
00:18.4 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1574
00:18.5 Host bridge: Advanced Micro Devices, Inc. [AMD] Device 1575
01:00.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5720
Gigabit Ethernet PCIe
01:00.1 Ethernet controller: Broadcom Corporation NetXtreme BCM5720
Gigabit Ethernet PCIe
02:00.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit
SFI/SFP+ Network Connection (rev 01)
02:00.1 Ethernet controller: Intel Corporation 82599ES 10-Gigabit
SFI/SFP+ Network Connection (rev 01)

The IVHD seems valid. We should not need to include from devid 0 if it
has already specified the IOAPIC[1] separately.

Thanks,
Suravee

2015-12-17 03:18:09

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: Re: [PATCH] iommu/amd: Assign default IOMMU when there is only one IOMMU

On 12/14/2015 9:08 AM, Joerg Roedel wrote:
> On Fri, Dec 11, 2015 at 04:54:38PM -0600, Suravee Suthikulpanit wrote:
>> Current driver makes assumption that device with devid zero is always
>> included in the range of devices to be managed by IOMMU. However,
>> certain FW does not include devid zero in IVRS table.
>> This has caused IOMMU perf driver to fail to initialize.
>
> Hmm, this is a firmware bug. Is this bug seen in any systems that are
> for sale?
>
>> This patch implements a workaround for this case by always assign
>> devid zero to be handled by the first IOMMU.
>
> Otherwise its better to fix the firmware than to add workarounds.
>
>
> Joerg
>

Please ignore this patch. There are more stuff that I am planning to
fix, and I am reworking them into V2. I'll send this out soon.

Suravee