drm/nouveau: Enable GP10B by default

From: Thierry Reding <[email protected]>

If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by Thierry Reding <[email protected]>

---
  .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
  1 file changed, 7 insertions(+), 12 deletions(-)

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +0 -9

index d0d52c1d4aee..fc652aaa41c7 100644
@@ -23,10 +23,6 @@
  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
  #include "priv.h"
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
  static int
  nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
  {

@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
  	unsigned long pgsize_bitmap;
  	int ret;
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);

Hi Thierry,

Quoted 39 lines · Thierry Reding

From: Thierry Reding <[email protected]>

If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by Thierry Reding <[email protected]>

---
  .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
  1 file changed, 7 insertions(+), 12 deletions(-)

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +3 -12

index d0d52c1d4aee..fc652aaa41c7 100644
@@ -23,10 +23,6 @@
  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
  #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
  static int
  nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
  {

@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
  	unsigned long pgsize_bitmap;
  	int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
+	/*
+	 * Skip explicit IOMMU initialization if the GPU is already attached
+	 * to an IOMMU domain. This can happen if the DMA API is backed by an

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) · line 114

Quoted 4 lines

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +4 -0

+	 * IOMMU.
+	 */
+	if (iommu_get_domain_for_dev(dev))
+		return;

Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.

Quoted 42 lines · Robin Murphy

Hi Thierry,

Quoted 39 lines · Thierry Reding

From: Thierry Reding <[email protected]>

If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by Thierry Reding <[email protected]>

---
   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
   1 file changed, 7 insertions(+), 12 deletions(-)

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +3 -12

index d0d52c1d4aee..fc652aaa41c7 100644
@@ -23,10 +23,6 @@
   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
   #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
   static int
   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
   {

@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
   	unsigned long pgsize_bitmap;
   	int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
+	/*
+	 * Skip explicit IOMMU initialization if the GPU is already attached
+	 * to an IOMMU domain. This can happen if the DMA API is backed by an

Quoted 8 lines

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) · line 114

Quoted 4 lines

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +4 -0

+	 * IOMMU.
+	 */
+	if (iommu_get_domain_for_dev(dev))
+		return;

Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.

Good point. An earlier version of this patch had an additional check for IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.

Quoted 52 lines · Thierry Reding

Quoted 42 lines · Robin Murphy

Hi Thierry,

Quoted 39 lines · Thierry Reding

From: Thierry Reding <[email protected]>

If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by Thierry Reding <[email protected]>

---
   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
   1 file changed, 7 insertions(+), 12 deletions(-)

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +3 -12

index d0d52c1d4aee..fc652aaa41c7 100644
@@ -23,10 +23,6 @@
   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
   #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
   static int
   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
   {

@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
   	unsigned long pgsize_bitmap;
   	int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
+	/*
+	 * Skip explicit IOMMU initialization if the GPU is already attached
+	 * to an IOMMU domain. This can happen if the DMA API is backed by an

Quoted 8 lines

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) · line 114

Quoted 4 lines

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +4 -0

+	 * IOMMU.
+	 */
+	if (iommu_get_domain_for_dev(dev))
+		return;

Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.

Quoted 5 lines

Good point. An earlier version of this patch had an additional check for IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.

Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be sufficient to answer "is the DMA layer managing my address space for me?" unless and until some massive API change happens (which I certainly don't foresee).

From: Thierry Reding <[email protected]>

The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.

Signed-off-by Thierry Reding <[email protected]>

---
 .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
 .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81 ++++++++++++-------
 .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
 3 files changed, 53 insertions(+), 34 deletions(-)

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +1 -1

index 62c5e162099a..280b1448df88 100644
@@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *,
                           struct nvkm_falcon *);
 
 /* Tegra-only */
-int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
+int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
 
 #endif

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +27 -24

index df8b919dcf09..f8a543122219 100644
@@ -23,39 +23,65 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA210_MC_BASE                       0x70019000
-
 #ifdef CONFIG_ARCH_TEGRA
-#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
-#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
-#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
-#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
-#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
 /**
  * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
  *
- * On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- * is reserved from system memory by the bootloader and irreversibly locked.
- * This function reads the address and size of the pre-configured WPR region.
+ * On dGPU, we can manage the WPR region ourselves, but on Tegra this region
+ * is allocated from system memory by the secure firmware. The region is then
+ * marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
+ * secure carveout is also configured to be sent to the GPU via a dedicated
+ * serial bus between the memory controller and the GPU. The GPU requests this
+ * information upon leaving reset and exposes it through a FIFO register at
+ * offset 0x100cd4.
+ *
+ * The FIFO register's lower 4 bits can be used to set the read index into the
+ * FIFO. After each read of the FIFO register, the read index is incremented.
+ *
+ * Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
+ * stored in units of 256 B. The WPR is inclusive of both addresses.
+ *
+ * Unfortunately, for some reason the WPR info register doesn't contain the
+ * correct values for the secure carveout. It seems like the upper address is
+ * always too small by 128 KiB - 1. Given that the secure carvout size in the
+ * memory controller configuration is specified in units of 128 KiB, it's
+ * possible that the computation of the upper address of the WPR is wrong and
+ * causes this difference.
  */
 int
-gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
+gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
 {
+       struct nvkm_device *device = gsb->base.subdev.device;
        struct nvkm_secboot *sb = &gsb->base;
-       void __iomem *mc;
-       u32 cfg;
+       u64 base, limit;
+       u32 value;
 
-       mc = ioremap(mc_base, 0xd00);
-       if (!mc) {
-               nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");
-               return -ENOMEM;
-       }
-       sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |
-             ((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);
-       sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)
-               << 17;
-       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
-       iounmap(mc);
+       /* set WPR info register to point at WPR base address register */
+       value = nvkm_rd32(device, 0x100cd4);
+       value &= ~0xf;

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +14 -6

+
+       /*
+        * The upper address of the WPR seems to be computed wrongly and is
+        * actually SZ_128K - 1 bytes lower than it should be. Adjust the
+        * value accordingly.
+        */
+       limit += SZ_128K - 1;
+
+       sb->wpr_size = limit - base + 1;
+       sb->wpr_addr = base;
+
+       nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,
+                 sb->wpr_addr + sb->wpr_size - 1);
 
        /* Check that WPR settings are valid */
        if (sb->wpr_size == 0) {

@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
                return -EINVAL;
        }
 
-       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
-               nvkm_error(&sb->subdev, "WPR region not locked\n");
-               return -EINVAL;
-       }
-
        return 0;
 }
 #else

@@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +1 -4

index 28ca29d0eeee..d84e85825995 100644
@@ -23,15 +23,13 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA186_MC_BASE                       0x02c10000
-
 static int
 gp10b_secboot_oneinit(struct nvkm_secboot *sb)
 {
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 
--

2.23.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 99 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.

Signed-off-by Thierry Reding <[email protected]>

---
 .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
 .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81 ++++++++++++-------
 .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
 3 files changed, 53 insertions(+), 34 deletions(-)

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +1 -1

index 62c5e162099a..280b1448df88 100644
@@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *,
                           struct nvkm_falcon *);
 
 /* Tegra-only */
-int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
+int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
 
 #endif

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +32 -24

index df8b919dcf09..f8a543122219 100644
@@ -23,39 +23,65 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA210_MC_BASE                       0x70019000
-
 #ifdef CONFIG_ARCH_TEGRA
-#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
-#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
-#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
-#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
-#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
 /**
  * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
  *
- * On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- * is reserved from system memory by the bootloader and irreversibly locked.
- * This function reads the address and size of the pre-configured WPR region.
+ * On dGPU, we can manage the WPR region ourselves, but on Tegra this region
+ * is allocated from system memory by the secure firmware. The region is then
+ * marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
+ * secure carveout is also configured to be sent to the GPU via a dedicated
+ * serial bus between the memory controller and the GPU. The GPU requests this
+ * information upon leaving reset and exposes it through a FIFO register at
+ * offset 0x100cd4.
+ *
+ * The FIFO register's lower 4 bits can be used to set the read index into the
+ * FIFO. After each read of the FIFO register, the read index is incremented.
+ *
+ * Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
+ * stored in units of 256 B. The WPR is inclusive of both addresses.
+ *
+ * Unfortunately, for some reason the WPR info register doesn't contain the
+ * correct values for the secure carveout. It seems like the upper address is
+ * always too small by 128 KiB - 1. Given that the secure carvout size in the
+ * memory controller configuration is specified in units of 128 KiB, it's
+ * possible that the computation of the upper address of the WPR is wrong and
+ * causes this difference.
  */
 int
-gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
+gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
 {
+       struct nvkm_device *device = gsb->base.subdev.device;
        struct nvkm_secboot *sb = &gsb->base;
-       void __iomem *mc;
-       u32 cfg;
+       u64 base, limit;
+       u32 value;
 
-       mc = ioremap(mc_base, 0xd00);
-       if (!mc) {
-               nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");
-               return -ENOMEM;
-       }
-       sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |
-             ((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);
-       sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)
-               << 17;
-       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
-       iounmap(mc);
+       /* set WPR info register to point at WPR base address register */
+       value = nvkm_rd32(device, 0x100cd4);
+       value &= ~0xf;
+       value |= 0x2;
+       nvkm_wr32(device, 0x100cd4, value);
+
+       /* read base address */
+       value = nvkm_rd32(device, 0x100cd4);

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c line 71

Quoted 5 lines

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +5 -0

+       base = (u64)(value >> 4) << 12;
+
+       /* read limit */
+       value = nvkm_rd32(device, 0x100cd4);
+       limit = (u64)(value >> 4) << 12;

acr_r352_wpr_is_set() does a similar readout to confirm the HS firmware did its job on dGPU, perhaps this part of it could be factored out into a function that could be used in both places?

Quoted 59 lines

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +14 -6

+
+       /*
+        * The upper address of the WPR seems to be computed wrongly and is
+        * actually SZ_128K - 1 bytes lower than it should be. Adjust the
+        * value accordingly.
+        */
+       limit += SZ_128K - 1;
+
+       sb->wpr_size = limit - base + 1;
+       sb->wpr_addr = base;
+
+       nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,
+                 sb->wpr_addr + sb->wpr_size - 1);
 
        /* Check that WPR settings are valid */
        if (sb->wpr_size == 0) {

@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
                return -EINVAL;
        }
 
-       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
-               nvkm_error(&sb->subdev, "WPR region not locked\n");
-               return -EINVAL;
-       }
-
        return 0;
 }
 #else

