iommu/tegra241-cmdqv: Fix two bugs in __tegra241_cmdqv_probe

PATCH Applied Nicolin Chen 4 participants 7 messages Sep 5, 2024

Subsystems (4)

Tegra architecture support IOMMU Arm SMMU drivers Tegra IOMMU drivers

lore

Share & tools

lore

Nicolin Chen

author

2024-09-05 02:41 UTC #0 lore raw

cover iommu/tegra241-cmdqv: Fix two bugs in __tegra241_cmdqv_probe

Found two more bugs today. And here are the fixes.

Based on Will's for-joerg/arm-smmu/updates branch.

Available on Github:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-fix

Cc Matt Ochs <[email protected]>

Thanks
Nicolin

Nicolin Chen (2):
iommu/tegra241-cmdqv: Drop static at local variable iommu/tegra241-cmdqv: Do not allocate vcmdq until
dma_set_mask_and_coherent

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  9 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  1 +
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 85 ++++++++++++-------
 3 files changed, 61 insertions(+), 34 deletions(-)

Signature · 1 line

2.43.0

Nicolin Chen

author

2024-09-05 02:41 UTC #1 lore raw

PATCH 1/2 iommu/tegra241-cmdqv: Drop static at local variable

This is likely a typo. Drop it.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +1 -1

index 240b54192177..0766dc2789cb 100644
@@ -761,8 +761,8 @@ static struct arm_smmu_device *
 __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 		       int irq)
 {
-	static struct arm_smmu_device *new_smmu;
 	struct tegra241_cmdqv *cmdqv = NULL;
+	struct arm_smmu_device *new_smmu;
 	struct tegra241_vintf *vintf;
 	void __iomem *base;
 	u32 regval;

Signature · 1 line

2.43.0

Jason Gunthorpe

Reviewed-by on v1 1/2

2024-09-05 12:30 UTC #2 lore raw

Re: [PATCH next 1/2] iommu/tegra241-cmdqv: Drop static at local variable

Quoted 7 lines · Nicolin Chen

This is likely a typo. Drop it.
Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Nicolin Chen

author

2024-09-05 02:41 UTC #3 lore raw

PATCH 2/2 iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent

In reply to Nicolin Chen

It's observed that, when the first 4GB of system memory was reserved, all VCMDQ allocations failed (even with the smallest qsz in the last attempt):
arm-smmu-v3: found companion CMDQV device: NVDA200C:00
arm-smmu-v3: option mask 0x10
arm-smmu-v3: failed to allocate queue (0x8000 bytes) for vcmdq0 acpi NVDA200C:00: tegra241_cmdqv: Falling back to standard SMMU CMDQ
arm-smmu-v3: ias 48-bit, oas 48-bit (features 0x001e1fbf)
arm-smmu-v3: allocated 524288 entries for cmdq
arm-smmu-v3: allocated 524288 entries for evtq
arm-smmu-v3: allocated 524288 entries for priq

This is because the 4GB reserved memory shifted the entire DMA zone from a lower 32-bit range (on a system without the 4GB carveout) to higher range, while the dev->coherent_dma_mask was set to DMA_BIT_MASK(32) by default.

The dma_set_mask_and_coherent() call is done in arm_smmu_device_hw_probe() of the SMMU driver. So any DMA allocation from tegra241_cmdqv_probe() must wait until the coherent_dma_mask is correctly set.

Move the vintf/vcmdq structure initialization routine into a different op, "init_structures". Call it at the end of arm_smmu_init_structures(), where standard SMMU queues get allocated.

Most of the impl_ops aren't ready until vintf/vcmdq structure are init-ed. So replace the full impl_ops with an init_ops in __tegra241_cmdqv_probe().

And switch to tegra241_cmdqv_impl_ops later in arm_smmu_init_structures(). Note that tegra241_cmdqv_impl_ops does not link to the new init_structures op after this switch, since there is no point in having it once it's done.

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")

Reported-by Matt Ochs <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  9 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  1 +
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 83 ++++++++++++-------
 3 files changed, 60 insertions(+), 33 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -1

index b2de56dfceb9..df852ab04fd7 100644
@@ -3744,7 +3744,14 @@ static int arm_smmu_init_structures(struct arm_smmu_device *smmu)
 	if (ret)
 		return ret;
 
-	return arm_smmu_init_strtab(smmu);
+	ret = arm_smmu_init_strtab(smmu);
+	if (ret)
+		return ret;
+
+	if (smmu->impl_ops && smmu->impl_ops->init_structures)
+		return smmu->impl_ops->init_structures(smmu);
+
+	return 0;
 }
 
 static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0

