Re: [PATCH v7 05/10] iommu/riscv: support GSCID and GVMA invalidation command

[fangyu.yu@linux.alibaba.com]

>Hi Fangyu,
>
>> From: Zong Li <zong.li@sifive.com>
>>
>> This patch adds a ID Allocator for GSCID and a wrap for setting up
>> GSCID in IOTLB invalidation command.
>>
>> Set up iohgatp to enable second stage table and flush stage-2 table if
>> the GSCID is set.
>>
>> The GSCID of domain should be freed when release domain. GSCID will be
>> allocated for parent domain in nested IOMMU process.
>>
>> Signed-off-by: Zong Li <zong.li@sifive.com>
>> Signed-off-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
>> ---
>>  drivers/iommu/riscv/iommu-bits.h |  7 ++++
>>  drivers/iommu/riscv/iommu.c      | 62 ++++++++++++++++++++++++++------
>>  2 files changed, 58 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/riscv/iommu-bits.h b/drivers/iommu/riscv/iommu-bits.h
>> index f2ef9bd3cde9..65d1f2250011 100644
>> --- a/drivers/iommu/riscv/iommu-bits.h
>> +++ b/drivers/iommu/riscv/iommu-bits.h
>> @@ -720,6 +720,13 @@ static inline void riscv_iommu_cmd_inval_vma(struct riscv_iommu_command *cmd)
>>  	cmd->dword1 = 0;
>>  }
>>
>> +static inline void riscv_iommu_cmd_inval_gvma(struct riscv_iommu_command *cmd)
>> +{
>> +	cmd->dword0 = FIELD_PREP(RISCV_IOMMU_CMD0_OPCODE, RISCV_IOMMU_CMD_IOTINVAL_OPCODE) |
>> +		      FIELD_PREP(RISCV_IOMMU_CMD0_FUNC, RISCV_IOMMU_CMD_IOTINVAL_FUNC_GVMA);
>> +	cmd->dword1 = 0;
>> +}
>> +
>>  static inline void riscv_iommu_cmd_inval_set_addr(struct riscv_iommu_command *cmd,
>>  						  u64 addr)
>>  {
>> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>> index 918fce4aa8c4..5546dfcd976c 100644
>> --- a/drivers/iommu/riscv/iommu.c
>> +++ b/drivers/iommu/riscv/iommu.c
>> @@ -49,6 +49,10 @@
>>  static DEFINE_IDA(riscv_iommu_pscids);
>>  #define RISCV_IOMMU_MAX_PSCID		(BIT(20) - 1)
>>
>> +/* IOMMU GSCID allocation namespace. */
>> +static DEFINE_IDA(riscv_iommu_gscids);
>> +#define RISCV_IOMMU_MAX_GSCID		(BIT(16) - 1)
>> +
>>  /* Device resource-managed allocations */
>>  struct riscv_iommu_devres {
>>  	void *addr;
>> @@ -842,6 +846,7 @@ struct riscv_iommu_domain {
>>  	struct list_head bonds;
>>  	spinlock_t lock;		/* protect bonds list updates. */
>>  	int pscid;
>> +	int gscid;
>>  };
>>  PT_IOMMU_CHECK_DOMAIN(struct riscv_iommu_domain, riscvpt.iommu, domain);
>>
>> @@ -1004,7 +1009,7 @@ static void riscv_iommu_tlbi_calc(struct riscv_iommu_tlbi *tlbi,
>>  }
>>
>>  static void riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
>> -					  int pscid,
>> +					  int pscid, int gscid,
>>  					  struct riscv_iommu_tlbi *tlbi)
>>  {
>>  	bool use_nl = tlbi->non_leaf &&
>> @@ -1012,8 +1017,13 @@ static void riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
>>  	struct riscv_iommu_command cmd;
>>  	unsigned int i;
>>
>> -	riscv_iommu_cmd_inval_vma(&cmd);
>> -	riscv_iommu_cmd_inval_set_pscid(&cmd, pscid);
>> +	if (gscid) {
>> +		riscv_iommu_cmd_inval_gvma(&cmd);
>> +		riscv_iommu_cmd_inval_set_gscid(&cmd, gscid);
>> +	} else {
>> +		riscv_iommu_cmd_inval_vma(&cmd);
>> +		riscv_iommu_cmd_inval_set_pscid(&cmd, pscid);
>> +	}
>
>As I understand it, the same choice is needed in
>riscv_iommu_bond_unlink().
>
>The missing GVMA command does not break anything today, because every
>caller of bond_unlink() runs iodir_update() just before it, and that
>already sends the GVMA for the old GSCID. But depending on that
>ordering seems fragile, and the comment in bond_unlink(), "invalidate
>all cached entries for domain's PSCID", is no longer true now that we
>have S2 domains.
>

There is no functional issue with the current code, since
iodir_update() already performs the required invalidation.

That said, as this series adds S2 domain support,
bond_unlink() should indeed be updated accordingly.
Good catch.

Thanks,
Fangyu

>The change, if you think it is needed, fits better in the last patch,
>"iommu/riscv: Add IOTINVAL.GVMA invalidations". I am replying here
>only because the code is very similar.
>
>Thanks,
>Shuai
>