diff mbox series

[RFC,v4] x86/mm: Clean up p2m_finish_type_change return value

Message ID 20190327152107.29288-1-aisaila@bitdefender.com (mailing list archive)
State Superseded
Headers show
Series [RFC,v4] x86/mm: Clean up p2m_finish_type_change return value | expand

Commit Message

Isaila Alexandru March 27, 2019, 3:21 p.m. UTC
In the case of any errors, finish_type_change() passes values returned
from p2m->recalc() up the stack (with some exceptions in the case where
an error is expected); this eventually ends up being returned to the
XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.

However, on Intel processors (but not on AMD processor), p2m->recalc()
can also return '1' as well as '0'.  This case is handled very
inconsistently: finish_type_change() will return the value of the final
entry it attempts, discarding results for other entries;
p2m_finish_type_change() will attempt to accumulate '1's, so that it
returns '1' if any of the calls to finish_type_change() returns '1'; and
dm_op() will again return '1' only if the very last call to
p2m_finish_type_change() returns '1'.  The result is that the
XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
0 and sometimes return 1 on success, in an unpredictable manner.

The hypercall documentation doesn't mention return values; but it's not
clear what the caller could do with the information about whether
entries had been changed or not.  At the moment it's always 0 on AMD
boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
'1' return value for correctness (or if it is, it's broken).

Make the return value on success consistently '0' by only returning
0/-ERROR from finish_type_change().  Also remove the accumulation code
from p2m_finish_type_change().

Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V3:
	- Remove positive returned values from p2m->recalc.
---
 xen/arch/x86/mm/p2m-ept.c | 10 ++++++----
 xen/arch/x86/mm/p2m.c     | 15 +++++----------
 2 files changed, 11 insertions(+), 14 deletions(-)

Comments

Jan Beulich March 27, 2019, 4:07 p.m. UTC | #1
>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote:
> @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>  
>      p2m_unlock(p2m);
>  
> -    return spurious ? (rc >= 0) : (rc > 0);
> +    return spurious && !rc;
>  }

I think you've gone too far now: This is - afaict - the one place
where the distinction matters. Looking back at Paul's reply and
my subsequent one on v3, I'm afraid I've managed to misguide
you by not looking closely enough at what Paul did sketch out.
I'm sorry for this.

I think you either want to leave EPT code untouched, and zap
the positive return value in finish_type_change() instead. Or
EPT's resolve_misconfig() would need to gain a wrapper for use
as the ->recalc hook, to squash the positive value for the outside
world. Iirc I've avoided introducing such a wrapper originally
just to limit the number of functions layered on top of one
another, while using resolve_misconfig() directly appeared to
be fine.

> @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
>  
>      /* Carry out any eventually pending earlier changes first. */
>      ret = resolve_misconfig(p2m, gfn);
> -    if ( ret < 0 )
> +    if ( ret )
>          return ret;

This would then need undoing as well.

> @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d,
>  
>      rc = finish_type_change(hostp2m, first_gfn, max_nr);
>  
> -    if ( rc < 0 )
> +    if ( rc )
>          goto out;

While I don't really object to this change, I also don't think it's
strictly necessary.

Jan
George Dunlap March 27, 2019, 4:44 p.m. UTC | #2
On 3/27/19 3:21 PM, Alexandru Stefan ISAILA wrote:
> In the case of any errors, finish_type_change() passes values returned
> from p2m->recalc() up the stack (with some exceptions in the case where
> an error is expected); this eventually ends up being returned to the
> XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
> 
> However, on Intel processors (but not on AMD processor), p2m->recalc()
> can also return '1' as well as '0'.  This case is handled very
> inconsistently: finish_type_change() will return the value of the final
> entry it attempts, discarding results for other entries;
> p2m_finish_type_change() will attempt to accumulate '1's, so that it
> returns '1' if any of the calls to finish_type_change() returns '1'; and
> dm_op() will again return '1' only if the very last call to
> p2m_finish_type_change() returns '1'.  The result is that the
> XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> 0 and sometimes return 1 on success, in an unpredictable manner.
> 
> The hypercall documentation doesn't mention return values; but it's not
> clear what the caller could do with the information about whether
> entries had been changed or not.  At the moment it's always 0 on AMD
> boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> '1' return value for correctness (or if it is, it's broken).
> 
> Make the return value on success consistently '0' by only returning
> 0/-ERROR from finish_type_change().  Also remove the accumulation code
> from p2m_finish_type_change().

Thanks for putting in the effort to clean this up.

One comment: this is the second instance of the patch you posted where
this paragraph is not true.  What I wrote was meant to be an example of
how a good patch description should be written, which includes a sketch
of what the patch does technically.  You need to make sure the sketch
matches the patch.

As it happens, it looks like you'll have to modify the patch such that
v5 actually matches the description again. :-)

Also, you probably should have dropped the RFC at v2.

 -George
