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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> On February 17, 2016 9:05pm, <JBeulich@xxxxxxxx> wrote:
> >>> 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), 

Good news. :):)
I think there are maybe some minor issues for this patch set ( _my_estimate_), 
and I will address these issues immediately.


> 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).
> 

Okay, I will try to do it in v6. Thanks.


Quan

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