[PATCH kernel] powerpc/npu: Do not try invalidating 32bit table when 64bit table is enabled

[Alexey Kardashevskiy]

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 <aik at ozlabs.ru>
> --
> 
> Do we need BUG_ON(IOMMU_TABLE_GROUP_MAX_TABLES != 2)?



Ping?



> 
> 
> 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