diff mbox series

[v3] x86/mm: Clean up p2m_finish_type_change return value

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

Commit Message

Isaila Alexandru March 22, 2019, 12:09 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 V2:
	- Update commit msg.
---
 xen/arch/x86/mm/p2m.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Paul Durrant March 25, 2019, 9:09 a.m. UTC | #1
> -----Original Message-----
> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
> Sent: 22 March 2019 12:09
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Alexandru Stefan ISAILA <aisaila@bitdefender.com>
> Subject: [PATCH v3] x86/mm: Clean up p2m_finish_type_change return value
> 
> 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().

Sorry, I don't think I was cc-ed on the original and I managed to miss George cc-ing me on his response to v2. Digging into the code a bit I can't honestly see what the point of returning anything other than 0/-errno out of p2m->recalc is. The only use of rc > 0 from p2m-ept.c:resolve_misconfig() is in ept_handle_misconfig() AFAICT so it would make more sense to me to tighten up the semantics of recalc (which I believe Jan suggested in response to v1) and turn any > 0 return from resolve_misconfig() into 0. So, the code below looks fine but the patch just needs to do a little more (and then your rc < 0 test can also be simplified to !rc too).

  Paul

> 
> Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V2:
> 	- Update commit msg.
> ---
>  xen/arch/x86/mm/p2m.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..bcf8cad423 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -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);
> --
> 2.17.1
Jan Beulich March 25, 2019, 1:37 p.m. UTC | #2
>>> On 25.03.19 at 10:09, <Paul.Durrant@citrix.com> wrote:
>> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
>> Sent: 22 March 2019 12:09
>> 
>> 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().
> 
> Sorry, I don't think I was cc-ed on the original and I managed to miss George 
> cc-ing me on his response to v2. Digging into the code a bit I can't honestly 
> see what the point of returning anything other than 0/-errno out of p2m->recalc 
> is. The only use of rc > 0 from p2m-ept.c:resolve_misconfig() is in 
> ept_handle_misconfig() AFAICT so it would make more sense to me to tighten up 
> the semantics of recalc (which I believe Jan suggested in response to v1) and 
> turn any > 0 return from resolve_misconfig() into 0. So, the code below looks 
> fine but the patch just needs to do a little more (and then your rc < 0 test 
> can also be simplified to !rc too).

Yes, indeed I'd prefer everything to be cleaned up in one go.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..bcf8cad423 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -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);