Add Tegra241 (Grace) CMDQV Support (part 1/2)

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

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

index c864c634cd23..ba0e24d5ffbf 100644
@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 		 FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
 		 FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+	if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+		return;
+	}
+
 	if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
 		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
 		return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
 					 struct arm_smmu_cmdq *cmdq,
 					 struct arm_smmu_ll_queue *llq)
 {
-	if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+	if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+	    !(cmdq->q.quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY))
 		return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 
 	return __arm_smmu_cmdq_poll_until_consumed(smmu, cmdq, llq);

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

index 180c0b1e0658..01227c0de290 100644

Quoted 42 lines · Nicolin Chen

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

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

index c864c634cd23..ba0e24d5ffbf 100644
@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
               FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
               FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+     if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY) {
+             cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+             return;
+     }
+
      if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
              cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
              return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                       struct arm_smmu_cmdq *cmdq,
                                       struct arm_smmu_ll_queue *llq)
 {
-     if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+     if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+         !(cmdq->q.quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY))
              return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 
      return __arm_smmu_cmdq_poll_until_consumed(smmu, cmdq, llq);

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

index 180c0b1e0658..01227c0de290 100644
@@ -543,6 +543,9 @@ struct arm_smmu_queue {
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_queue { · line 548

Quoted 5 lines

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

      u32 __iomem                     *prod_reg;
      u32 __iomem                     *cons_reg;
+
+#define CMDQ_QUIRK_SYNC_CS_NONE_ONLY BIT(0)  /* CMD_SYNC CS field supports CS_NONE only */
+     u32                             quirks;

Please can you use the existing smmu->options field instead of adding another place to track quirks? Or do you need this only for some of the queues for a given SMMU device?

Hi Will,

Quoted 43 lines · Will Deacon

Quoted 42 lines · Nicolin Chen

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

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

index c864c634cd23..ba0e24d5ffbf 100644
@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
               FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
               FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+     if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY) {
+             cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+             return;
+     }
+
      if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
              cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
              return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                       struct arm_smmu_cmdq *cmdq,
                                       struct arm_smmu_ll_queue *llq)
 {
-     if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+     if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+         !(cmdq->q.quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY))
              return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 
      return __arm_smmu_cmdq_poll_until_consumed(smmu, cmdq, llq);

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

index 180c0b1e0658..01227c0de290 100644
@@ -543,6 +543,9 @@ struct arm_smmu_queue {
 

Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_queue { · line 548

Quoted 5 lines

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

      u32 __iomem                     *prod_reg;
      u32 __iomem                     *cons_reg;
+
+#define CMDQ_QUIRK_SYNC_CS_NONE_ONLY BIT(0)  /* CMD_SYNC CS field supports CS_NONE only */
+     u32                             quirks;

Please can you use the existing smmu->options field instead of adding another place to track quirks? Or do you need this only for some of the queues for a given SMMU device?

VCMDQs are extension of a regular SMMU (with its own CMDQ). So, SMMU CMDQ still supports SIG_IRQ for the CS field, while VCMDQs could only support SIG_NONE. In another word, this quirk is not per SMMU but per Queue.

I can highlight this in the commit message, if that would make it clear.

Quoted 61 lines · Nicolin Chen

Hi Will,

Quoted 43 lines · Will Deacon

Quoted 42 lines · Nicolin Chen

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

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

index c864c634cd23..ba0e24d5ffbf 100644
@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
               FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
               FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+     if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY) {
+             cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+             return;
+     }
+
      if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
              cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
              return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                       struct arm_smmu_cmdq *cmdq,
                                       struct arm_smmu_ll_queue *llq)
 {
-     if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+     if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+         !(cmdq->q.quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY))
              return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 
      return __arm_smmu_cmdq_poll_until_consumed(smmu, cmdq, llq);

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

