diff mbox series

[v7,7/9] iommu/vt-d: Allow qi_submit_sync() to return the QI faults

Message ID 20231221153948.119007-8-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series Add iommufd nesting (part 2/2) | expand

Commit Message

Yi Liu Dec. 21, 2023, 3:39 p.m. UTC
From: Lu Baolu <baolu.lu@linux.intel.com>

This allows qi_submit_sync() to return back faults to callers.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/dmar.c          | 29 ++++++++++++++++++-----------
 drivers/iommu/intel/iommu.h         |  2 +-
 drivers/iommu/intel/irq_remapping.c |  2 +-
 drivers/iommu/intel/pasid.c         |  2 +-
 drivers/iommu/intel/svm.c           |  6 +++---
 5 files changed, 24 insertions(+), 17 deletions(-)

Comments

Tian, Kevin Dec. 22, 2023, 4:23 a.m. UTC | #1
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, December 21, 2023 11:40 PM
>
> +	fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
> +	if (fault) {
> +		if (fsts)
> +			*fsts |= fault;

do we expect the fault to be accumulated? otherwise it's clearer to
just do direct assignment instead of asking for the caller to clear
the variable before invocation.

the rest looks good:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Yi Liu Dec. 26, 2023, 4:03 a.m. UTC | #2
On 2023/12/22 12:23, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 21, 2023 11:40 PM
>>
>> +	fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>> +	if (fault) {
>> +		if (fsts)
>> +			*fsts |= fault;
> 
> do we expect the fault to be accumulated? otherwise it's clearer to
> just do direct assignment instead of asking for the caller to clear
> the variable before invocation.

not quite get. do you mean the fault should not be cleared in the caller
side?

> the rest looks good:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tian, Kevin Dec. 26, 2023, 4:13 a.m. UTC | #3
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, December 26, 2023 12:03 PM
> 
> On 2023/12/22 12:23, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Thursday, December 21, 2023 11:40 PM
> >>
> >> +	fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
> >> +	if (fault) {
> >> +		if (fsts)
> >> +			*fsts |= fault;
> >
> > do we expect the fault to be accumulated? otherwise it's clearer to
> > just do direct assignment instead of asking for the caller to clear
> > the variable before invocation.
> 
> not quite get. do you mean the fault should not be cleared in the caller
> side?
> 

I meant:

	if (fsts)
		*fsts = fault;

unless there is a reason to *OR* the original value.
Yi Liu Dec. 26, 2023, 6:15 a.m. UTC | #4
On 2023/12/26 12:13, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, December 26, 2023 12:03 PM
>>
>> On 2023/12/22 12:23, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Thursday, December 21, 2023 11:40 PM
>>>>
>>>> +	fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>>>> +	if (fault) {
>>>> +		if (fsts)
>>>> +			*fsts |= fault;
>>>
>>> do we expect the fault to be accumulated? otherwise it's clearer to
>>> just do direct assignment instead of asking for the caller to clear
>>> the variable before invocation.
>>
>> not quite get. do you mean the fault should not be cleared in the caller
>> side?
>>
> 
> I meant:
> 
> 	if (fsts)
> 		*fsts = fault;
> 
> unless there is a reason to *OR* the original value.

I guess no such a reason. :) let me modify it.
Yi Liu Dec. 26, 2023, 8:44 a.m. UTC | #5
On 2023/12/26 14:15, Yi Liu wrote:
> 
> 
> On 2023/12/26 12:13, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Tuesday, December 26, 2023 12:03 PM
>>>
>>> On 2023/12/22 12:23, Tian, Kevin wrote:
>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>> Sent: Thursday, December 21, 2023 11:40 PM
>>>>>
>>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>>>>> +    if (fault) {
>>>>> +        if (fsts)
>>>>> +            *fsts |= fault;
>>>>
>>>> do we expect the fault to be accumulated? otherwise it's clearer to
>>>> just do direct assignment instead of asking for the caller to clear
>>>> the variable before invocation.
>>>
>>> not quite get. do you mean the fault should not be cleared in the caller
>>> side?
>>>
>>
>> I meant:
>>
>>     if (fsts)
>>         *fsts = fault;
>>
>> unless there is a reason to *OR* the original value.
> 
> I guess no such a reason. :) let me modify it.

hmmm, replied too soon. The qi_check_fault() would be called multiple
times in one invalidation circle as qi_submit_sync() needs to see if any
fault happened before the hw writes back QI_DONE to the wait descriptor.
There can be ICE which may eventually result in ITE. So caller of 
qi_check_fault()
would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
then ICE would be missed since the input fsts pointer is the same in
one qi_submit_sync() call.
Tian, Kevin Dec. 26, 2023, 9:21 a.m. UTC | #6
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, December 26, 2023 4:44 PM
> 
> On 2023/12/26 14:15, Yi Liu wrote:
> >
> >
> > On 2023/12/26 12:13, Tian, Kevin wrote:
> >>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>> Sent: Tuesday, December 26, 2023 12:03 PM
> >>>
> >>> On 2023/12/22 12:23, Tian, Kevin wrote:
> >>>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>>> Sent: Thursday, December 21, 2023 11:40 PM
> >>>>>
> >>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
> >>>>> +    if (fault) {
> >>>>> +        if (fsts)
> >>>>> +            *fsts |= fault;
> >>>>
> >>>> do we expect the fault to be accumulated? otherwise it's clearer to
> >>>> just do direct assignment instead of asking for the caller to clear
> >>>> the variable before invocation.
> >>>
> >>> not quite get. do you mean the fault should not be cleared in the caller
> >>> side?
> >>>
> >>
> >> I meant:
> >>
> >>     if (fsts)
> >>         *fsts = fault;
> >>
> >> unless there is a reason to *OR* the original value.
> >
> > I guess no such a reason. :) let me modify it.
> 
> hmmm, replied too soon. The qi_check_fault() would be called multiple
> times in one invalidation circle as qi_submit_sync() needs to see if any
> fault happened before the hw writes back QI_DONE to the wait descriptor.
> There can be ICE which may eventually result in ITE. So caller of
> qi_check_fault()
> would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
> qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
> then ICE would be missed since the input fsts pointer is the same in
> one qi_submit_sync() call.
> 

ok, that makes sense then.
Duan, Zhenzhong Dec. 27, 2023, 9:06 a.m. UTC | #7
>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Tuesday, December 26, 2023 4:44 PM
>Subject: Re: [PATCH v7 7/9] iommu/vt-d: Allow qi_submit_sync() to return
>the QI faults
>
>On 2023/12/26 14:15, Yi Liu wrote:
>>
>>
>> On 2023/12/26 12:13, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Tuesday, December 26, 2023 12:03 PM
>>>>
>>>> On 2023/12/22 12:23, Tian, Kevin wrote:
>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>>> Sent: Thursday, December 21, 2023 11:40 PM
>>>>>>
>>>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>>>>>> +    if (fault) {
>>>>>> +        if (fsts)
>>>>>> +            *fsts |= fault;
>>>>>
>>>>> do we expect the fault to be accumulated? otherwise it's clearer to
>>>>> just do direct assignment instead of asking for the caller to clear
>>>>> the variable before invocation.
>>>>
>>>> not quite get. do you mean the fault should not be cleared in the caller
>>>> side?
>>>>
>>>
>>> I meant:
>>>
>>>     if (fsts)
>>>         *fsts = fault;
>>>
>>> unless there is a reason to *OR* the original value.
>>
>> I guess no such a reason. :) let me modify it.
>
>hmmm, replied too soon. The qi_check_fault() would be called multiple
>times in one invalidation circle as qi_submit_sync() needs to see if any
>fault happened before the hw writes back QI_DONE to the wait descriptor.
>There can be ICE which may eventually result in ITE. So caller of
>qi_check_fault()
>would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
>qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
>then ICE would be missed since the input fsts pointer is the same in
>one qi_submit_sync() call.

Is it necessary to return fault to user if qi_check_fault() return -EAGAIN and
a restart run succeeds?

Thanks
Zhenzhong
Ethan Zhao Dec. 27, 2023, 9:33 a.m. UTC | #8
On 12/27/2023 5:06 PM, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, December 26, 2023 4:44 PM
>> Subject: Re: [PATCH v7 7/9] iommu/vt-d: Allow qi_submit_sync() to return
>> the QI faults
>>
>> On 2023/12/26 14:15, Yi Liu wrote:
>>>
>>> On 2023/12/26 12:13, Tian, Kevin wrote:
>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>> Sent: Tuesday, December 26, 2023 12:03 PM
>>>>>
>>>>> On 2023/12/22 12:23, Tian, Kevin wrote:
>>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>>>> Sent: Thursday, December 21, 2023 11:40 PM
>>>>>>>
>>>>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>>>>>>> +    if (fault) {
>>>>>>> +        if (fsts)
>>>>>>> +            *fsts |= fault;
>>>>>> do we expect the fault to be accumulated? otherwise it's clearer to
>>>>>> just do direct assignment instead of asking for the caller to clear
>>>>>> the variable before invocation.
>>>>> not quite get. do you mean the fault should not be cleared in the caller
>>>>> side?
>>>>>
>>>> I meant:
>>>>
>>>>      if (fsts)
>>>>          *fsts = fault;
>>>>
>>>> unless there is a reason to *OR* the original value.
>>> I guess no such a reason. :) let me modify it.
>> hmmm, replied too soon. The qi_check_fault() would be called multiple
>> times in one invalidation circle as qi_submit_sync() needs to see if any
>> fault happened before the hw writes back QI_DONE to the wait descriptor.
>> There can be ICE which may eventually result in ITE. So caller of
>> qi_check_fault()
>> would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
>> qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
>> then ICE would be missed since the input fsts pointer is the same in
>> one qi_submit_sync() call.
> Is it necessary to return fault to user if qi_check_fault() return -EAGAIN and
> a restart run succeeds?

Issue a device-TLB invalidation to no response device there is possibility

will be trapped there loop for ITE , never get return.

Thanks,

Ethan

> Thanks
> Zhenzhong
Yi Liu Dec. 27, 2023, 2:12 p.m. UTC | #9
On 2023/12/27 17:33, Ethan Zhao wrote:
> 
> On 12/27/2023 5:06 PM, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Tuesday, December 26, 2023 4:44 PM
>>> Subject: Re: [PATCH v7 7/9] iommu/vt-d: Allow qi_submit_sync() to return
>>> the QI faults
>>>
>>> On 2023/12/26 14:15, Yi Liu wrote:
>>>>
>>>> On 2023/12/26 12:13, Tian, Kevin wrote:
>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>>> Sent: Tuesday, December 26, 2023 12:03 PM
>>>>>>
>>>>>> On 2023/12/22 12:23, Tian, Kevin wrote:
>>>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>>>>> Sent: Thursday, December 21, 2023 11:40 PM
>>>>>>>>
>>>>>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
>>>>>>>> +    if (fault) {
>>>>>>>> +        if (fsts)
>>>>>>>> +            *fsts |= fault;
>>>>>>> do we expect the fault to be accumulated? otherwise it's clearer to
>>>>>>> just do direct assignment instead of asking for the caller to clear
>>>>>>> the variable before invocation.
>>>>>> not quite get. do you mean the fault should not be cleared in the caller
>>>>>> side?
>>>>>>
>>>>> I meant:
>>>>>
>>>>>      if (fsts)
>>>>>          *fsts = fault;
>>>>>
>>>>> unless there is a reason to *OR* the original value.
>>>> I guess no such a reason. :) let me modify it.
>>> hmmm, replied too soon. The qi_check_fault() would be called multiple
>>> times in one invalidation circle as qi_submit_sync() needs to see if any
>>> fault happened before the hw writes back QI_DONE to the wait descriptor.
>>> There can be ICE which may eventually result in ITE. So caller of
>>> qi_check_fault()
>>> would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
>>> qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
>>> then ICE would be missed since the input fsts pointer is the same in
>>> one qi_submit_sync() call.
>> Is it necessary to return fault to user if qi_check_fault() return 
>> -EAGAIN and
>> a restart run succeeds?

no need if a restart succeeds. I would add a *fault = 0 per the restart.

> 
> Issue a device-TLB invalidation to no response device there is possibility
> 
> will be trapped there loop for ITE , never get return.

yes. This the implementation today, in future I think we may need a kind
of timeout mechanism, so that it can return and report the error to user.
In concept, in nested translation, the page table is owned by userspace, so
it makes more sense to let userspace know it and take proper action.
Tian, Kevin Dec. 28, 2023, 5:39 a.m. UTC | #10
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, December 27, 2023 10:13 PM
> 
> On 2023/12/27 17:33, Ethan Zhao wrote:
> >
> > On 12/27/2023 5:06 PM, Duan, Zhenzhong wrote:
> >>
> >>> -----Original Message-----
> >>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>> Sent: Tuesday, December 26, 2023 4:44 PM
> >>> Subject: Re: [PATCH v7 7/9] iommu/vt-d: Allow qi_submit_sync() to
> return
> >>> the QI faults
> >>>
> >>> On 2023/12/26 14:15, Yi Liu wrote:
> >>>>
> >>>> On 2023/12/26 12:13, Tian, Kevin wrote:
> >>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>>>> Sent: Tuesday, December 26, 2023 12:03 PM
> >>>>>>
> >>>>>> On 2023/12/22 12:23, Tian, Kevin wrote:
> >>>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>>>>>> Sent: Thursday, December 21, 2023 11:40 PM
> >>>>>>>>
> >>>>>>>> +    fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
> >>>>>>>> +    if (fault) {
> >>>>>>>> +        if (fsts)
> >>>>>>>> +            *fsts |= fault;
> >>>>>>> do we expect the fault to be accumulated? otherwise it's clearer to
> >>>>>>> just do direct assignment instead of asking for the caller to clear
> >>>>>>> the variable before invocation.
> >>>>>> not quite get. do you mean the fault should not be cleared in the
> caller
> >>>>>> side?
> >>>>>>
> >>>>> I meant:
> >>>>>
> >>>>>      if (fsts)
> >>>>>          *fsts = fault;
> >>>>>
> >>>>> unless there is a reason to *OR* the original value.
> >>>> I guess no such a reason. :) let me modify it.
> >>> hmmm, replied too soon. The qi_check_fault() would be called multiple
> >>> times in one invalidation circle as qi_submit_sync() needs to see if any
> >>> fault happened before the hw writes back QI_DONE to the wait
> descriptor.
> >>> There can be ICE which may eventually result in ITE. So caller of
> >>> qi_check_fault()
> >>> would continue to wait for QI_DONE. So qi_check_fault() returns 0 to let
> >>> qi_submit_sync() go on though ICE detected. If we use '*fsts = fault;',
> >>> then ICE would be missed since the input fsts pointer is the same in
> >>> one qi_submit_sync() call.
> >> Is it necessary to return fault to user if qi_check_fault() return
> >> -EAGAIN and
> >> a restart run succeeds?
> 
> no need if a restart succeeds. I would add a *fault = 0 per the restart.
> 
> >
> > Issue a device-TLB invalidation to no response device there is possibility
> >
> > will be trapped there loop for ITE , never get return.
> 
> yes. This the implementation today, in future I think we may need a kind
> of timeout mechanism, so that it can return and report the error to user.
> In concept, in nested translation, the page table is owned by userspace, so
> it makes more sense to let userspace know it and take proper action.
> 

it doesn't make sense to retry upon an invalidation request from userspace.
if retry is required that is the policy of guest iommu driver. Also it's not
 good to introduce a uapi flag which won't be set by current driver.

this can be solved by a simple change in qi_check_fault():

	if (qi->desc_status[wait_index] == QI_ABORT)
- 		return -EAGAIN;
+		return fsts ? -ETIMEDOUT : -EAGAIN;

because if the caller wants to know the fault reason the implication
is that the caller will decide how to cope with the fault. It is incorrect
for qi_check_fault() to decide.
diff mbox series

Patch

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 23cb80d62a9a..a3e0cb720e06 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1267,7 +1267,8 @@  static void qi_dump_fault(struct intel_iommu *iommu, u32 fault)
 	       (unsigned long long)desc->qw1);
 }
 
-static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
+static int qi_check_fault(struct intel_iommu *iommu, int index,
+			  int wait_index, u32 *fsts)
 {
 	u32 fault;
 	int head, tail;
@@ -1278,8 +1279,12 @@  static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
 		return -EAGAIN;
 
 	fault = readl(iommu->reg + DMAR_FSTS_REG);
-	if (fault & (DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE))
+	fault &= DMA_FSTS_IQE | DMA_FSTS_ITE | DMA_FSTS_ICE;
+	if (fault) {
+		if (fsts)
+			*fsts |= fault;
 		qi_dump_fault(iommu, fault);
+	}
 
 	/*
 	 * If IQE happens, the head points to the descriptor associated
@@ -1342,9 +1347,11 @@  static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
  * time, a wait descriptor will be appended to each submission to ensure
  * hardware has completed the invalidation before return. Wait descriptors
  * can be part of the submission but it will not be polled for completion.
+ * If callers are interested in the QI faults that occur during the handling
+ * of requests, the QI faults are saved in @fault.
  */
 int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
-		   unsigned int count, unsigned long options)
+		   unsigned int count, unsigned long options, u32 *fault)
 {
 	struct q_inval *qi = iommu->qi;
 	s64 devtlb_start_ktime = 0;
@@ -1430,7 +1437,7 @@  int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
 		 * a deadlock where the interrupt context can wait indefinitely
 		 * for free slots in the queue.
 		 */
-		rc = qi_check_fault(iommu, index, wait_index);
+		rc = qi_check_fault(iommu, index, wait_index, fault);
 		if (rc)
 			break;
 
@@ -1476,7 +1483,7 @@  void qi_global_iec(struct intel_iommu *iommu)
 	desc.qw3 = 0;
 
 	/* should never fail */
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid, u8 fm,
@@ -1490,7 +1497,7 @@  void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid, u8 fm,
 	desc.qw2 = 0;
 	desc.qw3 = 0;
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
@@ -1514,7 +1521,7 @@  void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
 	desc.qw2 = 0;
 	desc.qw3 = 0;
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
@@ -1545,7 +1552,7 @@  void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
 	desc.qw2 = 0;
 	desc.qw3 = 0;
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 /* PASID-based IOTLB invalidation */
@@ -1586,7 +1593,7 @@  void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, u64 addr,
 				QI_EIOTLB_AM(mask);
 	}
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 /* PASID-based device IOTLB Invalidate */
@@ -1639,7 +1646,7 @@  void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, u16 pfsid,
 		desc.qw1 |= QI_DEV_EIOTLB_SIZE;
 	}
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did,
@@ -1649,7 +1656,7 @@  void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did,
 
 	desc.qw0 = QI_PC_PASID(pasid) | QI_PC_DID(did) |
 			QI_PC_GRAN(granu) | QI_PC_TYPE;
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 /*
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index ce030c5b5772..c6de958e4f54 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -881,7 +881,7 @@  void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, u64 granu,
 			  u32 pasid);
 
 int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
-		   unsigned int count, unsigned long options);
+		   unsigned int count, unsigned long options, u32 *fault);
 /*
  * Options used in qi_submit_sync:
  * QI_OPT_WAIT_DRAIN - Wait for PRQ drain completion, spec 6.5.2.8.
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 29b9e55dcf26..f834afa3672d 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -153,7 +153,7 @@  static int qi_flush_iec(struct intel_iommu *iommu, int index, int mask)
 	desc.qw2 = 0;
 	desc.qw3 = 0;
 
-	return qi_submit_sync(iommu, &desc, 1, 0);
+	return qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 static int modify_irte(struct irq_2_iommu *irq_iommu,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 74e8e4c17e81..67f924760ba8 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -467,7 +467,7 @@  pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,
 	desc.qw2 = 0;
 	desc.qw3 = 0;
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 static void
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ac12f76c1212..660d049ad5b6 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -543,7 +543,7 @@  void intel_drain_pasid_prq(struct device *dev, u32 pasid)
 			QI_DEV_IOTLB_PFSID(info->pfsid);
 qi_retry:
 	reinit_completion(&iommu->prq_complete);
-	qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
+	qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN, NULL);
 	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
 		wait_for_completion(&iommu->prq_complete);
 		goto qi_retry;
@@ -646,7 +646,7 @@  static void handle_bad_prq_event(struct intel_iommu *iommu,
 		desc.qw3 = 0;
 	}
 
-	qi_submit_sync(iommu, &desc, 1, 0);
+	qi_submit_sync(iommu, &desc, 1, 0, NULL);
 }
 
 static irqreturn_t prq_event_thread(int irq, void *d)
@@ -811,7 +811,7 @@  int intel_svm_page_response(struct device *dev,
 				ktime_to_ns(ktime_get()) - prm->private_data[0]);
 		}
 
-		qi_submit_sync(iommu, &desc, 1, 0);
+		qi_submit_sync(iommu, &desc, 1, 0, NULL);
 	}
 out:
 	return ret;