Re: [RFC PATCH v5 07/10] iommu/riscv: Add domain_alloc_paging_flags for second-stage domain

[fangyu . yu]

>> [ ... 22 lines skipped ... ]
>> -static struct iommu_domain *riscv_iommu_alloc_paging_domain(struct device *dev)
>> +static struct iommu_domain *
>> +riscv_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>> +				      const struct iommu_user_data *user_data)
>>  {
>>  	struct pt_iommu_riscv_64_cfg cfg = {};
>>  	struct riscv_iommu_domain *domain;
>>  	struct riscv_iommu_device *iommu;
>>  	int ret;
>> +	const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
>> +				    IOMMU_HWPT_ALLOC_NEST_PARENT;
>>
>> -	iommu = dev_to_iommu(dev);
>> -	if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV57) {
>> -		cfg.common.hw_max_vasz_lg2 = 57;
>> -	} else if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV48) {
>> -		cfg.common.hw_max_vasz_lg2 = 48;
>> -	} else if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV39) {
>> -		cfg.common.hw_max_vasz_lg2 = 39;
>> -	} else {
>> -		dev_err(dev, "cannot find supported page table mode\n");
>> -		return ERR_PTR(-ENODEV);
>> -	}
>> -	cfg.common.hw_max_oasz_lg2 = 56;
>> +	if (flags & ~supported_flags)
>> +		return ERR_PTR(-EOPNOTSUPP);
>
>This probably isn't needed since the switch checks it?
>

Right, the switch already enumerates all valid combinations, and
the default case returns -EOPNOTSUPP, so this check is
redundant. I'll drop it, along with supported_flags, in v6.

>> [ ... 30 lines skipped ... ]
>> +	switch (flags) {
>> +	case 0:
>> +		cfg.common.features |= BIT(PT_FEAT_SIGN_EXTEND);
>> +		if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV57) {
>> +			cfg.common.hw_max_vasz_lg2 = 57;
>> +		} else if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV48) {
>> +			cfg.common.hw_max_vasz_lg2 = 48;
>> +		} else if (iommu->caps & RISCV_IOMMU_CAPABILITIES_SV39) {
>> +			cfg.common.hw_max_vasz_lg2 = 39;
>> +		} else {
>> +			ret = -ENODEV;
>> +			goto err_free;
>> +		}
>> +		domain->pscid = ida_alloc_range(&riscv_iommu_pscids, 1,
>> +						RISCV_IOMMU_MAX_PSCID, GFP_KERNEL);
>> +		if (domain->pscid < 0) {
>> +			ret = -ENOMEM;
>> +			goto err_free;
>> +		}
>> +		break;
>
>I'd prefer you make functions riscv_iommu_init_domain_s1/s2() to put
>this in, it will be clearer
>

Agreed. In v6, I will move the page table mode selection, the
PSCID/GSCID allocation, and the stage-specific feature bits
into riscv_iommu_init_domain_s1/s2().

Thanks,
Fangyu

>It looks ok otherwise
>
>Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>
>--
>Jason