Isaila Alexandru March 28, 2019, 9:30 a.m. UTC | #3
On 27.03.2019 18:07, Jan Beulich wrote:
>>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote:
>> @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>>   
>>       p2m_unlock(p2m);
>>   
>> -    return spurious ? (rc >= 0) : (rc > 0);
>> +    return spurious && !rc;
>>   }
> 
> I think you've gone too far now: This is - afaict - the one place
> where the distinction matters. Looking back at Paul's reply and
> my subsequent one on v3, I'm afraid I've managed to misguide
> you by not looking closely enough at what Paul did sketch out.
> I'm sorry for this.
> 
> I think you either want to leave EPT code untouched, and zap
> the positive return value in finish_type_change() instead. Or
> EPT's resolve_misconfig() would need to gain a wrapper for use
> as the ->recalc hook, to squash the positive value for the outside
> world. Iirc I've avoided introducing such a wrapper originally
> just to limit the number of functions layered on top of one
> another, while using resolve_misconfig() directly appeared to
> be fine.
> It's alright, it's an honest mistake and in this case I will go back and 
have finish_type_change() cut the positive value if that is ok with 
everyone.

Regards,
Alex

>> @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
>>   
>>       /* Carry out any eventually pending earlier changes first. */
>>       ret = resolve_misconfig(p2m, gfn);
>> -    if ( ret < 0 )
>> +    if ( ret )
>>           return ret;
> 
> This would then need undoing as well.
> 
>> @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d,
>>   
>>       rc = finish_type_change(hostp2m, first_gfn, max_nr);
>>   
>> -    if ( rc < 0 )
>> +    if ( rc )
>>           goto out;
> 
> While I don't really object to this change, I also don't think it's
> strictly necessary.
>
Isaila Alexandru March 28, 2019, 9:36 a.m. UTC | #4
On 27.03.2019 18:07, Jan Beulich wrote:
>>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote:
>> @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>>   
>>       p2m_unlock(p2m);
>>   
>> -    return spurious ? (rc >= 0) : (rc > 0);
>> +    return spurious && !rc;
>>   }
> 
> I think you've gone too far now: This is - afaict - the one place
> where the distinction matters. Looking back at Paul's reply and
> my subsequent one on v3, I'm afraid I've managed to misguide
> you by not looking closely enough at what Paul did sketch out.
> I'm sorry for this.
> 
> I think you either want to leave EPT code untouched, and zap
> the positive return value in finish_type_change() instead. Or
> EPT's resolve_misconfig() would need to gain a wrapper for use
> as the ->recalc hook, to squash the positive value for the outside
> world. Iirc I've avoided introducing such a wrapper originally
> just to limit the number of functions layered on top of one
> another, while using resolve_misconfig() directly appeared to
> be fine.
> 

Sorry the last reply got mixed up this was my reply:

It's alright, it's an honest mistake and in this case I will go back and
have finish_type_change() cut the positive value if that is ok with
everyone.

Regards,
Alex


>> @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
>>   
>>       /* Carry out any eventually pending earlier changes first. */
>>       ret = resolve_misconfig(p2m, gfn);
>> -    if ( ret < 0 )
>> +    if ( ret )
>>           return ret;
> 
> This would then need undoing as well.
> 
>> @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d,
>>   
>>       rc = finish_type_change(hostp2m, first_gfn, max_nr);
>>   
>> -    if ( rc < 0 )
>> +    if ( rc )
>>           goto out;
> 
> While I don't really object to this change, I also don't think it's
> strictly necessary.
>
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index e3044bee2e..d336c138b0 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -459,8 +459,7 @@  static int ept_invalidate_emt_range(struct p2m_domain *p2m,
  *   for entries not involved in the translation of the given GFN.
  * Returns:
  * - negative errno values in error,
- * - zero if no adjustment was done,
- * - a positive value if at least one adjustment was done.
+ * - zero for ok
  */
 static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
 {
@@ -600,6 +599,9 @@  static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
             v->arch.hvm.vmx.ept_spurious_misconfig = 1;
     }
 
+    if ( rc > 0 )
+        rc = 0;
+
     return rc;
 }
 
@@ -621,7 +623,7 @@  bool_t ept_handle_misconfig(uint64_t gpa)
 
     p2m_unlock(p2m);
 
-    return spurious ? (rc >= 0) : (rc > 0);
+    return spurious && !rc;
 }
 
 /*
@@ -668,7 +670,7 @@  ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
 
     /* Carry out any eventually pending earlier changes first. */
     ret = resolve_misconfig(p2m, gfn);
-    if ( ret < 0 )
+    if ( ret )
         return ret;
 
     ASSERT((target == 2 && hap_has_1gb) ||
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d5690b96bf 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1172,7 +1172,7 @@  static int finish_type_change(struct p2m_domain *p2m,
     {
         rc = p2m->recalc(p2m, gfn);
         /*
-         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
+         * ept->recalc could return 0/-ENOMEM. pt->recalc could return
          * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
          * gfn here.
          */
@@ -1201,7 +1201,7 @@  int p2m_finish_type_change(struct domain *d,
 
     rc = finish_type_change(hostp2m, first_gfn, max_nr);
 
-    if ( rc < 0 )
+    if ( rc )
         goto out;
 
 #ifdef CONFIG_HVM
@@ -1213,22 +1213,17 @@  int p2m_finish_type_change(struct domain *d,
             if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             {
                 struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
-                int rc1;
 
                 p2m_lock(altp2m);
-                rc1 = finish_type_change(altp2m, first_gfn, max_nr);
+                rc = finish_type_change(altp2m, first_gfn, max_nr);
                 p2m_unlock(altp2m);
 
-                if ( rc1 < 0 )
-                {
-                    rc = rc1;
+                if ( rc < 0 )
                     goto out;
-                }
-
-                rc |= rc1;
             }
     }
 #endif
+    rc = 0;
 
  out:
     p2m_unlock(hostp2m);