Re: [PATCH v2 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/12/2015 02:09, Xu, Quan wrote:
> On 11.12.2015 at 3:03pm, <andrew.cooper3@citrix.com> wrote:
>> On 10/12/15 09:33, Quan Xu wrote:
>>> Signed-off-by: Quan Xu <quan.xu@intel.com>
>>> ---
>>>  xen/drivers/passthrough/vtd/qinval.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/vtd/qinval.c
>>> b/xen/drivers/passthrough/vtd/qinval.c
>>> index b81b0bd..990baf2 100644
>>> --- a/xen/drivers/passthrough/vtd/qinval.c
>>> +++ b/xen/drivers/passthrough/vtd/qinval.c
>>> @@ -28,6 +28,11 @@
>>>  #include "vtd.h"
>>>  #include "extern.h"
>>>
>>> +static int __read_mostly iommu_qi_timeout_ms = 1;
>>> +integer_param("iommu_qi_timeout_ms", iommu_qi_timeout_ms);
>>> +
>>> +#define IOMMU_QI_TIMEOUT (iommu_qi_timeout_ms * MILLISECS(1))
>>> +
>>>  static void print_qi_regs(struct iommu *iommu)  {
>>>      u64 val;
>>> @@ -167,10 +172,12 @@ static int queue_invalidate_wait(struct iommu
>> *iommu,
>>>          start_time = NOW();
>>>          while ( poll_slot != QINVAL_STAT_DONE )
>>>          {
>>> -            if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
>>> +            if ( NOW() > (start_time + IOMMU_QI_TIMEOUT) )
>>>              {
>>>                  print_qi_regs(iommu);
>>> -                panic("queue invalidate wait descriptor was not
>> executed");
>>> +                dprintk(XENLOG_WARNING VTDPREFIX,
>>> +                        "Queue invalidate wait descriptor was
>> timeout.\n");
>>> +                return -ETIMEDOUT;
>>>              }
>>>              cpu_relax();
>>>          }
>> This patch misses a second use of DMAR_OPERATION_TIMEOUT, in
>> IOMMU_WAIT_OP() which in turn is used in a large number of locations.
>> All of these locations equally need to be chopped down to a low number of
>> milliseconds.
> Andrew, thanks for your comments.
>
> I know that DMAR_OPERATION_TIMEOUT should be also chopped down to a low number of milliseconds.
> As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We also confirmed with hardware team
> that 1ms is large enough for IOMMU internal flush.
> So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.

Ok - sounds good.

>
> IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is also a panic. We need a further discussion 
> whether or how to remove this panic.

We certainly should see about removing it.

> I can send another patch set to fix it. in this patch set, I want to focus on VT-d
> QI flush.

Agreed.

~Andrew