index e044ce5b5372..e8320e9341d7 100644
@@ -643,6 +643,7 @@ struct arm_smmu_strtab_cfg {
 struct arm_smmu_impl_ops {
 	int (*device_reset)(struct arm_smmu_device *smmu);
 	void (*device_remove)(struct arm_smmu_device *smmu);
+	int (*init_structures)(struct arm_smmu_device *smmu);
 	struct arm_smmu_cmdq *(*get_secondary_cmdq)(
 		struct arm_smmu_device *smmu, struct arm_smmu_cmdq_ent *ent);
 };

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +51 ~~-32~~

114 lines not on this page · raw message

Signature · 1 line

2.43.0

Jason Gunthorpe

Reviewed-by on v1 2/2

2024-09-05 13:18 UTC #4 lore raw

Re: [PATCH next 2/2] iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent

Quoted 30 lines · Nicolin Chen

It's observed that, when the first 4GB of system memory was reserved, all VCMDQ allocations failed (even with the smallest qsz in the last attempt):
arm-smmu-v3: found companion CMDQV device: NVDA200C:00
arm-smmu-v3: option mask 0x10
arm-smmu-v3: failed to allocate queue (0x8000 bytes) for vcmdq0 acpi NVDA200C:00: tegra241_cmdqv: Falling back to standard SMMU CMDQ
arm-smmu-v3: ias 48-bit, oas 48-bit (features 0x001e1fbf)
arm-smmu-v3: allocated 524288 entries for cmdq
arm-smmu-v3: allocated 524288 entries for evtq
arm-smmu-v3: allocated 524288 entries for priq
This is because the 4GB reserved memory shifted the entire DMA zone from a lower 32-bit range (on a system without the 4GB carveout) to higher range, while the dev->coherent_dma_mask was set to DMA_BIT_MASK(32) by default.
The dma_set_mask_and_coherent() call is done in arm_smmu_device_hw_probe() of the SMMU driver. So any DMA allocation from tegra241_cmdqv_probe() must wait until the coherent_dma_mask is correctly set.
Move the vintf/vcmdq structure initialization routine into a different op, "init_structures". Call it at the end of arm_smmu_init_structures(), where standard SMMU queues get allocated.
Most of the impl_ops aren't ready until vintf/vcmdq structure are init-ed. So replace the full impl_ops with an init_ops in __tegra241_cmdqv_probe().
And switch to tegra241_cmdqv_impl_ops later in arm_smmu_init_structures(). Note that tegra241_cmdqv_impl_ops does not link to the new init_structures op after this switch, since there is no point in having it once it's done.

Quoted 8 lines

Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Reported-by Matt Ochs <[email protected]>
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  9 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  1 +
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 83 ++++++++++++-------
 3 files changed, 60 insertions(+), 33 deletions(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Matt Ochs

Tested-by on v1 2/2

2024-09-05 16:26 UTC #5 lore raw

Re: [PATCH next 2/2] iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent

In reply to Nicolin Chen

Quoted 28 lines · Nicolin Chen

It's observed that, when the first 4GB of system memory was reserved, all VCMDQ allocations failed (even with the smallest qsz in the last attempt):
arm-smmu-v3: found companion CMDQV device: NVDA200C:00
arm-smmu-v3: option mask 0x10
arm-smmu-v3: failed to allocate queue (0x8000 bytes) for vcmdq0 acpi NVDA200C:00: tegra241_cmdqv: Falling back to standard SMMU CMDQ
arm-smmu-v3: ias 48-bit, oas 48-bit (features 0x001e1fbf)
arm-smmu-v3: allocated 524288 entries for cmdq
arm-smmu-v3: allocated 524288 entries for evtq
arm-smmu-v3: allocated 524288 entries for priq
This is because the 4GB reserved memory shifted the entire DMA zone from a lower 32-bit range (on a system without the 4GB carveout) to higher range, while the dev->coherent_dma_mask was set to DMA_BIT_MASK(32) by default.
The dma_set_mask_and_coherent() call is done in arm_smmu_device_hw_probe() of the SMMU driver. So any DMA allocation from tegra241_cmdqv_probe() must wait until the coherent_dma_mask is correctly set.
Move the vintf/vcmdq structure initialization routine into a different op, "init_structures". Call it at the end of arm_smmu_init_structures(), where standard SMMU queues get allocated.
Most of the impl_ops aren't ready until vintf/vcmdq structure are init-ed. So replace the full impl_ops with an init_ops in __tegra241_cmdqv_probe().

Quoted 8 lines

And switch to tegra241_cmdqv_impl_ops later in arm_smmu_init_structures(). Note that tegra241_cmdqv_impl_ops does not link to the new init_structures op after this switch, since there is no point in having it once it's done.
Fixes 918eb5c856f6 ("iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV")
Reported-by Matt Ochs <[email protected]>
Signed-off-by Nicolin Chen <[email protected]>
—

Tested-by Matt Ochs <[email protected]>

Will Deacon

maintainer Applied

2024-09-05 16:18 UTC #6 lore raw

In reply to Nicolin Chen

Quoted 8 lines · Nicolin Chen

Found two more bugs today. And here are the fixes.
Based on Will's for-joerg/arm-smmu/updates branch.
Available on Github:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-fix
[...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/2] iommu/tegra241-cmdqv: Drop static at local variable

      https://git.kernel.org/will/c/2408b81f817b
[2/2] iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent
      https://git.kernel.org/will/c/483e0bd8883a

Cheers,

Signature · 5 lines

Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev