diff mbox series

[v11,9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

Message ID 153fb887cf4bf6318c6f313a4be9b40a25a24e7d.1722993435.git.nicolinc@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Add Tegra241 (Grace) CMDQV Support (part 1/2) | expand

Commit Message

Nicolin Chen Aug. 7, 2024, 2:11 a.m. UTC 
When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset),
only TLB and ATC invalidation commands are supported by the VCMDQ HW. So,
add a new helper to scan the input cmd to make sure it is supported when
selecting a queue, though this assumes that SMMUv3 driver will only add
the same type of commands into an arm_smmu_cmdq_batch as it does today.

Note that the guest VM shouldn't have HYP_OWN bit being set regardless of
guest kernel driver writing it or not, i.e. the hypervisor running in the
host OS should wire this bit to zero when trapping a write access to this
VINTF_CONFIG register from a guest kernel.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 22 +++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  3 +-
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 35 ++++++++++++++++++-
 3 files changed, 49 insertions(+), 11 deletions(-)

Comments

Will Deacon Aug. 16, 2024, 1:21 p.m. UTC | #1 
On Tue, Aug 06, 2024 at 07:11:54PM -0700, Nicolin Chen wrote:
> When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset),
> only TLB and ATC invalidation commands are supported by the VCMDQ HW. So,
> add a new helper to scan the input cmd to make sure it is supported when
> selecting a queue, though this assumes that SMMUv3 driver will only add
> the same type of commands into an arm_smmu_cmdq_batch as it does today.
> 
> Note that the guest VM shouldn't have HYP_OWN bit being set regardless of
> guest kernel driver writing it or not, i.e. the hypervisor running in the
> host OS should wire this bit to zero when trapping a write access to this
> VINTF_CONFIG register from a guest kernel.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 22 +++++++-----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  3 +-
>  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 35 ++++++++++++++++++-
>  3 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 18d940c65e2c..8ff8e264d5e7 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -336,12 +336,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>  	return 0;
>  }
>  
> -static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
> +static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
> +					       u8 opcode)
>  {
>  	struct arm_smmu_cmdq *cmdq = NULL;
>  
>  	if (smmu->impl && smmu->impl->get_secondary_cmdq)
> -		cmdq = smmu->impl->get_secondary_cmdq(smmu);
> +		cmdq = smmu->impl->get_secondary_cmdq(smmu, opcode);
>  
>  	return cmdq ?: &smmu->cmdq;
>  }
> @@ -889,7 +890,7 @@ static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
>  	}
>  
>  	return arm_smmu_cmdq_issue_cmdlist(
> -		smmu, arm_smmu_get_cmdq(smmu), cmd, 1, sync);
> +		smmu, arm_smmu_get_cmdq(smmu, ent->opcode), cmd, 1, sync);
>  }
>  
>  static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
> @@ -905,10 +906,13 @@ static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
>  }
>  
>  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> -				     struct arm_smmu_cmdq_batch *cmds)
> +				     struct arm_smmu_cmdq_batch *cmds,
> +				     u8 opcode)
>  {
> +	WARN_ON_ONCE(!opcode);

This seems like a fairly arbitrary warning. Remove it?

> +
>  	cmds->num = 0;
> -	cmds->cmdq = arm_smmu_get_cmdq(smmu);
> +	cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);

If we stashed the opcode here, we could actually just enforce that all
commands in the batch are the same type in arm_smmu_cmdq_batch_add().

Would that work better for you or not?

Will
Nicolin Chen Aug. 16, 2024, 5:34 p.m. UTC | #2 
On Fri, Aug 16, 2024 at 02:21:03PM +0100, Will Deacon wrote:

> >  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> > -                                  struct arm_smmu_cmdq_batch *cmds)
> > +                                  struct arm_smmu_cmdq_batch *cmds,
> > +                                  u8 opcode)
> >  {
> > +     WARN_ON_ONCE(!opcode);
> 
> This seems like a fairly arbitrary warning. Remove it?

OK.

> > +
> >       cmds->num = 0;
> > -     cmds->cmdq = arm_smmu_get_cmdq(smmu);
> > +     cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);
> 
> If we stashed the opcode here, we could actually just enforce that all
> commands in the batch are the same type in arm_smmu_cmdq_batch_add().
> 
> Would that work better for you or not?

