RE: [PATCH v2 6/7] iommu/vt-d: Add page request draining support
> From: Lu Baolu > Sent: Thursday, April 16, 2020 4:38 PM > > Hi Kevin, > > On 2020/4/15 19:10, Tian, Kevin wrote: > > the completion of above sequence ensures that previous queued > > page group responses are sent out and received by the endpoint > > and vice versa all in-fly page requests from the endpoint are queued > > in iommu page request queue. Then comes a problem - you didn't > > wait for completion of those newly-queued requests and their > > responses. > > I thought about this again. > > We do page request draining after PASID table entry gets torn down and > the devTLB gets invalidated. At this point, if any new page request for > this pasid comes in, IOMMU will generate an unrecoverable fault and > response the device with IR (Invalid Request). IOMMU won't put this page > request into the queue. [VT-d spec 7.4.1] Non-coverable fault implies severe errors, so I don't see why we should allow such thing happen when handling a normal situation. if you look at the start of chapter 7: -- Non-recoverable Faults: Requests that encounter non-recoverable address translation faults are aborted by the remapping hardware, and typically require a reset of the device (such as through a function- level-reset) to recover and re-initialize the device to put it back into service. -- > > Hence, we don't need to worry about the newly-queued requests here. > > Best regards, > Baolu Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/7] iommu/vt-d: Add page request draining support
Hi Kevin, On 2020/4/15 19:10, Tian, Kevin wrote: the completion of above sequence ensures that previous queued page group responses are sent out and received by the endpoint and vice versa all in-fly page requests from the endpoint are queued in iommu page request queue. Then comes a problem - you didn't wait for completion of those newly-queued requests and their responses. I thought about this again. We do page request draining after PASID table entry gets torn down and the devTLB gets invalidated. At this point, if any new page request for this pasid comes in, IOMMU will generate an unrecoverable fault and response the device with IR (Invalid Request). IOMMU won't put this page request into the queue. [VT-d spec 7.4.1] Hence, we don't need to worry about the newly-queued requests here. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/7] iommu/vt-d: Add page request draining support
On 2020/4/15 19:10, Tian, Kevin wrote:
From: Lu Baolu
Sent: Wednesday, April 15, 2020 1:26 PM
When a PASID is stopped or terminated, there can be pending
PRQs (requests that haven't received responses) in remapping
hardware. This adds the interface to drain page requests and
call it when a PASID is terminated.
Signed-off-by: Jacob Pan
Signed-off-by: Liu Yi L
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-svm.c | 90 ++---
include/linux/intel-iommu.h | 1 +
2 files changed, 86 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 05aeb8ea51c4..736dd39fb52b 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -23,6 +23,7 @@
#include "intel-pasid.h"
static irqreturn_t prq_event_thread(int irq, void *d);
+static void intel_svm_drain_prq(struct device *dev, int pasid);
#define PRQ_ORDER 0
@@ -210,6 +211,7 @@ static void intel_mm_release(struct mmu_notifier
*mn, struct mm_struct *mm)
rcu_read_lock();
list_for_each_entry_rcu(sdev, >devs, list) {
intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm-
pasid);
+ intel_svm_drain_prq(sdev->dev, svm->pasid);
I feel there is a problem here. If you clear the PASID entry before draining,
in-fly requests will hit unrecoverable fault instead, due to invalid PASID
entry.
The in-fly requests will be ignored by IOMMU if the pasid entry is
empty. It won't result in an unrecoverable fault.
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
}
rcu_read_unlock();
@@ -403,12 +405,8 @@ int intel_svm_unbind_gpasid(struct device *dev, int
pasid)
if (!sdev->users) {
list_del_rcu(>list);
intel_pasid_tear_down_entry(iommu, dev, svm-
pasid);
+ intel_svm_drain_prq(dev, svm->pasid);
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
- /* TODO: Drain in flight PRQ for the PASID since it
-* may get reused soon, we don't want to
-* confuse with its previous life.
-* intel_svm_drain_prq(dev, pasid);
-*/
kfree_rcu(sdev, rcu);
if (list_empty(>devs)) {
@@ -646,6 +644,7 @@ int intel_svm_unbind_mm(struct device *dev, int
pasid)
* large and has to be physically contiguous. So it's
* hard to be as defensive as we might like. */
intel_pasid_tear_down_entry(iommu, dev, svm-
pasid);
+ intel_svm_drain_prq(dev, svm->pasid);
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
kfree_rcu(sdev, rcu);
@@ -703,6 +702,7 @@ struct page_req_dsc {
struct page_req {
struct list_head list;
struct page_req_dsc desc;
+ struct completion complete;
unsigned int processing:1;
unsigned int drained:1;
unsigned int completed:1;
@@ -732,9 +732,83 @@ static bool is_canonical_address(u64 addr)
return (((saddr << shift) >> shift) == saddr);
}
+/**
+ * intel_svm_drain_prq:
+ *
+ * Drain all pending page requests related to a specific pasid in both
+ * software and hardware. The caller must guarantee that no more page
+ * requests related to this pasid coming.
+ */
+static void intel_svm_drain_prq(struct device *dev, int pasid)
+{
+ struct device_domain_info *info;
+ struct dmar_domain *domain;
+ struct intel_iommu *iommu;
+ struct qi_desc desc[3];
+ struct pci_dev *pdev;
+ struct page_req *req;
+ unsigned long flags;
+ u16 sid, did;
+ int qdep;
+
+ info = get_domain_info(dev);
+ if (WARN_ON(!info || !dev_is_pci(dev)))
+ return;
+
+ iommu = info->iommu;
+ domain = info->domain;
+ pdev = to_pci_dev(dev);
+
+ /* Mark all related pending requests drained. */
+ spin_lock_irqsave(>prq_lock, flags);
+ list_for_each_entry(req, >prq_list, list)
+ if (req->desc.pasid_present && req->desc.pasid == pasid)
+ req->drained = true;
+ spin_unlock_irqrestore(>prq_lock, flags);
+
+ /* Wait until all related pending requests complete. */
+retry:
+ spin_lock_irqsave(>prq_lock, flags);
+ list_for_each_entry(req, >prq_list, list) {
+ if (req->desc.pasid_present &&
+ req->desc.pasid == pasid &&
+ !req->completed) {
+ spin_unlock_irqrestore(>prq_lock, flags);
+ wait_for_completion_timeout(>complete, 5 *
HZ);
+ goto retry;
+ }
+ }
+ spin_unlock_irqrestore(>prq_lock, flags);
+
+ /*
+* Perform steps described in VT-d spec CH7.10 to drain page
+*
RE: [PATCH v2 6/7] iommu/vt-d: Add page request draining support
> From: Lu Baolu
> Sent: Wednesday, April 15, 2020 1:26 PM
>
> When a PASID is stopped or terminated, there can be pending
> PRQs (requests that haven't received responses) in remapping
> hardware. This adds the interface to drain page requests and
> call it when a PASID is terminated.
>
> Signed-off-by: Jacob Pan
> Signed-off-by: Liu Yi L
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/intel-svm.c | 90 ++---
> include/linux/intel-iommu.h | 1 +
> 2 files changed, 86 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 05aeb8ea51c4..736dd39fb52b 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -23,6 +23,7 @@
> #include "intel-pasid.h"
>
> static irqreturn_t prq_event_thread(int irq, void *d);
> +static void intel_svm_drain_prq(struct device *dev, int pasid);
>
> #define PRQ_ORDER 0
>
> @@ -210,6 +211,7 @@ static void intel_mm_release(struct mmu_notifier
> *mn, struct mm_struct *mm)
> rcu_read_lock();
> list_for_each_entry_rcu(sdev, >devs, list) {
> intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm-
> >pasid);
> + intel_svm_drain_prq(sdev->dev, svm->pasid);
I feel there is a problem here. If you clear the PASID entry before draining,
in-fly requests will hit unrecoverable fault instead, due to invalid PASID
entry.
> intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> }
> rcu_read_unlock();
> @@ -403,12 +405,8 @@ int intel_svm_unbind_gpasid(struct device *dev, int
> pasid)
> if (!sdev->users) {
> list_del_rcu(>list);
> intel_pasid_tear_down_entry(iommu, dev, svm-
> >pasid);
> + intel_svm_drain_prq(dev, svm->pasid);
> intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> - /* TODO: Drain in flight PRQ for the PASID since it
> - * may get reused soon, we don't want to
> - * confuse with its previous life.
> - * intel_svm_drain_prq(dev, pasid);
> - */
> kfree_rcu(sdev, rcu);
>
> if (list_empty(>devs)) {
> @@ -646,6 +644,7 @@ int intel_svm_unbind_mm(struct device *dev, int
> pasid)
>* large and has to be physically contiguous. So it's
>* hard to be as defensive as we might like. */
> intel_pasid_tear_down_entry(iommu, dev, svm-
> >pasid);
> + intel_svm_drain_prq(dev, svm->pasid);
> intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> kfree_rcu(sdev, rcu);
>
> @@ -703,6 +702,7 @@ struct page_req_dsc {
> struct page_req {
> struct list_head list;
> struct page_req_dsc desc;
> + struct completion complete;
> unsigned int processing:1;
> unsigned int drained:1;
> unsigned int completed:1;
> @@ -732,9 +732,83 @@ static bool is_canonical_address(u64 addr)
> return (((saddr << shift) >> shift) == saddr);
> }
>
> +/**
> + * intel_svm_drain_prq:
> + *
> + * Drain all pending page requests related to a specific pasid in both
> + * software and hardware. The caller must guarantee that no more page
> + * requests related to this pasid coming.
> + */
> +static void intel_svm_drain_prq(struct device *dev, int pasid)
> +{
> + struct device_domain_info *info;
> + struct dmar_domain *domain;
> + struct intel_iommu *iommu;
> + struct qi_desc desc[3];
> + struct pci_dev *pdev;
> + struct page_req *req;
> + unsigned long flags;
> + u16 sid, did;
> + int qdep;
> +
> + info = get_domain_info(dev);
> + if (WARN_ON(!info || !dev_is_pci(dev)))
> + return;
> +
> + iommu = info->iommu;
> + domain = info->domain;
> + pdev = to_pci_dev(dev);
> +
> + /* Mark all related pending requests drained. */
> + spin_lock_irqsave(>prq_lock, flags);
> + list_for_each_entry(req, >prq_list, list)
> + if (req->desc.pasid_present && req->desc.pasid == pasid)
> + req->drained = true;
> + spin_unlock_irqrestore(>prq_lock, flags);
> +
> + /* Wait until all related pending requests complete. */
> +retry:
> + spin_lock_irqsave(>prq_lock, flags);
> + list_for_each_entry(req, >prq_list, list) {
> + if (req->desc.pasid_present &&
> + req->desc.pasid == pasid &&
> + !req->completed) {
> + spin_unlock_irqrestore(>prq_lock, flags);
> + wait_for_completion_timeout(>complete, 5 *
> HZ);
> + goto retry;
> + }
> + }
> + spin_unlock_irqrestore(>prq_lock, flags);
> +
> + /*
> + * Perform steps described in VT-d spec CH7.10 to drain page
> + * request and responses in hardware.
> + */
> +
[PATCH v2 6/7] iommu/vt-d: Add page request draining support
When a PASID is stopped or terminated, there can be pending
PRQs (requests that haven't received responses) in remapping
hardware. This adds the interface to drain page requests and
call it when a PASID is terminated.
Signed-off-by: Jacob Pan
Signed-off-by: Liu Yi L
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-svm.c | 90 ++---
include/linux/intel-iommu.h | 1 +
2 files changed, 86 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 05aeb8ea51c4..736dd39fb52b 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -23,6 +23,7 @@
#include "intel-pasid.h"
static irqreturn_t prq_event_thread(int irq, void *d);
+static void intel_svm_drain_prq(struct device *dev, int pasid);
#define PRQ_ORDER 0
@@ -210,6 +211,7 @@ static void intel_mm_release(struct mmu_notifier *mn,
struct mm_struct *mm)
rcu_read_lock();
list_for_each_entry_rcu(sdev, >devs, list) {
intel_pasid_tear_down_entry(svm->iommu, sdev->dev, svm->pasid);
+ intel_svm_drain_prq(sdev->dev, svm->pasid);
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
}
rcu_read_unlock();
@@ -403,12 +405,8 @@ int intel_svm_unbind_gpasid(struct device *dev, int pasid)
if (!sdev->users) {
list_del_rcu(>list);
intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
+ intel_svm_drain_prq(dev, svm->pasid);
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
- /* TODO: Drain in flight PRQ for the PASID since it
-* may get reused soon, we don't want to
-* confuse with its previous life.
-* intel_svm_drain_prq(dev, pasid);
-*/
kfree_rcu(sdev, rcu);
if (list_empty(>devs)) {
@@ -646,6 +644,7 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
* large and has to be physically contiguous. So it's
* hard to be as defensive as we might like. */
intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
+ intel_svm_drain_prq(dev, svm->pasid);
intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
kfree_rcu(sdev, rcu);
@@ -703,6 +702,7 @@ struct page_req_dsc {
struct page_req {
struct list_head list;
struct page_req_dsc desc;
+ struct completion complete;
unsigned int processing:1;
unsigned int drained:1;
unsigned int completed:1;
@@ -732,9 +732,83 @@ static bool is_canonical_address(u64 addr)
return (((saddr << shift) >> shift) == saddr);
}
+/**
+ * intel_svm_drain_prq:
+ *
+ * Drain all pending page requests related to a specific pasid in both
+ * software and hardware. The caller must guarantee that no more page
+ * requests related to this pasid coming.
+ */
+static void intel_svm_drain_prq(struct device *dev, int pasid)
+{
+ struct device_domain_info *info;
+ struct dmar_domain *domain;
+ struct intel_iommu *iommu;
+ struct qi_desc desc[3];
+ struct pci_dev *pdev;
+ struct page_req *req;
+ unsigned long flags;
+ u16 sid, did;
+ int qdep;
+
+ info = get_domain_info(dev);
+ if (WARN_ON(!info || !dev_is_pci(dev)))
+ return;
+
+ iommu = info->iommu;
+ domain = info->domain;
+ pdev = to_pci_dev(dev);
+
+ /* Mark all related pending requests drained. */
+ spin_lock_irqsave(>prq_lock, flags);
+ list_for_each_entry(req, >prq_list, list)
+ if (req->desc.pasid_present && req->desc.pasid == pasid)
+ req->drained = true;
+ spin_unlock_irqrestore(>prq_lock, flags);
+
+ /* Wait until all related pending requests complete. */
+retry:
+ spin_lock_irqsave(>prq_lock, flags);
+ list_for_each_entry(req, >prq_list, list) {
+ if (req->desc.pasid_present &&
+ req->desc.pasid == pasid &&
+ !req->completed) {
+ spin_unlock_irqrestore(>prq_lock, flags);
+ wait_for_completion_timeout(>complete, 5 * HZ);
+ goto retry;
+ }
+ }
+ spin_unlock_irqrestore(>prq_lock, flags);
+
+ /*
+* Perform steps described in VT-d spec CH7.10 to drain page
+* request and responses in hardware.
+*/
+ sid = PCI_DEVID(info->bus, info->devfn);
+ did = domain->iommu_did[iommu->seq_id];
+ qdep = pci_ats_queue_depth(pdev);
+
+ memset(desc, 0, sizeof(desc));
+ desc[0].qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
+ QI_IWD_FENCE |
+ QI_IWD_TYPE;
+ desc[1].qw0 =