index 180c0b1e0658..01227c0de290 100644
@@ -543,6 +543,9 @@ struct arm_smmu_queue {
 

Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_queue { · line 548

Quoted 5 lines

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

      u32 __iomem                     *prod_reg;
      u32 __iomem                     *cons_reg;
+
+#define CMDQ_QUIRK_SYNC_CS_NONE_ONLY BIT(0)  /* CMD_SYNC CS field supports CS_NONE only */
+     u32                             quirks;

Please can you use the existing smmu->options field instead of adding another place to track quirks? Or do you need this only for some of the queues for a given SMMU device?

VCMDQs are extension of a regular SMMU (with its own CMDQ). So, SMMU CMDQ still supports SIG_IRQ for the CS field, while VCMDQs could only support SIG_NONE. In another word, this quirk is not per SMMU but per Queue.

Quoted 2 lines

I can highlight this in the commit message, if that would make it clear.

I think we could still use smmu->options and have something like ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY which could be applied when the queue is != arm_smmu_get_cmdq(smmu).

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -2

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 		FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
 		FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+	if (cmdq->type == TEGRA241_VCMDQ) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+		return;
+	}
+
 	if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
 		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
 		return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
 					struct arm_smmu_cmdq *cmdq,
 					struct arm_smmu_ll_queue *llq)
 {
-	if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+	if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+	    cmdq->type != TEGRA241_VCMDQ) {
 		return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 
--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

Quoted 21 lines · Nicolin Chen

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

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 		FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
 		FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+	if (cmdq->type == TEGRA241_VCMDQ) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+		return;
+	}
+
 	if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
 		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
 		return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
 					struct arm_smmu_cmdq *cmdq,
 					struct arm_smmu_ll_queue *llq)
 {
-	if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+	if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+	    cmdq->type != TEGRA241_VCMDQ) {
 		return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 

Quoted 4 lines

(quoted diff) +0 -1

--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

And we would need some additional lines of comments for the two pieces above, explaining why TEGRA241_VCMDQ type needs the first one while bypasses the second one. Again, it feels even worse :(

Quoted 22 lines · Nicolin Chen

Quoted 21 lines · Nicolin Chen

(quoted diff) +7 -1

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
            FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
            FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+   if (cmdq->type == TEGRA241_VCMDQ) {
+           cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+           return;
+   }
+
    if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
            cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
            return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                    struct arm_smmu_cmdq *cmdq,
                                    struct arm_smmu_ll_queue *llq)
 {
-   if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+   if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+       cmdq->type != TEGRA241_VCMDQ) {
            return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 

Quoted 8 lines

Quoted 4 lines

(quoted diff) +0 -1

--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

And we would need some additional lines of comments for the two pieces above, explaining why TEGRA241_VCMDQ type needs the first one while bypasses the second one. Again, it feels even worse :(

I hacked the code around a bit this afternoon. Please can you see if:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-nicolin/grace-vcmdq-wip

does roughly what you need?

Hi Will,

Quoted 32 lines · Will Deacon

Quoted 22 lines · Nicolin Chen

Quoted 21 lines · Nicolin Chen

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

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
            FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
            FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+   if (cmdq->type == TEGRA241_VCMDQ) {
+           cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+           return;
+   }
+
    if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
            cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
            return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                    struct arm_smmu_cmdq *cmdq,
                                    struct arm_smmu_ll_queue *llq)
 {
-   if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+   if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+       cmdq->type != TEGRA241_VCMDQ) {
            return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 

Quoted 8 lines

Quoted 4 lines

(quoted diff) +0 -1

--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

And we would need some additional lines of comments for the two pieces above, explaining why TEGRA241_VCMDQ type needs the first one while bypasses the second one. Again, it feels even worse :(

Quoted 5 lines

I hacked the code around a bit this afternoon. Please can you see if:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-nicolin/grace-vcmdq-wip

does roughly what you need?

I appreciate the patch. Yet, we cannot use IORT's model field. This would need to go through IORT documentation, for A. And B, we had a very long discussion with ARM (Robin was there) years ago, and concluded that this CMDQV would not be a model in IORT but a DSDT node as an extension. So, this is firm...

With that, we cannot avoid an unconditional hard-coding tegra function call even if we switch to an impl design:

(quoted diff) +11 -0

+static int acpi_smmu_impl_init(u32 model, struct arm_smmu_device *smmu)
+{
+	/*
+	 * unconditional go through ACPI table to detect if there is a tegra241
+	 * implementation that extends SMMU with a CMDQV. The probe() will fill
+	 * the smmu->impl pointer upon success. Otherwise, fall back to regular
+	 * SMMU CMDQ.
+	 */
+	tegra241_impl_acpi_probe(smmu);
+	return 0;
+}

As for arm_smmu_cmdq_needs_busy_polling, it doesn't really look very optimal to me. But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

Quoted 60 lines · Nicolin Chen

Quoted 32 lines · Will Deacon

Quoted 22 lines · Nicolin Chen

Quoted 21 lines · Nicolin Chen

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

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
            FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
            FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+   if (cmdq->type == TEGRA241_VCMDQ) {
+           cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+           return;
+   }
+
    if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
            cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
            return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                    struct arm_smmu_cmdq *cmdq,
                                    struct arm_smmu_ll_queue *llq)
 {
-   if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+   if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+       cmdq->type != TEGRA241_VCMDQ) {
            return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 

Quoted 8 lines

Quoted 4 lines

(quoted diff) +0 -1

--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

And we would need some additional lines of comments for the two pieces above, explaining why TEGRA241_VCMDQ type needs the first one while bypasses the second one. Again, it feels even worse :(

Quoted 5 lines

I hacked the code around a bit this afternoon. Please can you see if:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-nicolin/grace-vcmdq-wip

does roughly what you need?

I appreciate the patch. Yet, we cannot use IORT's model field. This would need to go through IORT documentation, for A. And B, we had a very long discussion with ARM (Robin was there) years ago, and concluded that this CMDQV would not be a model in IORT but a DSDT node as an extension. So, this is firm...

With that, we cannot avoid an unconditional hard-coding tegra function call even if we switch to an impl design:

(quoted diff) +11 -0

+static int acpi_smmu_impl_init(u32 model, struct arm_smmu_device *smmu)
+{
+	/*
+	 * unconditional go through ACPI table to detect if there is a tegra241
+	 * implementation that extends SMMU with a CMDQV. The probe() will fill
+	 * the smmu->impl pointer upon success. Otherwise, fall back to regular
+	 * SMMU CMDQ.
+	 */
+	tegra241_impl_acpi_probe(smmu);
+	return 0;
+}

Quoted 5 lines

As for arm_smmu_cmdq_needs_busy_polling, it doesn't really look very optimal to me. But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

Please ignore the attachment. Since we are adding arm_smmu_impl, I figure that we could add an arm_smmu_cmdq_impl too. There's an another small feature that I didn't implement in this v9, while being able to benefit from a cmdq impl now.

The impl can also hold a boolean busy_polling, so we won't need a global smmu option.

I will send a new version asap, though I am not sure if we can still make it to this cycle that we hoped for :-/

Quoted 11 lines

Quoted 5 lines

As for arm_smmu_cmdq_needs_busy_polling, it doesn't really look very optimal to me. But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

Please ignore the attachment. Since we are adding arm_smmu_impl, I figure that we could add an arm_smmu_cmdq_impl too. There's an another small feature that I didn't implement in this v9, while being able to benefit from a cmdq impl now.

Quoted 2 lines

The impl can also hold a boolean busy_polling, so we won't need a global smmu option.

So /that/ might be overkill. Architectural queues can use polling, so I don't mind having that option in the driver and it should keep the number of impl hooks to a minimum.

Quoted 2 lines

I will send a new version asap, though I am not sure if we can still make it to this cycle that we hoped for :-/

I'm in fixes-only mode at this point, especially since we've not had a linux-next for a while.

Quoted 36 lines · Will Deacon

Quoted 22 lines · Nicolin Chen

Quoted 21 lines · Nicolin Chen

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

@@ -345,6 +345,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
            FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
            FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
 
+   if (cmdq->type == TEGRA241_VCMDQ) {
+           cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+           return;
+   }
+
    if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
            cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
            return;

@@ -690,7 +695,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
                                    struct arm_smmu_cmdq *cmdq,
                                    struct arm_smmu_ll_queue *llq)
 {
-   if (smmu->options & ARM_SMMU_OPT_MSIPOLL)
+   if (smmu->options & ARM_SMMU_OPT_MSIPOLL &&
+       cmdq->type != TEGRA241_VCMDQ) {
            return __arm_smmu_cmdq_poll_until_msi(smmu, cmdq, llq);
 

Quoted 8 lines

Quoted 4 lines

(quoted diff) +0 -1

--------------------------------------------------------------

Would you prefer this one? I feel CMDQ_QUIRK_SYNC_CS_NONE_ONLY is more general looking though..

And we would need some additional lines of comments for the two pieces above, explaining why TEGRA241_VCMDQ type needs the first one while bypasses the second one. Again, it feels even worse :(

I hacked the code around a bit this afternoon. Please can you see if:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-nicolin/grace-vcmdq-wip

Quoted 1 line

does roughly what you need?

I appreciate the patch. Yet, we cannot use IORT's model field. This would need to go through IORT documentation, for A. And B, we had a very long discussion with ARM (Robin was there) years ago, and concluded that this CMDQV would not be a model in IORT but a DSDT node as an extension. So, this is firm...

(quoted diff) +2 -0

+	return 0;
+}

As for arm_smmu_cmdq_needs_busy_polling, it doesn't really look very optimal to me.

But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

Quoted 3 lines · Nicolin Chen

But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

With my hacks, I think you can just call arm_smmu_cmdq_build_sync_cmd() from the irqhandler and it will work.

External email: Use caution opening links or attachments

Quoted 5 lines · Will Deacon

Quoted 3 lines · Nicolin Chen

But if you insist on having an smmu option, we still have to take in the PATCH-3 in this series, enforcing an arm_smmu_cmdq_build_sync_cmd() call in the IRQ handler too. So, it would eventually look like [attachment].

Quoted 2 lines

With my hacks, I think you can just call arm_smmu_cmdq_build_sync_cmd() from the irqhandler and it will work.

Hmm, actually, that will mean we end up using MSIs for the error case on hardware which supports it, which is a strange change in behaviour.

What does your hardware do if it sees SIG_SEV in a CMD_SYNC? Is it just a case of failing to generate the event on completion, or does it treat it as an invalid opcode?

Review

Quoted 12 lines

With that, we cannot avoid an unconditional hard-coding tegra function call even if we switch to an impl design:

(quoted diff) +9 -0

+static int acpi_smmu_impl_init(u32 model, struct arm_smmu_device *smmu)
+{
+     /*
+      * unconditional go through ACPI table to detect if there is a tegra241
+      * implementation that extends SMMU with a CMDQV. The probe() will fill
+      * the smmu->impl pointer upon success. Otherwise, fall back to regular
+      * SMMU CMDQ.
+      */
+     tegra241_impl_acpi_probe(smmu);

In-line the minimal DSDT parsing to figure out if we're on a Tegra part. If it's that bad, put it in a static inline in arm-smmu-v3.h.

Quoted 5 lines

(quoted diff) +2 -0

+     return 0;
+}

As for arm_smmu_cmdq_needs_busy_polling, it doesn't really look very optimal to me.

"optimal" in what sense? In that you don't like how it smells, or that it's measurably bad?

Quoted 15 lines · Will Deacon

Review

Quoted 12 lines

With that, we cannot avoid an unconditional hard-coding tegra function call even if we switch to an impl design:

(quoted diff) +9 -0

+static int acpi_smmu_impl_init(u32 model, struct arm_smmu_device *smmu)
+{
+     /*
+      * unconditional go through ACPI table to detect if there is a tegra241
+      * implementation that extends SMMU with a CMDQV. The probe() will fill
+      * the smmu->impl pointer upon success. Otherwise, fall back to regular
+      * SMMU CMDQ.
+      */
+     tegra241_impl_acpi_probe(smmu);

In-line the minimal DSDT parsing to figure out if we're on a Tegra part. If it's that bad, put it in a static inline in arm-smmu-v3.h.

OK. How about the following?

/* arm-smmu-v3.h */
static inline void arm_smmu_impl_acpi_dsdt_probe(struct arm_smmu_device *smmu,
struct acpi_iort_node *node)

{
	tegra241_cmdqv_acpi_dsdt_probe(smmu, node);
}

/* arm-smmu-v3.c */
static int arm_smmu_impl_acpi_probe(struct arm_smmu_device *smmu,
struct acpi_iort_node *node)

{
	/*
	 * DSDT might holds some SMMU extension, so we have no option but to go
	 * through ACPI tables unconditionally. This probe function should fill
	 * the smmu->impl pointer upon success. Otherwise, just carry on with a
	 * standard SMMU.
	 */
	arm_smmu_impl_acpi_dsdt_probe(smmu, node);

	return 0;
}

From: Nate Watterson <[email protected]>

NVIDIA's Tegra241 Soc has a CMDQ-Virtualization (CMDQV) hardware, extending the standard ARM SMMU v3 IP to support multiple VCMDQs with virtualization capabilities. In terms of command queue, they are very like a standard SMMU CMDQ (or ECMDQs), but only support CS_NONE in the CS field of CMD_SYNC.

Add a new tegra241-cmdqv driver, and insert its structure pointer into the existing arm_smmu_device, and then add related function calls in the SMMUv3 driver to interact with the CMDQV driver.

In the CMDQV driver, add a minimal part for the in-kernel support: reserve VINTF0 for in-kernel use, and assign some of the VCMDQs to the VINTF0, and select one VCMDQ based on the current CPU ID to execute supported commands. This multi-queue design for in-kernel use gives some limited improvements:
up to 20% reduction of invalidation time was measured by a multi-threaded DMA unmap benchmark, compared to a single queue.

The other part of the CMDQV driver will be user-space support that gives a hypervisor running on the host OS to talk to the driver for virtualization use cases, allowing VMs to use VCMDQs without trappings, i.e. no VM Exits. This is designed based on IOMMUFD, and its RFC series is also under review. It will provide a guest OS a bigger improvement: 70% to 90% reductions of TLB invalidation time were measured by DMA unmap tests running in a guest, compared to nested SMMU CMDQ (with trappings).

However, it is very important for this in-kernel support to get merged and installed to VMs running on Grace-powered servers as soon as possible. So, later those servers would only need to upgrade their host kernels for the user-space support.

As the initial version, the CMDQV driver only supports ACPI configurations.

Signed-off-by Nate Watterson <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Co-developed-by Nicolin Chen <[email protected]>

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

---
 MAINTAINERS                                   |   1 +
 drivers/iommu/Kconfig                         |  11 +
 drivers/iommu/arm/arm-smmu-v3/Makefile        |   1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  52 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  50 ++
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 842 ++++++++++++++++++
 6 files changed, 945 insertions(+), 12 deletions(-)
 create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c

MAINTAINERS +1 -0

index aacccb376c28..ecf7af1b2df8 100644
@@ -22078,6 +22078,7 @@ M:	Thierry Reding <[email protected]>
 R:	Krishna Reddy <[email protected]>
 L:	[email protected]
 S:	Supported
+F:	drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
 F:	drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
 F:	drivers/iommu/tegra*
 

drivers/iommu/Kconfig +11 -0

index c04584be3089..e009387d3cba 100644
@@ -423,6 +423,17 @@ config ARM_SMMU_V3_KUNIT_TEST
 	  Enable this option to unit-test arm-smmu-v3 driver functions.
 
 	  If unsure, say N.
+
+config TEGRA241_CMDQV
+	bool "NVIDIA Tegra241 CMDQ-V extension support for ARM SMMUv3"
+	depends on ACPI
+	help
+	  Support for NVIDIA CMDQ-Virtualization extension for ARM SMMUv3. The
+	  CMDQ-V extension is similar to v3.3 ECMDQ for multi command queues
+	  support, except with virtualization capabilities.
+
+	  Say Y here if your system is NVIDIA Tegra241 (Grace) or it has the same
+	  CMDQ-V extension.
 endif
 
 config S390_IOMMU

drivers/iommu/arm/arm-smmu-v3/Makefile +1 -0

index 014a997753a8..55201fdd7007 100644
@@ -2,6 +2,7 @@
 obj-$(CONFIG_ARM_SMMU_V3) += arm_smmu_v3.o
 arm_smmu_v3-objs-y += arm-smmu-v3.o
 arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_SVA) += arm-smmu-v3-sva.o
+arm_smmu_v3-objs-$(CONFIG_TEGRA241_CMDQV) += tegra241-cmdqv.o
 arm_smmu_v3-objs := $(arm_smmu_v3-objs-y)
 
 obj-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

index ba0e24d5ffbf..430e84fe3679 100644

Quoted 26 lines · Nicolin Chen

From: Nate Watterson <[email protected]>

NVIDIA's Tegra241 Soc has a CMDQ-Virtualization (CMDQV) hardware, extending the standard ARM SMMU v3 IP to support multiple VCMDQs with virtualization capabilities. In terms of command queue, they are very like a standard SMMU CMDQ (or ECMDQs), but only support CS_NONE in the CS field of CMD_SYNC.

Add a new tegra241-cmdqv driver, and insert its structure pointer into the existing arm_smmu_device, and then add related function calls in the SMMUv3 driver to interact with the CMDQV driver.

In the CMDQV driver, add a minimal part for the in-kernel support: reserve VINTF0 for in-kernel use, and assign some of the VCMDQs to the VINTF0, and select one VCMDQ based on the current CPU ID to execute supported commands. This multi-queue design for in-kernel use gives some limited improvements:
up to 20% reduction of invalidation time was measured by a multi-threaded DMA unmap benchmark, compared to a single queue.

The other part of the CMDQV driver will be user-space support that gives a hypervisor running on the host OS to talk to the driver for virtualization use cases, allowing VMs to use VCMDQs without trappings, i.e. no VM Exits. This is designed based on IOMMUFD, and its RFC series is also under review. It will provide a guest OS a bigger improvement: 70% to 90% reductions of TLB invalidation time were measured by DMA unmap tests running in a guest, compared to nested SMMU CMDQ (with trappings).

Quoted 4 lines

However, it is very important for this in-kernel support to get merged and installed to VMs running on Grace-powered servers as soon as possible. So, later those servers would only need to upgrade their host kernels for the user-space support.

^^^ This is a weird paragraph to put in the commit message.

Quoted 67 lines

As the initial version, the CMDQV driver only supports ACPI configurations.

Signed-off-by Nate Watterson <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Co-developed-by Nicolin Chen <[email protected]>

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

---
 MAINTAINERS                                   |   1 +
 drivers/iommu/Kconfig                         |  11 +
 drivers/iommu/arm/arm-smmu-v3/Makefile        |   1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  52 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  50 ++
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 842 ++++++++++++++++++
 6 files changed, 945 insertions(+), 12 deletions(-)
 create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c

MAINTAINERS +1 -0

index aacccb376c28..ecf7af1b2df8 100644
@@ -22078,6 +22078,7 @@ M:    Thierry Reding <[email protected]>
 R:   Krishna Reddy <[email protected]>
 L:   [email protected]
 S:   Supported
+F:   drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
 F:   drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
 F:   drivers/iommu/tegra*
 

drivers/iommu/Kconfig +11 -0

index c04584be3089..e009387d3cba 100644
@@ -423,6 +423,17 @@ config ARM_SMMU_V3_KUNIT_TEST
        Enable this option to unit-test arm-smmu-v3 driver functions.
 
        If unsure, say N.
+
+config TEGRA241_CMDQV
+     bool "NVIDIA Tegra241 CMDQ-V extension support for ARM SMMUv3"
+     depends on ACPI
+     help
+       Support for NVIDIA CMDQ-Virtualization extension for ARM SMMUv3. The
+       CMDQ-V extension is similar to v3.3 ECMDQ for multi command queues
+       support, except with virtualization capabilities.
+
+       Say Y here if your system is NVIDIA Tegra241 (Grace) or it has the same
+       CMDQ-V extension.
 endif
 
 config S390_IOMMU

drivers/iommu/arm/arm-smmu-v3/Makefile +1 -0

index 014a997753a8..55201fdd7007 100644
@@ -2,6 +2,7 @@
 obj-$(CONFIG_ARM_SMMU_V3) += arm_smmu_v3.o
 arm_smmu_v3-objs-y += arm-smmu-v3.o
 arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_SVA) += arm-smmu-v3-sva.o
+arm_smmu_v3-objs-$(CONFIG_TEGRA241_CMDQV) += tegra241-cmdqv.o
 arm_smmu_v3-objs := $(arm_smmu_v3-objs-y)
 
 obj-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

index ba0e24d5ffbf..430e84fe3679 100644

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 340

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0

@@ -334,6 +334,9 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 
 static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
 {
+     if (arm_smmu_has_tegra241_cmdqv(smmu))
+             return tegra241_cmdqv_get_cmdq(smmu);
+
      return &smmu->cmdq;

Hardcoding all these tegra-specific checks in the core driver is pretty horrible :/

Instead, please can we do something similar to the SMMUv2 driver? That is, tweak the probe routine to call something akin to the arm_smmu_impl_init() function, which looks at the 'model' field pulled out of the IORT and can then dispatch directly to a tegra-specific init function (see, e.g. nvidia_smmu_impl_init() for SMMUv2).

From there, you can both install function pointers into the 'arm_smmu_device' structure which can be used instead of having the the 'if (tegra)' checks in the main driver and you can also re-allocate the structu to live inside a private structure instead of having the backpointer.

Maybe those cmdq function pointers would be happy for other extensions too (e.g. the ECMDQ stuff at [1]).

Will

[1] https://lore.kernel.org/r/[email protected]

NVIDIA's Tegra241 (Grace) SoC has a CMDQ-Virtualization (CMDQV) hardware that extends standard ARM SMMUv3 to support multiple command queues with virtualization capabilities. Though this is similar to the ECMDQ in SMMU v3.3, CMDQV provides additional Virtual Interfaces (VINTFs) allowing VMs to have their own VINTFs and Virtual Command Queues (VCMDQs). The VCMDQs can only execute a limited set of commands, mainly invalidation commands when exclusively used by the VMs, compared to the standard SMMUv3 CMDQ.

Hi!

Quoted 7 lines

NVIDIA's Tegra241 (Grace) SoC has a CMDQ-Virtualization (CMDQV) hardware that extends standard ARM SMMUv3 to support multiple command queues with virtualization capabilities. Though this is similar to the ECMDQ in SMMU v3.3, CMDQV provides additional Virtual Interfaces (VINTFs) allowing VMs to have their own VINTFs and Virtual Command Queues (VCMDQs). The VCMDQs can only execute a limited set of commands, mainly invalidation commands when exclusively used by the VMs, compared to the standard SMMUv3 CMDQ.

Text is block-aligned without duplicated spaces. How did you do that...?