On 7/3/18 2:40 pm, Alexey Kardashevskiy wrote:
> On 13/02/18 16:51, Alexey Kardashevskiy wrote:
>> GPUs and the corresponding NVLink bridges get different PEs as they have
>> separate translation validation entries (TVEs). We put these PEs to
>> the same IOMMU group so they cannot be passed through separately.
>> So the iommu_table_group_ops::set_window/unset_window for GPUs do set
>> tables to the NPU PEs as well which means that iommu_table's list of
>> attached PEs (iommu_table_group_link) has both GPU and NPU PEs linked.
>> This list is used for TCE cache invalidation.
>>
>> The problem is that NPU PE has just a single TVE and can be programmed
>> to point to 32bit or 64bit windows while GPU PE has two (as any other PCI
>> device). So we end up having an 32bit iommu_table struct linked to both
>> PEs even though only the 64bit TCE table cache can be invalidated on NPU.
>> And a relatively recent skiboot detects this and prints errors.
>>
>> This changes GPU's iommu_table_group_ops::set_window/unset_window to make
>> sure that NPU PE is only linked to the table actually used by the hardware.
>> If there are two tables used by an IOMMU group, the NPU PE will use
>> the last programmed one which with the current use scenarios is expected
>> to be a 64bit one.
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> --
>>
>> Do we need BUG_ON(IOMMU_TABLE_GROUP_MAX_TABLES != 2)?
>
>
>
> Ping?
Anyone? Alistair? :)
>
>
>
>>
>>
>> This is an example for:
>>
>> 0004:04:00.0 3D: NVIDIA Corporation Device 1db1 (rev a1)
>> 0006:00:00.0 Bridge: IBM Device 04ea (rev 01)
>> 0006:00:00.1 Bridge: IBM Device 04ea (rev 01)
>>
>> Before the patch (npu2_tce_kill messages are from skiboot):
>>
>> pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
>> pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
>> pci 0004:04 : [PE# 00] Setting up window#1
>> 800000000000000..8000000ffffffff pg=10000
>> pci 0006:00:00.0: [PE# 0d] Setting up window
>> 800000000000000..8000000ffffffff pg=10000
>> NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
>> NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
>> NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
>> NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
>> NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
>> ...
>> pci 0004:04 : [PE# 00] Removing DMA window #0
>> pci 0006:00:00.0: [PE# 0d] Removing DMA window
>> pci 0004:04 : [PE# 00] Removing DMA window #1
>> pci 0006:00:00.0: [PE# 0d] Removing DMA window
>> pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
>> pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
>> pci 0004:04 : [PE# 00] Setting up window#1
>> 800000000000000..8000000ffffffff pg=10000
>> pci 0006:00:00.0: [PE# 0d] Setting up window
>> 800000000000000..8000000ffffffff pg=10000
>>
>> After the patch (no errors here):
>>
>> pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
>> pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
>> pci 0004:04 : [PE# 00] Setting up window#1
>> 800000000000000..8000000ffffffff pg=10000
>> pci 0006:00:00.0: [PE# 0d] Removing DMA window
>> pci 0006:00:00.0: [PE# 0d] Setting up window
>> 800000000000000..8000000ffffffff pg=10000
>> pci 0004:04 : [PE# 00] Removing DMA window #0
>> pci 0004:04 : [PE# 00] Removing DMA window #1
>> pci 0006:00:00.0: [PE# 0d] Removing DMA window
>> pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
>> pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
>> pci 0004:04 : [PE# 00] Setting up window#1
>> 800000000000000..8000000ffffffff pg=10000
>> pci 0006:00:00.0: [PE# 0d] Removing DMA window
>> pci 0006:00:00.0: [PE# 0d] Setting up window
>> 800000000000000..8000000ffffffff pg=10000
>> ---
>> arch/powerpc/platforms/powernv/pci-ioda.c | 27 ++++++++++++++++++++++++---
>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
>> b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index 496e476..2f91815 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -2681,14 +2681,23 @@ static struct pnv_ioda_pe *gpe_table_group_to_npe(
>> static long pnv_pci_ioda2_npu_set_window(struct iommu_table_group
>> *table_group,
>> int num, struct iommu_table *tbl)
>> {
>> + struct pnv_ioda_pe *npe = gpe_table_group_to_npe(table_group);
>> + int num2 = (num == 0) ? 1 : 0;
>> long ret = pnv_pci_ioda2_set_window(table_group, num, tbl);
>>
>> if (ret)
>> return ret;
>>
>> - ret = pnv_npu_set_window(gpe_table_group_to_npe(table_group), num, tbl);
>> - if (ret)
>> + if (table_group->tables[num2])
>> + pnv_npu_unset_window(npe, num2);
>> +
>> + ret = pnv_npu_set_window(npe, num, tbl);
>> + if (ret) {
>> pnv_pci_ioda2_unset_window(table_group, num);
>> + if (table_group->tables[num2])
>> + pnv_npu_set_window(npe, num2,
>> + table_group->tables[num2]);
>> + }
>>
>> return ret;
>> }
>> @@ -2697,12 +2706,24 @@ static long pnv_pci_ioda2_npu_unset_window(
>> struct iommu_table_group *table_group,
>> int num)
>> {
>> + struct pnv_ioda_pe *npe = gpe_table_group_to_npe(table_group);
>> + int num2 = (num == 0) ? 1 : 0;
>> long ret = pnv_pci_ioda2_unset_window(table_group, num);
>>
>> if (ret)
>> return ret;
>>
>> - return pnv_npu_unset_window(gpe_table_group_to_npe(table_group), num);
>> + if (!npe->table_group.tables[num])
>> + return 0;
>> +
>> + ret = pnv_npu_unset_window(npe, num);
>> + if (ret)
>> + return ret;
>> +
>> + if (table_group->tables[num2])
>> + ret = pnv_npu_set_window(npe, num2, table_group->tables[num2]);
>> +
>> + return ret;
>> }
>>
>> static void pnv_ioda2_npu_take_ownership(struct iommu_table_group
>> *table_group)
>>
>
>
--
Alexey