A guested-owned queue is okay to mix different command types:
	CMDQ_OP_TLBI_NH_ASID
	CMDQ_OP_TLBI_NH_VA
	CMDQ_OP_ATC_INV

So, limiting a batch to one single opcode isn't ideal. Instead,
if we really have to apply an enforcement to every batch_add(),
I think the cmdq structure would need a scan function pointer:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d0d7c75c030a..1a83ad5ebadc 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
 
+static bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
+				       struct arm_smmu_cmdq_ent *ent)
+{
+	if (!cmdq->supports_cmd)
+		return true;
+	return cmdq->supports_cmd(ent);
+}
+
 static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
@@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
 
-	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
-	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) {
+	if ((cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
+	     (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) ||
+	    !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd)) {
 		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index e131d8170b90..c4872af6232c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -616,2 +616,3 @@ struct arm_smmu_cmdq {
 	atomic_t			lock;
+	bool                            (*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
 };

That being said, the whole thing doesn't seem to have a lot value
at this moment, since the SMMU driver doesn't mix commands?

Thanks
Nicolin
Nicolin Chen Aug. 16, 2024, 6:15 p.m. UTC | #3 
On Fri, Aug 16, 2024 at 10:34:24AM -0700, Nicolin Chen wrote:
> On Fri, Aug 16, 2024 at 02:21:03PM +0100, Will Deacon wrote:
> 
> > >  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> > > -                                  struct arm_smmu_cmdq_batch *cmds)
> > > +                                  struct arm_smmu_cmdq_batch *cmds,
> > > +                                  u8 opcode)
> > >  {
> > > +     WARN_ON_ONCE(!opcode);
> > 
> > This seems like a fairly arbitrary warning. Remove it?
> 
> OK.
> 
> > > +
> > >       cmds->num = 0;
> > > -     cmds->cmdq = arm_smmu_get_cmdq(smmu);
> > > +     cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);
> > 
> > If we stashed the opcode here, we could actually just enforce that all
> > commands in the batch are the same type in arm_smmu_cmdq_batch_add().
> > 
> > Would that work better for you or not?
> 
> A guested-owned queue is okay to mix different command types:
> 	CMDQ_OP_TLBI_NH_ASID
> 	CMDQ_OP_TLBI_NH_VA
> 	CMDQ_OP_ATC_INV
> 
> So, limiting a batch to one single opcode isn't ideal. Instead,
> if we really have to apply an enforcement to every batch_add(),
> I think the cmdq structure would need a scan function pointer:
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index d0d7c75c030a..1a83ad5ebadc 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
>  
> +static bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
> +				       struct arm_smmu_cmdq_ent *ent)
> +{
> +	if (!cmdq->supports_cmd)
> +		return true;
> +	return cmdq->supports_cmd(ent);
> +}
> +
>  static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
> @@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
>  
> -	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> -	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) {
> +	if ((cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> +	     (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) ||
> +	    !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd)) {
>  		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,

We'd need re-init the batch after this too..

Nicolin

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index e131d8170b90..c4872af6232c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -616,2 +616,3 @@ struct arm_smmu_cmdq {
>  	atomic_t			lock;
> +	bool                            (*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
>  };
> 
> That being said, the whole thing doesn't seem to have a lot value
> at this moment, since the SMMU driver doesn't mix commands?
> 
> Thanks
> Nicolin
Nicolin Chen Aug. 16, 2024, 7:42 p.m. UTC | #4 
On Fri, Aug 16, 2024 at 11:15:31AM -0700, Nicolin Chen wrote:
> On Fri, Aug 16, 2024 at 10:34:24AM -0700, Nicolin Chen wrote:
> > On Fri, Aug 16, 2024 at 02:21:03PM +0100, Will Deacon wrote:
> > 
> > > >  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> > > > -                                  struct arm_smmu_cmdq_batch *cmds)
> > > > +                                  struct arm_smmu_cmdq_batch *cmds,
> > > > +                                  u8 opcode)
> > > >  {
> > > > +     WARN_ON_ONCE(!opcode);
> > > 
> > > This seems like a fairly arbitrary warning. Remove it?
> > 
> > OK.
> > 
> > > > +
> > > >       cmds->num = 0;
> > > > -     cmds->cmdq = arm_smmu_get_cmdq(smmu);
> > > > +     cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);
> > > 
> > > If we stashed the opcode here, we could actually just enforce that all
> > > commands in the batch are the same type in arm_smmu_cmdq_batch_add().
> > > 
> > > Would that work better for you or not?
> > 
> > A guested-owned queue is okay to mix different command types:
> > 	CMDQ_OP_TLBI_NH_ASID
> > 	CMDQ_OP_TLBI_NH_VA
> > 	CMDQ_OP_ATC_INV
> > 
> > So, limiting a batch to one single opcode isn't ideal. Instead,
> > if we really have to apply an enforcement to every batch_add(),
> > I think the cmdq structure would need a scan function pointer:
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index d0d7c75c030a..1a83ad5ebadc 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> >  
> > +static bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
> > +				       struct arm_smmu_cmdq_ent *ent)
> > +{
> > +	if (!cmdq->supports_cmd)
> > +		return true;
> > +	return cmdq->supports_cmd(ent);
> > +}
> > +
> >  static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
> > @@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
> >  
> > -	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> > -	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) {
> > +	if ((cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> > +	     (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) ||
> > +	    !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd)) {
> >  		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
> 
> We'd need re-init the batch after this too..
> 
> Nicolin
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index e131d8170b90..c4872af6232c 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -616,2 +616,3 @@ struct arm_smmu_cmdq {
> >  	atomic_t			lock;
> > +	bool                            (*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
> >  };
> > 
> > That being said, the whole thing doesn't seem to have a lot value
> > at this moment, since the SMMU driver doesn't mix commands?

OK. I have added a patch for this. Let's just make things a bit
perfect at once.

Here is a v13 branch that addressed most of your remarks here:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v13

Would you please let me know if you are okay with this?

Thank you
Nicolin
Jason Gunthorpe Aug. 19, 2024, 5:39 p.m. UTC | #5 
On Fri, Aug 16, 2024 at 12:42:17PM -0700, Nicolin Chen wrote:
> > 
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > index e131d8170b90..c4872af6232c 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > @@ -616,2 +616,3 @@ struct arm_smmu_cmdq {
> > >  	atomic_t			lock;
> > > +	bool                            (*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
> > >  };
> > > 
> > > That being said, the whole thing doesn't seem to have a lot value
> > > at this moment, since the SMMU driver doesn't mix commands?
> 
> OK. I have added a patch for this. Let's just make things a bit
> perfect at once.

> Here is a v13 branch that addressed most of your remarks here:
> https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v13

It seems reasonable to me, it is sort of going back to my original
suggestion to pass in some kind of flag to say which commands were
going to be in the batch. Since all batches are simple op code right
now this seems good enough for today!

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 18d940c65e2c..8ff8e264d5e7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -336,12 +336,13 @@  static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 	return 0;
 }
 
-static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
+static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
+					       u8 opcode)
 {
 	struct arm_smmu_cmdq *cmdq = NULL;
 
 	if (smmu->impl && smmu->impl->get_secondary_cmdq)
-		cmdq = smmu->impl->get_secondary_cmdq(smmu);
+		cmdq = smmu->impl->get_secondary_cmdq(smmu, opcode);
 
 	return cmdq ?: &smmu->cmdq;
 }
@@ -889,7 +890,7 @@  static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
 	}
 
 	return arm_smmu_cmdq_issue_cmdlist(
-		smmu, arm_smmu_get_cmdq(smmu), cmd, 1, sync);
+		smmu, arm_smmu_get_cmdq(smmu, ent->opcode), cmd, 1, sync);
 }
 
 static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
