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

Yi Liu
2024-11-06T09:26:57Z

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