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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 07, 2024 at 10:56: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.
> 
> 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.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 20 ++++++-----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 +--
>  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 36 ++++++++++++++++++-
>  3 files changed, 49 insertions(+), 12 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 d1098991d64e..baf20e9976d3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -332,10 +332,11 @@ 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)
>  {
>  	if (arm_smmu_has_tegra241_cmdqv(smmu))
> -		return tegra241_cmdqv_get_cmdq(smmu);
> +		return tegra241_cmdqv_get_cmdq(smmu, opcode);

It is worth a comment descrbing opcode here, I think.. At least the
nesting invalidation will send mixed batches.

opcode is sort of a handle for a group of related commands. But what
is the group? Minimally it is opcode + SYNC, right?

 The caller must only send opcode + SYNC commands to this queue.
 The opcodes XX,YY,ZZ are interchangable and can be sent together.

?

> -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> +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;
> +	}

When I look at the nesting patch it also includes SYNC, NH_VAA, and
NH_ALL. Are they supported here? VAA is not supported in the HW at all
right? What about NH_ALL?

It looks fine otherwise

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason