Re: [PATCH v7 09/10] iommu/riscv: Add dirty tracking support for second-stage domains

[fangyu.yu@linux.alibaba.com]

>Hi Fangyu,
>
>> From: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>> 
>> Add hardware dirty tracking support for second-stage (iohgatp) domains
>> used in KVM VFIO device pass-through.
>> 
>> The RISC-V IOMMU can automatically set the dirty bit in PTEs on write
>> access when DC.tc.GADE is set and the hardware has AMO_HWAD capability.
>> Wire this up to the iommufd dirty tracking interface:
>> 
>>   - riscv_iommu_set_dirty_tracking(): Return success without toggling GADE.
>> 
>>   - riscv_iommu_dirty_ops: Exposes set_dirty_tracking and the generic
>>     page-table read_and_clear_dirty via IOMMU_PT_DIRTY_OPS(riscv_64).
>> 
>>   - domain_alloc_paging_flags: Assigns dirty_ops to second-stage domains
>>     when IOMMU_HWPT_ALLOC_DIRTY_TRACKING is requested.
>> 
>>   - riscv_iommu_capable: Reports IOMMU_CAP_DIRTY_TRACKING when
>>     AMO_HWAD is present.
>> 
>> Signed-off-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
>> ---
>>  drivers/iommu/riscv/iommu.c | 32 ++++++++++++++++++++++++++++++--
>>  1 file changed, 30 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>> index b3f05dfdf278..ff1043afe973 100644
>> --- a/drivers/iommu/riscv/iommu.c
>> +++ b/drivers/iommu/riscv/iommu.c
>> @@ -1350,6 +1350,10 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
>>  	if (domain->gscid) {
>>  		if (!riscv_iommu_iohgatp_supported(iommu, pt_info.iohgatp_mode))
>>  			return -EINVAL;
>> +		/* Dirty tracking requires AMO_HWAD on every attached IOMMU */
>> +		if (iommu_domain->dirty_ops &&
>> +		    !(iommu->caps & RISCV_IOMMU_CAPABILITIES_AMO_HWAD))
>> +			return -EINVAL;
>>  
>>  		dc.iohgatp =
>>  			FIELD_PREP(RISCV_IOMMU_DC_IOHGATP_MODE, pt_info.iohgatp_mode) |
>> @@ -1377,6 +1381,21 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
>>  	return 0;
>>  }
>>  
>> +static int riscv_iommu_set_dirty_tracking(struct iommu_domain *iommu_domain,
>> +					  bool enable)
>> +{
>> +	/*
>> +	 * Always enabled and the dirty bitmap is cleared prior to
>> +	 * set_dirty_tracking().
>> +	 */
>> +	return 0;
>> +}
>> +
>> +static const struct iommu_dirty_ops riscv_iommu_dirty_ops = {
>> +	IOMMU_PT_DIRTY_OPS(riscv_64),
>> +	.set_dirty_tracking = riscv_iommu_set_dirty_tracking,
>> +};
>> +
>>  static const struct iommu_domain_ops riscv_iommu_paging_domain_ops = {
>>  	IOMMU_PT_DOMAIN_OPS(riscv_64),
>>  	.attach_dev = riscv_iommu_attach_paging_domain,
>> @@ -1418,7 +1437,8 @@ static int riscv_iommu_init_domain_s1(struct device *dev,
>>   */
>>  static int riscv_iommu_init_domain_s2(struct device *dev,
>>  				      struct riscv_iommu_domain *domain,
>> -				      struct pt_iommu_riscv_64_cfg *cfg)
>> +				      struct pt_iommu_riscv_64_cfg *cfg,
>> +				      u32 flags)
>>  {
>>  	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
>>  
>> @@ -1439,6 +1459,10 @@ static int riscv_iommu_init_domain_s2(struct device *dev,
>>  	if (domain->gscid < 0)
>>  		return -ENOMEM;
>>  
>> +	/* AMO_HWAD is checked by iommufd via IOMMU_CAP_DIRTY_TRACKING */
>> +	if (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING)
>> +		domain->domain.dirty_ops = &riscv_iommu_dirty_ops;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1481,7 +1505,7 @@ riscv_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>>  	case IOMMU_HWPT_ALLOC_NEST_PARENT:
>>  	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING:
>>  	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_NEST_PARENT:
>> -		ret = riscv_iommu_init_domain_s2(dev, domain, &cfg);
>> +		ret = riscv_iommu_init_domain_s2(dev, domain, &cfg, flags);
>>  		break;
>>  	default:
>>  		ret = -EOPNOTSUPP;
>> @@ -1559,9 +1583,13 @@ static struct iommu_group *riscv_iommu_device_group(struct device *dev)
>>  
>>  static bool riscv_iommu_capable(struct device *dev, enum iommu_cap cap)
>>  {
>> +	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
>> +
>>  	switch (cap) {
>>  	case IOMMU_CAP_CACHE_COHERENCY:
>>  		return dev_is_dma_coherent(dev);
>> +	case IOMMU_CAP_DIRTY_TRACKING:
>> +		return !!(iommu->caps & RISCV_IOMMU_CAPABILITIES_AMO_HWAD);
>
>I'm not quite sure about this. Could there be a device that supports
>AMO_HWAD but not any of the S2 modes? GET_HW_INFO would report
>DIRTY_TRACKING as supported, but HWPT_ALLOC would then fail in
>riscv_iommu_init_domain_s2(). Should this check also include the
>S2 modes, like
>
>    AMO_HWAD && (SV39X4|SV48X4|SV57X4) ?
>

Good question, I had also considered this.

From the IOMMU spec alone, I could not confirm whether
AMO_HWAD implies support for S2 modes.

However, I found this statement in the IOMMU spec:

"The IOMMU must support all the virtual memory
extensions that are supported by any of the harts in
the system."

For virtualization scenarios, we assume the CPU hart
must support at least one S2 mode (hgatp). From this
perspective, the IOMMU should support at least one S2
mode in a virtualization setup, regardless of
AMO_HWAD.

>BTW, in riscv_iommu_init_domain_s1/s2, why not return -EOPNOTSUPP
>instead of -ENODEV for "cannot find supported page table mode"?
>

I used -ENODEV to follow the existing S1 behavior:
no compatible paging mode is available on this IOMMU.

While -EOPNOTSUPP would also be reasonable, I suggest
keeping S1 and S2 consistent and leaving this unchanged
in this series.

Thanks,
Fangyu

>Thanks,
>Shuai
>
>>  	default:
>>  		return false;
>>  	}
>> -- 
>> 2.50.1
>>