@@ -905,10 +906,13 @@  static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
 }
 
 static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
-				     struct arm_smmu_cmdq_batch *cmds)
+				     struct arm_smmu_cmdq_batch *cmds,
+				     u8 opcode)
 {
+	WARN_ON_ONCE(!opcode);
+
 	cmds->num = 0;
-	cmds->cmdq = arm_smmu_get_cmdq(smmu);
+	cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);
 }
 
 static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
@@ -1195,7 +1199,7 @@  static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 		},
 	};
 
-	arm_smmu_cmdq_batch_init(smmu, &cmds);
+	arm_smmu_cmdq_batch_init(smmu, &cmds, CMDQ_OP_CFGI_CD);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.cfgi.sid = master->streams[i].id;
 		arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
@@ -2046,7 +2050,7 @@  static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
 
 	arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd);
 
-	arm_smmu_cmdq_batch_init(master->smmu, &cmds);
+	arm_smmu_cmdq_batch_init(master->smmu, &cmds, CMDQ_OP_ATC_INV);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
 		arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
@@ -2084,7 +2088,7 @@  int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
 		return 0;
 
-	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds);
+	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds, CMDQ_OP_ATC_INV);
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
 	list_for_each_entry(master_domain, &smmu_domain->devices,
@@ -2166,7 +2170,7 @@  static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 			num_pages++;
 	}
 
