Re: [Xen-devel] [PATCH v5 01/10] vt-d: fix the IOMMU flush issue

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

On May 25, 2016 4:30 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 25.05.16 at 10:04, <quan.xu@xxxxxxxxx> wrote:
> > On May 23, 2016 11:43 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 23.05.16 at 17:22, <quan.xu@xxxxxxxxx> wrote:
> >> > On May 23, 2016 9:31 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >> >>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> > @@ -557,14 +557,16 @@ static void iommu_flush_all(void)
> >> >> >      }
> >> >> >  }
> >> >> >
> >> >> > -static void __intel_iommu_iotlb_flush(struct domain *d,
> >> >> > unsigned long
> >> gfn,
> >> >> > -        int dma_old_pte_present, unsigned int page_count)
> >> >> > +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned
> >> >> > +long
> >> gfn,
> >> >> > +                                     bool_t dma_old_pte_present,
> >> >> > +                                     unsigned int page_count)
> >> >>
> >> >> I realize you say so in the overview mail, but the continuing lack
> >> >> of __must_check here causes review trouble again. And I have a
> >> >> hard time seeing how adding these annotations right away would
> >> >> "disrupt the order", as long as the series is properly ordered / broken 
> >> >> up.
> >> >>
> >> >
> >> > If I add __must_check annotations here right now, e.g.
> >> >
> >> > -static void intel_iommu_iotlb_flush()
> >> > +static int __must_check iommu_flush_iotlb_pages()
> >> >
> >> > ...
> >> >
> >> > @@ -179,8 +179,9 @@ struct iommu_ops {
> >> > -    void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned 
> >> > int
> >> > page_count);
> >> > +    int __must_check (*iotlb_flush)(struct domain *d, unsigned
> >> > + long gfn,
> >> > unsigned int page_count);
> >> > ...
> >> > }
> >> >
> >> > Should belong  to here too.
> >>
> >> Correct. And where's the problem?
> >>
> >
> > IMO it is not a big deal..
> >
> > I think this makes this patch 1 fat.. why not focus on the positive
> > propagation value from IOMMU flush interfaces in this patch.
> > If we are clear I will add annotation and rename them in another
> > patches, it is acceptable to me.
> 
> The patch getting too large is easy to deal with: Split it at a reasonable
> boundary. It's one thing that I want to be clear: Any conversion of a void
> return type of some function to non-void should be accompanied by it at the
> same time becoming __must_check. I dislike having to repeat yet again what I
> have been saying a number of times: Without doing so, it is harder for you as
> the person writing the patch to verify all callers deal with errors, and it's
> perhaps even harder for reviewers to verify you did.
> 

It is clear to me, but I was lock of attention on reviewers part.
I'll follow your suggestion from now on.. thanks.

Quan


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