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

[Will Deacon]

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@xxxxxxxxxx>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> ---
>  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