Re: [Xen-devel] [PATCH v5 1/7] VT-d: Check VT-d Device-TLB flush error(IOMMU part).

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.16 at 13:49, <quan.xu@xxxxxxxxx> wrote:
>>  On February 16, 2016 7:06pm, <JBeulich@xxxxxxxx> wrote:
>> >>> On 16.02.16 at 11:50, <quan.xu@xxxxxxxxx> wrote:
>> >>  On February 11, 2016 at 1:01am, <JBeulich@xxxxxxxx> wrote:
>> >> >>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
>  
>> >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain*
>> d)
>> >> >          ops->share_p2m(d);
>> >> >  }
>> >> >
>> >> > -void iommu_crash_shutdown(void)
>> >> > +int iommu_crash_shutdown(void)
>> >> >  {
>> >> >      const struct iommu_ops *ops = iommu_get_ops();
>> >> > +
>> >> >      if ( iommu_enabled )
>> >> > -        ops->crash_shutdown();
>> >> > +        return ops->crash_shutdown();
>> >> > +
>> >> >      iommu_enabled = iommu_intremap = 0;
>> >> > +
>> >> > +    return 0;
>> >> >  }
>> >>
>> >> Here again the question is - what is the error value going to be used
>> >> for? We're trying to shut down a crashed system when coming here.
>> >>
>> >  I tried to clean up in error handling path chained up. It logs an
>> > error message,  When it calls iommu_crash_shutdown() and returns a
>> > non-zero value [in patch 2/7].
>> 
>> That sounds okay than (I didn't get around to look at patches 2-7 yet), but 
> is
>> somewhat contrary to me request of adding __must_check as far as possible,
>> which - if done here - would break the build without also adjusting the 
> caller(s).
>> 
> 
> 
> If they are in the same patch set, I think it is acceptable. 

If unavoidable, yes. But please remember that patch series don't
necessarily get committed in one go.

> BTW, with patch 1/7, I can build Xen successfully( make xen ).
> To align this rule, I'd better merge patch1/7 and patch 2/7 into a large 
> patch.

Not having looked at patch 2 yet (I'm about to), I can't tell whether
this makes sense. I'd rather not see large patches become even
larger though - please make a reasonable attempt at splitting things
(in the case here for example by adjusting top level functions first,
working your way down to leaf ones, thus guaranteeing that newly
added __must_check annotations will be properly honored at each
patch boundary).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel