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

Commit Message

Quan Xu April 27, 2016, 6:21 a.m. UTC

On April 26, 2016 8:49 PM, Jan Beulich <[e-mail]> wrote:
> >>> On 26.04.16 at 14:23, <[e-mail]> wrote:
> > On April 25, 2016 6:19 PM,  Jan Beulich <[e-mail]> wrote:
> >> >>> On 18.04.16 at 16:00, <[e-mail]> wrote:
> >> > --- a/xen/common/memory.c
> >> > +++ b/xen/common/memory.c
> >> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct
> domain
> >> *d,
> >> >      if ( need_iommu(d) )
> >> >      {
> >> >          this_cpu(iommu_dont_flush_iotlb) = 0;
> >> > -        iommu_iotlb_flush(d, xatp->idx - done, done);
> >> > -        iommu_iotlb_flush(d, xatp->gpfn - done, done);
> >> > +        rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> >> > +        if ( !rc )
> >> > +            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> >>
> >> And again - best effort flushing in all cases. I.e. you shouldn't
> >> bypass the second one if the first one failed, but instead properly
> >> accumulate the return value, and also again not clobber any negative value
> rc may already hold.
> >
> >
> > Thanks for correcting me. I did like accumulating the return value.
> > :( I will follow your suggestion in next v3.
> >
> >> What's worse (and makes me wonder whether this got tested) - you also
> >> clobber the positive return value this function may produce, even in
> >> the case the flushes succeed (and hence the function as a whole was
> successful).
> >>
> > I have tested it with previous patches (i.e.  1st patch), launching a
> > VM with PT ATS device to invoke this call tree.
> > btw, I fix the positive issue in 1st patch.
> > I know this is not strict, as I assume this patch set will be
> > committed at the same time.
> 
> 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.

The following is my modification:


Quan

Comments

Jan Beulich April 27, 2016, 6:31 a.m. UTC | #1

>>> On 27.04.16 at 08:21, <[e-mail]> wrote:
> On April 26, 2016 8:49 PM, Jan Beulich <[e-mail]> 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.

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

> @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct domain *d,
>      if ( need_iommu(d) )
>      {
>          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"?

> +            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".

Jan

Quan Xu April 27, 2016, 9:08 a.m. UTC | #2

On April 27, 2016 2:32 PM, Jan Beulich <[e-mail]> wrote:
> >>> On 27.04.16 at 08:21, <[e-mail]> wrote:
> > On April 26, 2016 8:49 PM, Jan Beulich <[e-mail]> 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

Jan Beulich April 27, 2016, 9:38 a.m. UTC | #3

>>> On 27.04.16 at 11:08, <[e-mail]> wrote:
> On April 27, 2016 2:32 PM, Jan Beulich <[e-mail]> wrote:
>> >>> On 27.04.16 at 08:21, <[e-mail]> wrote:
>> > @@ -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? 

Yes, exactly.

Patch

--- 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
+
     if ( xatp->space != XENMAPSPACE_gmfn_range )
         return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
                                          xatp->idx, xatp->gpfn);
@@ -678,8 +682,15 @@  static int xenmem_add_to_physmap(struct domain *d,
     if ( need_iommu(d) )
     {
         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 )
+            rc = ret;
+
+        rv = iommu_iotlb_flush(d, xatp->gpfn - done, done);
+
+        if ( rc >=0 && ret == 0 && rv < 0 )
+            rc = rv;
     }
 #endif