Re: [PATCH v3 3/4] iommu/vt-d: Add page request draining support

[Jacob Pan]

On Wed, 29 Apr 2020 14:00:05 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

> Hi Jacob,
> 
> On 2020/4/29 11:36, Jacob Pan wrote:
> > On Wed, 22 Apr 2020 16:06:10 +0800
> > Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>  wrote:
> >   
> >> 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<jacob.jun.pan@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Liu Yi L<yi.l.liu@xxxxxxxxx>
> >> Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/iommu/intel-svm.c   | 103
> >> ++++++++++++++++++++++++++++++++++-- include/linux/intel-iommu.h |
> >> 4 ++ 2 files changed, 102 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> >> index 83dc4319f661..2534641ef707 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
> >>   
> >> @@ -66,6 +67,8 @@ int intel_svm_enable_prq(struct intel_iommu
> >> *iommu) dmar_writeq(iommu->reg + DMAR_PQT_REG, 0ULL);
> >>   	dmar_writeq(iommu->reg + DMAR_PQA_REG,
> >> virt_to_phys(iommu->prq) | PRQ_ORDER);
> >> +	init_completion(&iommu->prq_complete);
> >> +
> >>   	return 0;
> >>   }
> >>   
> >> @@ -208,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, &svm->devs, list) {
> >>   		intel_pasid_tear_down_entry(svm->iommu,
> >> sdev->dev, svm->pasid);
> >> +		intel_svm_drain_prq(sdev->dev, svm->pasid);  
> > mmu_notifier release is called in atomic context, drain_prq needs to
> > wait for completion. I tested exit path when a process crashes. I
> > got
> > 
> > [  +0.696214] BUG: sleeping function called from invalid context at
> > kernel/sched/completion.c:101 [  +0.000068] in_atomic(): 1,
> > irqs_disabled(): 0, non_block: 0, pid: 3235, name: dsatest
> > [  +0.000046] INFO: lockdep is turned off. [  +0.000002] CPU: 1
> > PID: 3235 Comm: dsatest Not tainted 5.7.0-rc1-z-svmtest+ #1637
> > [  +0.000000] Hardware name: Intel Corporation Kabylake Client
> > platform/Skylake Halo DDR4 RVP11, BIOS 04.1709050855 09/05/2017
> > [  +0.000001] Call Trace: [  +0.000004]  dump_stack+0x68/0x9b
> > [  +0.000003]  ___might_sleep+0x229/0x250
> > [  +0.000003]  wait_for_completion_timeout+0x3c/0x110
> > [  +0.000003]  intel_svm_drain_prq+0x12f/0x210
> > [  +0.000003]  intel_mm_release+0x73/0x110
> > [  +0.000003]  __mmu_notifier_release+0x94/0x220
> > [  +0.000002]  ? do_munmap+0x10/0x10
> > [  +0.000002]  ? prepare_ftrace_return+0x5c/0x80
> > [  +0.000003]  exit_mmap+0x156/0x1a0
> > [  +0.000002]  ? mmput+0x44/0x120
> > [  +0.000003]  ? exit_mmap+0x5/0x1a0
> > [  +0.000002]  ? ftrace_graph_caller+0xa0/0xa0
> > [  +0.000001]  mmput+0x5e/0x120
> > 
> >   
> 
> Thanks a lot!
> 
> Actually, we can't drain page requests in this mm_notifier code path,
> right? The assumptions of page request draining are that 1) the device
> driver has drained DMA requests in the device end; 2) the pasid entry
> has been marked as non-present. So we could only drain page requests
> in the unbind path.
> 
> Thought?
> 
Right, we could save the drain here. unbind will come soon when mm
exits. So even the in flight PRs come through, we could just respond
with "Invalid Response" after mm exit starts. The current code already
checks if the mm is exiting by mmget_not_zero.

> Best regards,
> baolu

[Jacob Pan]