[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ccd8352c-16e7-4845-a94e-b22bdb3ec5c7@intel.com>
Date: Wed, 30 Oct 2024 13:51:07 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Joel Granados
<joel.granados@...nel.org>
CC: David Woodhouse <dwmw2@...radead.org>, Joerg Roedel <joro@...tes.org>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
Gunthorpe" <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>, Klaus Jensen
<its@...elevant.dk>, <linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>,
Klaus Jensen <k.jensen@...sung.com>
Subject: Re: [PATCH v4 2/5] iommu/vt-d: Remove the pasid present check in
prq_event_thread
On 2024/10/29 13:39, Baolu Lu wrote:
> On 2024/10/29 13:13, Yi Liu wrote:
>> On 2024/10/29 11:12, Baolu Lu wrote:
>>> On 2024/10/28 18:24, Joel Granados wrote:
>>>> On Mon, Oct 28, 2024 at 03:50:46PM +0800, Yi Liu wrote:
>>>>> On 2024/10/16 05:08, Joel Granados wrote:
>>>>>> From: Klaus Jensen<k.jensen@...sung.com>
>>>>>>
>>>>>> PASID is not strictly needed when handling a PRQ event; remove the check
>>>>>> for the pasid present bit in the request. This change was not included
>>>>>> in the creation of prq.c to emphasize the change in capability checks
>>>>>> when handing PRQ events.
>>>>>>
>>>>>> Signed-off-by: Klaus Jensen<k.jensen@...sung.com>
>>>>>> Reviewed-by: Kevin Tian<kevin.tian@...el.com>
>>>>>> Signed-off-by: Joel Granados<joel.granados@...nel.org>
>>>>> looks like the PRQ draining is missed for the PRI usage. When a pasid
>>>>> entry is destroyed, it might need to add helper similar to the
>>>>> intel_drain_pasid_prq() to drain PRQ for the non-pasid usage.
>>>> These types of user space PRIs (non-pasid, non-svm) are created by
>>>> making use of iommufd_hwpt_replace_device. Which adds an entry to the
>>>> pasid_array indexed on IOMMU_NO_PASID (0U) via the following path:
>>>>
>>>> iommufd_hwpt_replace_device
>>>> -> iommufd_fault_domain_repalce_dev
>>>> -> __fault_domain_replace_dev
>>>> -> iommu_replace_group_handle
>>> -> __iommu_group_set_domain
>>> -> intel_iommu_attach_device
>>> -> device_block_translation
>>> -> intel_pasid_tear_down_entry(IOMMU_NO_PASID)
>>>
>>> Here a domain is removed from the pasid entry, hence we need to flush
>>> all page requests that are pending in the IOMMU page request queue or
>>> the PCI fabric.
>>>
>>>> -> xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
>>>>
>>>> It is my understanding that this will provide the needed relation
>>>> between the device and the prq in such a way that when remove_dev_pasid
>>>> is called, intel_iommu_drain_pasid_prq will be called with the
>>>> appropriate pasid value set to IOMMU_NO_PASID. Please correct me if I'm
>>>> mistaken.
>>>
>>> Removing a domain from a RID and a PASID are different paths.
>>> Previously, this IOMMU driver only supported page requests on PASID
>>> (non-IOMMU_NO_PASID). It is acceptable that it does not flush the PRQ in
>>> the domain-removing RID path.
>>>
>>> With the changes made in this series, the driver now supports page
>>> requests for RID. It should also flush the PRQ when removing a domain
>>> from a PASID entry for IOMMU_NO_PASID.
>>>
>>>>
>>>> Does this answer your question? Do you have a specific path that you are
>>>> looking at where a specific non-pasid drain is needed?
>>>
>>> Perhaps we can simply add below change.
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index e860bc9439a2..a24a42649621 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -4283,7 +4283,6 @@ static void intel_iommu_remove_dev_pasid(struct
>>> device *dev, ioasid_t pasid,
>>> intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>> kfree(dev_pasid);
>>> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>> - intel_drain_pasid_prq(dev, pasid);
>>> }
>>>
>>> static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>> index 2e5fa0a23299..8639f3eb4264 100644
>>> --- a/drivers/iommu/intel/pasid.c
>>> +++ b/drivers/iommu/intel/pasid.c
>>> @@ -265,6 +265,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu
>>> *iommu, struct device *dev,
>>> iommu->flush.flush_iotlb(iommu, did, 0, 0,
>>> DMA_TLB_DSI_FLUSH);
>>>
>>> devtlb_invalidation_with_pasid(iommu, dev, pasid);
>>> + intel_drain_pasid_prq(dev, pasid);
>>> }
>>>
>>> /*
>>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>>> index 078d1e32a24e..ff88f31053d1 100644
>>> --- a/drivers/iommu/intel/svm.c
>>> +++ b/drivers/iommu/intel/svm.c
>>> @@ -304,9 +304,6 @@ void intel_drain_pasid_prq(struct device *dev, u32
>>> pasid)
>>> int qdep;
>>>
>>> info = dev_iommu_priv_get(dev);
>>> - if (WARN_ON(!info || !dev_is_pci(dev)))
>>> - return;
>>> -
>>> if (!info->pri_enabled)
>>> return;
>>>
>>> Generally, intel_drain_pasid_prq() should be called if
>>>
>>> - a translation is removed from a pasid entry; and
>>> - PRI on this device is enabled.
>>
>> If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb invalidation
>> and dev-tlb invalidation descriptors. So extra code change is needed in
>> intel_drain_pasid_prq(). Or perhaps it's better to have a separate helper
>> for draining prq for non-pasid case.
>
> According to VT-d spec, section 7.10, "Software Steps to Drain Page
> Requests & Responses", we can simply replace p_iotlb_inv_dsc and
> p_dev_tlb_inv_dsc with iotlb_inv_dsc and dev_tlb_inv_dsc. Any
> significant negative performance impact?
It's not about performance impact. My point is to use iotlb_inv_dsc and
dev_tlb_inv_dsc for the @pasid==IOMMU_NO_PASID case. The existing
intel_drain_pasid_prq() only uses p_iotlb_inv_dsc and p_dev_tlb_inv_dsc.
The way you described in above reply works. But it needs to add if/else
to use the correct invalidation descriptor. Since the descriptor
composition has several lines, so just an ask if it's better to have a
separate helper. :)
--
Regards,
Yi Liu
Powered by blists - more mailing lists