Re: [PATCH v4 2/5] iommu/vt-d: Remove the pasid present check in prq_event_thread
From: Baolu Lu
Date: Mon Oct 28 2024 - 23:14:03 EST
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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Reviewed-by: Kevin Tian<kevin.tian@xxxxxxxxx>
Signed-off-by: Joel Granados<joel.granados@xxxxxxxxxx>
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.
Thought?
--
baolu