[PATCH for-4.10] common/multicall: Increase debugability for bad hypercalls

[PATCH for-4.10] common/multicall: Increase debugability for bad hypercalls

[Andrew Cooper]

While investigating an issue (in a new codepath I'd introduced, as it turns
out), leaving interrupts disabled manifested as a subsequent op in the
multicall failing a check_lock() test.

The codepath would have hit the ASSERT_NOT_IN_ATOMIC on the return-to-guest
path, had it not hit the check_lock() first.

Call ASSERT_NOT_IN_ATOMIC() after each operation in the multicall, to make
failures more obvious.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

As with the related check_lock() patch, this only affects debug builds, so is
a very low risk change for 4.10
---
 xen/common/multicall.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index c7af4e0..d98e59d 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -66,6 +66,13 @@ do_multicall(
 
         disp = arch_do_multicall_call(mcs);
 
+        /*
+         * In the unlikley event that a hypercall has left interrupts,
+         * spinlocks, or other things in a bad way, continuting the multicall
+         * will typically lead to far more subtle issues to debug.
+         */
+        ASSERT_NOT_IN_ATOMIC();
+
 #ifndef NDEBUG
         {
             /*
-- 
2.1.4


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

Re: [PATCH for-4.10] common/multicall: Increase debugability for bad hypercalls

[George Dunlap]

On 10/31/2017 05:18 PM, Andrew Cooper wrote:
> While investigating an issue (in a new codepath I'd introduced, as it turns
> out), leaving interrupts disabled manifested as a subsequent op in the
> multicall failing a check_lock() test.
> 
> The codepath would have hit the ASSERT_NOT_IN_ATOMIC on the return-to-guest
> path, had it not hit the check_lock() first.
> 
> Call ASSERT_NOT_IN_ATOMIC() after each operation in the multicall, to make
> failures more obvious.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [PATCH for-4.10] common/multicall: Increase debugability for bad hypercalls

[Wei Liu]

On Tue, Oct 31, 2017 at 05:18:52PM +0000, Andrew Cooper wrote:
> While investigating an issue (in a new codepath I'd introduced, as it turns
> out), leaving interrupts disabled manifested as a subsequent op in the
> multicall failing a check_lock() test.
> 
> The codepath would have hit the ASSERT_NOT_IN_ATOMIC on the return-to-guest
> path, had it not hit the check_lock() first.
> 
> Call ASSERT_NOT_IN_ATOMIC() after each operation in the multicall, to make
> failures more obvious.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.10] common/multicall: Increase debugability for bad hypercalls

[Julien Grall]

Hi Andrew,

On 31/10/17 17:18, Andrew Cooper wrote:
> While investigating an issue (in a new codepath I'd introduced, as it turns
> out), leaving interrupts disabled manifested as a subsequent op in the
> multicall failing a check_lock() test.
> 
> The codepath would have hit the ASSERT_NOT_IN_ATOMIC on the return-to-guest
> path, had it not hit the check_lock() first.
> 
> Call ASSERT_NOT_IN_ATOMIC() after each operation in the multicall, to make
> failures more obvious.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> As with the related check_lock() patch, this only affects debug builds, so is
> a very low risk change for 4.10

Release-acked-by: Julien Grall <julien.grall@linaro.org>

With a couple of typos below.

> ---
>   xen/common/multicall.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index c7af4e0..d98e59d 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -66,6 +66,13 @@ do_multicall(
>   
>           disp = arch_do_multicall_call(mcs);
>   
> +        /*
> +         * In the unlikley event that a hypercall has left interrupts,

s/unlikley/unlikely/

> +         * spinlocks, or other things in a bad way, continuting the multicall

s/continuting/continuing/

> +         * will typically lead to far more subtle issues to debug.
> +         */
> +        ASSERT_NOT_IN_ATOMIC();
> +
>   #ifndef NDEBUG
>           {
>               /*
> 

Cheers,

-- 
Julien Grall

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