@@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +1 -3

index 28ca29d0eeee..d84e85825995 100644
@@ -23,15 +23,13 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA186_MC_BASE                       0x02c10000
-
 static int
 gp10b_secboot_oneinit(struct nvkm_secboot *sb)
 {
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 

Quoted 7 lines

--
2.23.0

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 100 lines · Ben Skeggs

Quoted 99 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.

Signed-off-by Thierry Reding <[email protected]>

---
 .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
 .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81 ++++++++++++-------
 .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
 3 files changed, 53 insertions(+), 34 deletions(-)

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +1 -1

index 62c5e162099a..280b1448df88 100644
@@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *,
                           struct nvkm_falcon *);
 
 /* Tegra-only */
-int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
+int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
 
 #endif

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +32 -24

index df8b919dcf09..f8a543122219 100644
@@ -23,39 +23,65 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA210_MC_BASE                       0x70019000
-
 #ifdef CONFIG_ARCH_TEGRA
-#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
-#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
-#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
-#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
-#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
 /**
  * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
  *
- * On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- * is reserved from system memory by the bootloader and irreversibly locked.
- * This function reads the address and size of the pre-configured WPR region.
+ * On dGPU, we can manage the WPR region ourselves, but on Tegra this region
+ * is allocated from system memory by the secure firmware. The region is then
+ * marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
+ * secure carveout is also configured to be sent to the GPU via a dedicated
+ * serial bus between the memory controller and the GPU. The GPU requests this
+ * information upon leaving reset and exposes it through a FIFO register at
+ * offset 0x100cd4.
+ *
+ * The FIFO register's lower 4 bits can be used to set the read index into the
+ * FIFO. After each read of the FIFO register, the read index is incremented.
+ *
+ * Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
+ * stored in units of 256 B. The WPR is inclusive of both addresses.
+ *
+ * Unfortunately, for some reason the WPR info register doesn't contain the
+ * correct values for the secure carveout. It seems like the upper address is
+ * always too small by 128 KiB - 1. Given that the secure carvout size in the
+ * memory controller configuration is specified in units of 128 KiB, it's
+ * possible that the computation of the upper address of the WPR is wrong and
+ * causes this difference.
  */
 int
-gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
+gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
 {
+       struct nvkm_device *device = gsb->base.subdev.device;
        struct nvkm_secboot *sb = &gsb->base;
-       void __iomem *mc;
-       u32 cfg;
+       u64 base, limit;
+       u32 value;
 
-       mc = ioremap(mc_base, 0xd00);
-       if (!mc) {
-               nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");
-               return -ENOMEM;
-       }
-       sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |
-             ((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);
-       sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)
-               << 17;
-       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
-       iounmap(mc);
+       /* set WPR info register to point at WPR base address register */
+       value = nvkm_rd32(device, 0x100cd4);
+       value &= ~0xf;
+       value |= 0x2;
+       nvkm_wr32(device, 0x100cd4, value);
+
+       /* read base address */
+       value = nvkm_rd32(device, 0x100cd4);

Quoted 8 lines

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c line 71

Quoted 5 lines

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +5 -0

+       base = (u64)(value >> 4) << 12;
+
+       /* read limit */
+       value = nvkm_rd32(device, 0x100cd4);
+       limit = (u64)(value >> 4) << 12;

acr_r352_wpr_is_set() does a similar readout to confirm the HS firmware did its job on dGPU, perhaps this part of it could be factored out into a function that could be used in both places?

I hadn't seen that function. It looks to me like these are now both doing exactly the same thing. The acr_r352_wpr_is_set() also seems to serve only to check that what's read from these WPR info registers matches what was configured as the WPR region previously. This is now rather pointless because, unless the computations differ, the result must be true.

Honestly, I'm not sure we really need to check this. My understanding is that these WPR info registers are the canonical way of obtaining the WPR region base and limit. The way that this works is that the GPU has a dedicated bus to the memory controller and it fetches these parameters from the MC when it leaves reset.

One oddity here, though, is that the code in acr_r352_wpr_is_set() does not account for the strange way that the limit is encoded in these registers. I suspect that this is some weird hardware bug that happens during the transfer of the WPR information to the GPU. I came across some documentation that specifically defines the limit register to contain the upper limit of the WPR in units of 256 B with the WPR being inclusive of both the base and the limit. I'm not exactly sure why this has gone unnoticed, but I think nvgpu doesn't actually test for the WPR size when it loads the firmware. I only ran into this when implementing the WPR info register readout because Nouveau would refuse to load the firmware because it didn't fit into what it thought was the WPR.

Anyway, I've tested this on all of gm20b, gp10b and gv11b and the above computations give me the same values that the (SoC) firmware claims that it configured the WPR with.

Given the above, do you see any further use for acr_r352_wpr_is_set()? Should I follow up with a patch removing it?

Thierry

Quoted 59 lines

Quoted 59 lines

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +14 -6

+
+       /*
+        * The upper address of the WPR seems to be computed wrongly and is
+        * actually SZ_128K - 1 bytes lower than it should be. Adjust the
+        * value accordingly.
+        */
+       limit += SZ_128K - 1;
+
+       sb->wpr_size = limit - base + 1;
+       sb->wpr_addr = base;
+
+       nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,
+                 sb->wpr_addr + sb->wpr_size - 1);
 
        /* Check that WPR settings are valid */
        if (sb->wpr_size == 0) {

@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
                return -EINVAL;
        }
 
