x86/HAP: avoid using bogus/misleading locking

Commit Message

Jan Beulich May 29, 2017, 12:37 p.m. UTC

hap_teardown() unconditionally releases the paging lock and is always
being called without the lock held: Lock acquire should then be
unconditional too.

Signed-off-by: Jan Beulich <[e-mail]>
---
While this is only a cosmetic change afaict I would still like to
explore whether to include this in 4.9.
x86/HAP: avoid using bogus/misleading locking

hap_teardown() unconditionally releases the paging lock and is always
being called without the lock held: Lock acquire should then be
unconditional too.

Signed-off-by: Jan Beulich <[e-mail]>
---
While this is only a cosmetic change afaict I would still like to
explore whether to include this in 4.9.

--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -576,8 +576,7 @@ void hap_teardown(struct domain *d, bool
     ASSERT(d->is_dying);
     ASSERT(d != current->domain);
 
-    if ( !paging_locked_by_me(d) )
-        paging_lock(d); /* Keep various asserts happy */
+    paging_lock(d); /* Keep various asserts happy */
 
     if ( paging_mode_enabled(d) )
     {

Comments

Andrew Cooper May 29, 2017, 1:55 p.m. UTC | #1

On 29/05/2017 13:37, Jan Beulich wrote:
> hap_teardown() unconditionally releases the paging lock and is always
> being called without the lock held: Lock acquire should then be
> unconditional too.
>
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Andrew Cooper <[e-mail]>

Julien Grall May 31, 2017, 9:03 a.m. UTC | #2

Hi,

On 05/29/2017 02:55 PM, Andrew Cooper wrote:
> On 29/05/2017 13:37, Jan Beulich wrote:
>> hap_teardown() unconditionally releases the paging lock and is always
>> being called without the lock held: Lock acquire should then be
>> unconditional too.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>
> Reviewed-by: Andrew Cooper <[e-mail]>

Assuming this goes through staging first:

Release-acked-by: Julien Grall <[e-mail]>

Cheers,

Jan Beulich May 31, 2017, 9:12 a.m. UTC | #3

>>> On 31.05.17 at 11:03, <[e-mail]> wrote:
> On 05/29/2017 02:55 PM, Andrew Cooper wrote:
>> On 29/05/2017 13:37, Jan Beulich wrote:
>>> hap_teardown() unconditionally releases the paging lock and is always
>>> being called without the lock held: Lock acquire should then be
>>> unconditional too.
>>>
>>> Signed-off-by: Jan Beulich <[e-mail]>
>>
>> Reviewed-by: Andrew Cooper <[e-mail]>
> 
> Assuming this goes through staging first:

Of course, but strictly speaking this still needs an ack from George
before it can go in anywhere.

Jan

> Release-acked-by: Julien Grall <[e-mail]>
> 
> Cheers,
> 
> -- 
> Julien Grall

Jan Beulich June 6, 2017, 3:50 p.m. UTC | #4

>>> On 29.05.17 at 14:37, <[e-mail]> wrote:
> hap_teardown() unconditionally releases the paging lock and is always
> being called without the lock held: Lock acquire should then be
> unconditional too.
> 
> Signed-off-by: Jan Beulich <[e-mail]>
> ---
> While this is only a cosmetic change afaict I would still like to
> explore whether to include this in 4.9.
> 
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -576,8 +576,7 @@ void hap_teardown(struct domain *d, bool
>      ASSERT(d->is_dying);
>      ASSERT(d != current->domain);
>  
> -    if ( !paging_locked_by_me(d) )
> -        paging_lock(d); /* Keep various asserts happy */
> +    paging_lock(d); /* Keep various asserts happy */
>  
>      if ( paging_mode_enabled(d) )
>      {

Patch

--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -576,8 +576,7 @@  void hap_teardown(struct domain *d, bool
     ASSERT(d->is_dying);
     ASSERT(d != current->domain);
 
-    if ( !paging_locked_by_me(d) )
-        paging_lock(d); /* Keep various asserts happy */
+    paging_lock(d); /* Keep various asserts happy */
 
     if ( paging_mode_enabled(d) )
     {