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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 14.12.15 at 09:31, <quan.xu@intel.com> wrote:
>>  On 14.12.2015 at 4:19pm, <JBeulich@suse.com> wrote:
>> >>> On 12.12.15 at 10:03, <quan.xu@intel.com> wrote:
>> >> On 11.12.2015 at 6:01pm, <JBeulich@suse.com> wrpte:
>> >> >>> On 10.12.15 at 10:33, <quan.xu@intel.com> wrote:
>> >> > @@ -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;
>> >> >              }
>> >>
>> >> I don't see such a change be valid without making sure callers
>> >> actually honor errors. For example, no caller of
>> >> iommu_flush_iec_{global,index}() cares to check. And not even your second
>> patch addresses this (i.e.
>> >> it's also not just bad patch ordering).
>> >>
>> >
>> > I check it again.
>> > For iommu_flush_iec_{global,index}()  are both call __iommu_flush_iec().
>> > In my patch, I have check it in __iommu_flush_iec().
>> > I think it does not need to check it in
>> > iommu_flush_iec_{global,index}() again. Right?
>> 
>> No, not in the v2 version of it. While you call invalidate_timeout() in the
>> -ETIMEDOUT case, you still pass on the error code, and hence the callers need
>> to also either pass it on or deal with it.
> 
> Jan, sorry for that. I still don't get the point.
> Check it again :(:(
> 
> Should I also check it where call iommu_flush_iec_{global,index}(), if it is 
> -ETIMEDOUT, it should return or deal with it??

You should check for _any_ kind of error here. I.e. the problem
doesn't get introduced by your patches, it only gets made worse
(by virtue of the fact that the only error queue_invalidate_wait()
may return before your patch is for when calling code sets a flag
it isn't currently supposed to set, i.e. not handling errors right
now is pretty much benign), and hence properly dealing with
errors here would probably best go into a prereq patch.

Jan