Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu <yi.l.liu@intel.com>]

On 2024/11/6 15:31, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> pasid replacement allows converting a present pasid entry to be FS, SS,
>> PT or nested, hence add helpers for such operations. This simplifies the
>> callers as well since the caller can switch the pasid to the new domain
>> by one-shot.
> 
> 'simplify' compared to what? if it's an obvious result from creating
> the helpers then no need to talk about it.

agreed, no need to talk about it.

>>
>> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 173
>> ++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/intel/pasid.h |  12 +++
>>   2 files changed, 185 insertions(+)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 65fd2fee01b7..b7c2d65b8726 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>   	return 0;
>>   }
>>
>> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
>> +				    struct device *dev, pgd_t *pgd,
>> +				    u32 pasid, u16 did, int flags)
>> +{
>> +	struct pasid_entry *pte;
>> +	u16 old_did;
>> +
>> +	if (!ecap_flts(iommu->ecap) ||
>> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
>> +		return -EINVAL;
> 
> better copy the error messages from the setup part.
> 
> there may be further chance to consolidate them later but no clear
> reason why different error warning schemes should be used
> between them.
> 
> same for other helpers.

sure. I think Baolu has a point that this may be trigger-able by userspace
hence drop the error message to avoid DOS.

>> +
>> +	spin_lock(&iommu->lock);
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	if (!pte) {
>> +		spin_unlock(&iommu->lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!pasid_pte_is_present(pte)) {
>> +		spin_unlock(&iommu->lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	old_did = pasid_get_domain_id(pte);
> 
> probably should pass the old domain in and check whether the
> domain->did is same as the one in the pasid entry and warn otherwise.

this would be a sw bug. :) Do we really want to catch every bug by warn? :)

> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

-- 
Regards,
Yi Liu