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

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

On 2024/11/6 17:51, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 6, 2024 5:31 PM
>>
>> 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
>>>>
>>>>
>>>> +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.
>>
> 
> Isn't the existing path also trigger-able by userspace? It's better to
> have a consistent policy cross all paths then you can clean it up
> together later.

I see. May we add ratelimit to it.

>   
>>>> +
>>>> +	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? :)
>>
> 
> this one should not happen. If it does, something severe jumps out...

yes. that's why I doubt if it's valuable to do it. It should be a vital
bug that bring us this warn. or instead of passing id old domain, how
about just old_did? We use the passed in did instead of using the did
from pte.

-- 
Regards,
Yi Liu