[PATCH v2] x86/mm: Clean up p2m_finish_type_change return value

[PATCH v2] x86/mm: Clean up p2m_finish_type_change return value

[Alexandru Stefan ISAILA]

Changed the return value of 1 to 0 so now p2m_finish_type_change returns
0 for success or <0 for error.
The “root” caller of p2m_finish_type_change() is
XEN_DMOP_map_mem_type_to_ioreq_server and this does nothing useful with
positive values.

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

---
Changes since V1:
	- Update commit message
	- Move rc = 0 before out tag.
---
 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 d14ce57dd5..a9c366bb5e 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1188,22 +1188,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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Clean up p2m_finish_type_change return value

[Jan Beulich]

>>> On 17.01.19 at 10:06, <aisaila@bitdefender.com> wrote:
> Changed the return value of 1 to 0 so now p2m_finish_type_change returns
> 0 for success or <0 for error.
> The “root” caller of p2m_finish_type_change() is
> XEN_DMOP_map_mem_type_to_ioreq_server and this does nothing useful with
> positive values.

By "root" I supposed you mean "only current"? I have to admit that
I'm only half way happy with this as the underlying reason. I would
expect for there to have been a reason for the return value being
this way, and that's what would need to be reasoned against.
Furthermore the question then is whether down the call tree the
same adjustment shouldn't be made. After all (and possibly just as
a first step) why would finish_type_change() return a positive
value if its only caller doesn't care?

But anyway, I'm not the maintainer here, so what I say may well
be completely ignored.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Clean up p2m_finish_type_change return value

[Alexandru Stefan ISAILA]

Ping. Is this ok with you, George?

Regards,
Alex

On 17.01.2019 11:06, Alexandru Stefan ISAILA wrote:
> Changed the return value of 1 to 0 so now p2m_finish_type_change returns
> 0 for success or <0 for error.
> The “root” caller of p2m_finish_type_change() is
> XEN_DMOP_map_mem_type_to_ioreq_server and this does nothing useful with
> positive values.
> 
> Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V1:
> 	- Update commit message
> 	- Move rc = 0 before out tag.
> ---
>   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 d14ce57dd5..a9c366bb5e 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1188,22 +1188,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);
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Clean up p2m_finish_type_change return value

[George Dunlap]

On 2/26/19 9:17 AM, Alexandru Stefan ISAILA wrote:
> Ping. Is this ok with you, George?

Sorry -- somehow I thought I'd responded to this, but apparently not.

> On 17.01.2019 11:06, Alexandru Stefan ISAILA wrote:
>> Changed the return value of 1 to 0 so now p2m_finish_type_change returns
>> 0 for success or <0 for error.
>> The “root” caller of p2m_finish_type_change() is
>> XEN_DMOP_map_mem_type_to_ioreq_server and this does nothing useful with
>> positive values.

I realize you're just copying what I said in the email where I suggested
this, but as Jan pointed out in v1, commit message needs to be more
thorough than an off-hand comment during review.  I also agree with him
that it makes more sense to 'sanitize' ept's recalc_entry() return value
in finish_type_change(), rather than at p2m_finish_type_change().

I think it's probably good to get feedback from Paul, who wrote this
interface (IIRC) and works with one of its main users (QEMU).

My normal 'template' is "Situation / Problem / Solution": What's the
current situation, why is that a problem, and how does this patch fix it.

With that in mind (and also bringing Paul up to speed), I think I'd say
something like this:

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

Thoughts?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel