Re: [Xen-devel] [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

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

On April 27, 2016 2:32 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 27.04.16 at 08:21, <quan.xu@xxxxxxxxx> wrote:
> > On April 26, 2016 8:49 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> Hmm, the "positive" here has nothing to do with the "positive" in patch 1.
> >> Please just have a look at xenmem_add_to_physmap() as a whole.
> >>
> >
> > Thanks for reminding me. The 'positive'  is ' rc = start + done'..
> >
> > Think twice, I found I need two other new return values for this
> > function (correct me if I am wrong!).
> > If the first iommu_iotlb_flush() is failed, I shouldn't  accumulate
> > the return value of the second iommu_iotlb_flush() -- but instead
> > properly accumulate the return value.
> 
> I think just one will do.
> 

Got it.

> > The following is my modification:
> >
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -640,6 +640,10 @@ static int xenmem_add_to_physmap(struct
> domain *d,
> >      unsigned int done = 0;
> >      long rc = 0;
> >
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    int ret, rv;
> > +#endif
> 
> This should go into the scope below, allowing to avoid the extra #ifdef.
> 


Agreed .

> > @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct
> domain *d,
> >      if ( need_iommu(d) )
> >      {

Check it again.  The 'int ret'  declaration should be here? 


> >          this_cpu(iommu_dont_flush_iotlb) = 0;
> > -        iommu_iotlb_flush(d, xatp->idx - done, done);
> > -        iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +        ret = iommu_iotlb_flush(d, xatp->idx - done, done);
> > +
> > +        if ( rc >= 0 && ret < 0 )
> 
> Why "ret < 0" instead of just "ret"?
> 

"ret" is enough.


> > +            rc = ret;
> > +
> > +        rv = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +
> > +        if ( rc >=0 && ret == 0 && rv < 0 )
> > +            rc = rv;
> 
> I don't see the middle part being needed here, eliminating the need for "rv".
> 

 I will drop 'rv'.

Quan



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