-       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
-               nvkm_error(&sb->subdev, "WPR region not locked\n");
-               return -EINVAL;
-       }
-
        return 0;
 }
 #else

@@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +1 -3

index 28ca29d0eeee..d84e85825995 100644
@@ -23,15 +23,13 @@
 #include "acr.h"
 #include "gm200.h"
 
-#define TEGRA186_MC_BASE                       0x02c10000
-
 static int
 gp10b_secboot_oneinit(struct nvkm_secboot *sb)
 {
        struct gm200_secboot *gsb = gm200_secboot(sb);
        int ret;
 
-       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
+       ret = gm20b_secboot_tegra_read_wpr(gsb);
        if (ret)
                return ret;
 

Quoted 7 lines

Quoted 7 lines

--
2.23.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Thierry Reding <[email protected]>

There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.

Signed-off-by Thierry Reding <[email protected]>

---
  .../gpu/drm/nouveau/include/nvkm/subdev/ltc.h |  1 +
  .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
  .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild    |  1 +
  .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c   | 69 +++++++++++++++++++
  .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h    |  2 +
  5 files changed, 74 insertions(+), 1 deletion(-)

create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c

drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +1 -0

index 644d527c3b96..d76f60d7d29a 100644
@@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
+int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  #endif

drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +1 -1

index c3c7159f3411..d2d6d5f4028a 100644
@@ -2380,7 +2380,7 @@ nv13b_chipset = {
  	.fuse = gm107_fuse_new,
  	.ibus = gp10b_ibus_new,
  	.imem = gk20a_instmem_new,
-	.ltc = gp102_ltc_new,
+	.ltc = gp10b_ltc_new,
  	.mc = gp10b_mc_new,
  	.mmu = gp10b_mmu_new,
  	.secboot = gp10b_secboot_new,

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +1 -0

index 2b6d36ea7067..728d75010847 100644
@@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o
  nvkm-y += nvkm/subdev/ltc/gm200.o
  nvkm-y += nvkm/subdev/ltc/gp100.o
  nvkm-y += nvkm/subdev/ltc/gp102.o
+nvkm-y += nvkm/subdev/ltc/gp10b.o

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file +37 -0

new file mode 100644
index 000000000000..4d27c6ea1552
@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2019 NVIDIA Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Thierry Reding
+ */
+
+#include "priv.h"
+
+static void
+gp10b_ltc_init(struct nvkm_ltc *ltc)
+{
+	struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
+	struct iommu_fwspec *spec;
+#endif
+
+	nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
+	nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
+	nvkm_wr32(device, 0x100800, ltc->ltc_nr);

Quoted 98 lines · Thierry Reding

From: Thierry Reding <[email protected]>

There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.

Signed-off-by Thierry Reding <[email protected]>

---
  .../gpu/drm/nouveau/include/nvkm/subdev/ltc.h |  1 +
  .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
  .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild    |  1 +
  .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c   | 69 +++++++++++++++++++
  .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h    |  2 +
  5 files changed, 74 insertions(+), 1 deletion(-)

create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c

drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +1 -0

index 644d527c3b96..d76f60d7d29a 100644
@@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
+int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  #endif

drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +1 -1

index c3c7159f3411..d2d6d5f4028a 100644
@@ -2380,7 +2380,7 @@ nv13b_chipset = {
  	.fuse = gm107_fuse_new,
  	.ibus = gp10b_ibus_new,
  	.imem = gk20a_instmem_new,
-	.ltc = gp102_ltc_new,
+	.ltc = gp10b_ltc_new,
  	.mc = gp10b_mc_new,
  	.mmu = gp10b_mmu_new,
  	.secboot = gp10b_secboot_new,

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +1 -0

index 2b6d36ea7067..728d75010847 100644
@@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o
  nvkm-y += nvkm/subdev/ltc/gm200.o
  nvkm-y += nvkm/subdev/ltc/gp100.o
  nvkm-y += nvkm/subdev/ltc/gp102.o
+nvkm-y += nvkm/subdev/ltc/gp10b.o

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file +44 -0

new file mode 100644
index 000000000000..4d27c6ea1552
@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2019 NVIDIA Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Thierry Reding
+ */
+
+#include "priv.h"
+
+static void
+gp10b_ltc_init(struct nvkm_ltc *ltc)
+{
+	struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
+	struct iommu_fwspec *spec;
+#endif
+
+	nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
+	nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
+	nvkm_wr32(device, 0x100800, ltc->ltc_nr);
+
+#ifdef CONFIG_IOMMU_API
+	spec = dev_iommu_fwspec_get(device->dev);
+	if (spec) {
+		u32 sid = spec->ids[0] & 0xffff;
+
+		/* stream ID */

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c line 47

Quoted 3 lines

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file +3 -0

+		nvkm_wr32(device, 0x160000, sid << 2);
+	}
+#endif

Could we get rid of the #ifdef blocks here if there was a NULL inline version of dev_iommu_fwspec_get() in the include/linux/iommu.h header? The compiler should then optimise the lot out if the config is not set.

Quoted 99 lines · Ben Dooks

Quoted 98 lines · Thierry Reding

From: Thierry Reding <[email protected]>

There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.

Signed-off-by Thierry Reding <[email protected]>

---
  .../gpu/drm/nouveau/include/nvkm/subdev/ltc.h |  1 +
  .../gpu/drm/nouveau/nvkm/engine/device/base.c |  2 +-
  .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild    |  1 +
  .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c   | 69 +++++++++++++++++++
  .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h    |  2 +
  5 files changed, 74 insertions(+), 1 deletion(-)

create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c

drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +1 -0

index 644d527c3b96..d76f60d7d29a 100644
@@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
+int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **);
  #endif

drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +1 -1

index c3c7159f3411..d2d6d5f4028a 100644
@@ -2380,7 +2380,7 @@ nv13b_chipset = {
  	.fuse = gm107_fuse_new,
  	.ibus = gp10b_ibus_new,
  	.imem = gk20a_instmem_new,
-	.ltc = gp102_ltc_new,
+	.ltc = gp10b_ltc_new,
  	.mc = gp10b_mc_new,
  	.mmu = gp10b_mmu_new,
  	.secboot = gp10b_secboot_new,

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +1 -0

index 2b6d36ea7067..728d75010847 100644
@@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o
  nvkm-y += nvkm/subdev/ltc/gm200.o
  nvkm-y += nvkm/subdev/ltc/gp100.o
  nvkm-y += nvkm/subdev/ltc/gp102.o
+nvkm-y += nvkm/subdev/ltc/gp10b.o

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file +44 -0

new file mode 100644
index 000000000000..4d27c6ea1552
@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2019 NVIDIA Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Thierry Reding
+ */
+
+#include "priv.h"
+
+static void
+gp10b_ltc_init(struct nvkm_ltc *ltc)
+{
+	struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
+	struct iommu_fwspec *spec;
+#endif
+
+	nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
+	nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
+	nvkm_wr32(device, 0x100800, ltc->ltc_nr);
+
+#ifdef CONFIG_IOMMU_API
+	spec = dev_iommu_fwspec_get(device->dev);
+	if (spec) {
+		u32 sid = spec->ids[0] & 0xffff;
+
+		/* stream ID */

Quoted 8 lines

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c line 47

Quoted 3 lines

drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file +3 -0

+		nvkm_wr32(device, 0x160000, sid << 2);
+	}
+#endif

Could we get rid of the #ifdef blocks here if there was a NULL inline version of dev_iommu_fwspec_get() in the include/linux/iommu.h header? The compiler should then optimise the lot out if the config is not set.

I had thought the same thing and had even typed up the patch to add a dummy for dev_iommu_fwspec_get(), but then when I tested some builds, I got build errors nevertheless because struct iommu_fwspec is only declared under IOMMU_API.

The obvious thing would have been to move struct iommu_fwspec outside of the IOMMU_API protection, but then I remembered having an earlier discussion with Joerg about something similar. I guess the issue here is that we need to reach into struct iommu_fwspec, so it has to be available in full.

Anyway, I can add a patch to do this and remove the IOMMU_API guards, but let's see what Joerg thinks about that first.

Joerg, to summarize: the proposal here is to move the declaration of the iommu_fwspec outside of the IOMMU_API guard and provide a dummy implementation of dev_iommu_fwspec_get() to allow this code to be built without the #ifdef guards. We had discussed something similar about 5 years back and at the time you had been opposed:

	https://lore.kernel.org/linux-iommu/[email protected]/

The case here is slightly different and a lot of time has passed since, so just wanted to ask if your opinion here is the same, or whether you would accept a patch to make this buildable without resorting to #ifdef'ery.

Thierry

Quoted 6 lines

Joerg, to summarize: the proposal here is to move the declaration of the iommu_fwspec outside of the IOMMU_API guard and provide a dummy implementation of dev_iommu_fwspec_get() to allow this code to be built without the #ifdef guards. We had discussed something similar about 5 years back and at the time you had been opposed:

Quoted 6 lines

	https://lore.kernel.org/linux-iommu/[email protected]/

The case here is slightly different and a lot of time has passed since, so just wanted to ask if your opinion here is the same, or whether you would accept a patch to make this buildable without resorting to #ifdef'ery.