-	arm_smmu_cmdq_batch_init(smmu, &cmds);
+	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd->opcode);
 
 	while (iova < end) {
 		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 367f5e160af4..c7f34a5c31f3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -633,7 +633,8 @@  struct arm_smmu_strtab_cfg {
 struct arm_smmu_impl {
 	int (*device_reset)(struct arm_smmu_device *smmu);
 	void (*device_remove)(struct arm_smmu_device *smmu);
-	struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu);
+	struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu,
+			       u8 opcode);
 };
 
 #ifdef CONFIG_TEGRA241_CMDQV
diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index c1e85c95fb99..c20e54aeec99 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -144,6 +144,7 @@  struct tegra241_vcmdq {
  * struct tegra241_vintf - Virtual Interface
  * @idx: Global index in the CMDQV
  * @enabled: Enable status
+ * @hyp_own: Owned by hypervisor (in-kernel)
  * @cmdqv: Parent CMDQV pointer
  * @lvcmdqs: List of logical VCMDQ pointers
  * @base: MMIO base address
@@ -152,6 +153,7 @@  struct tegra241_vintf {
 	u16 idx;
 
 	bool enabled;
+	bool hyp_own;
 
 	struct tegra241_cmdqv *cmdqv;
 	struct tegra241_vcmdq **lvcmdqs;
@@ -301,8 +303,25 @@  static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 
 /* Command Queue Function */
 
+static bool tegra241_vintf_support_cmd(struct tegra241_vintf *vintf, u8 opcode)
+{
+       /* Hypervisor-owned VINTF can execute any command in its VCMDQs */
+	if (READ_ONCE(vintf->hyp_own))
+		return true;
+
+	/* Guest-owned VINTF must check against the list of supported CMDs */
+	switch (opcode) {
+	case CMDQ_OP_TLBI_NH_ASID:
+	case CMDQ_OP_TLBI_NH_VA:
+	case CMDQ_OP_ATC_INV:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static struct arm_smmu_cmdq *
-tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
+tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u8 opcode)
 {
 	struct tegra241_cmdqv *cmdqv =
 		container_of(smmu, struct tegra241_cmdqv, smmu);
@@ -317,6 +336,10 @@  tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
 	if (!READ_ONCE(vintf->enabled))
 		return NULL;
 
+	/* Unsupported CMD go for smmu->cmdq pathway */
+	if (!tegra241_vintf_support_cmd(vintf, opcode))
+		return NULL;
+
 	/*
 	 * Select a LVCMDQ to use. Here we use a temporal solution to
 	 * balance out traffic on cmdq issuing: each cmdq has its own
@@ -406,12 +429,22 @@  static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own)
 	tegra241_vintf_hw_deinit(vintf);
 
 	/* Configure and enable VINTF */
+	/*
+	 * Note that HYP_OWN bit is wired to zero when running in guest kernel,
+	 * whether enabling it here or not, as !HYP_OWN cmdq HWs only support a
+	 * restricted set of supported commands.
+	 */
 	regval = FIELD_PREP(VINTF_HYP_OWN, hyp_own);
 	writel(regval, REG_VINTF(vintf, CONFIG));
 
 	ret = vintf_write_config(vintf, regval | VINTF_EN);
 	if (ret)
 		return ret;
+	/*
+	 * As being mentioned above, HYP_OWN bit is wired to zero for a guest
+	 * kernel, so read it back from HW to ensure that reflects in hyp_own
+	 */
+	vintf->hyp_own = !!(VINTF_HYP_OWN & readl(REG_VINTF(vintf, CONFIG)));
 
 	for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) {
 		if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {