[PATCH for-4.5 v2] x86/hvm: remove stray ioreq_server.lock lock release from hvm_ioreq_server_init()

[PATCH for-4.5 v2] x86/hvm: remove stray ioreq_server.lock lock release from hvm_ioreq_server_init()

[Vitaly Kuznetsov]

If HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, or HVM_PARAM_BUFIOREQ_EVTCHN
parameters are read when guest domain is dying it leads to the following
ASSERT:

(XEN) Assertion '_raw_spin_is_locked(lock)' failed at ...workspace/KERNEL/xen/xen/include/asm/spinlock.h:18
(XEN) ----[ Xen-4.5-unstable  x86_64  debug=y  Not tainted ]----
...
(XEN) Xen call trace:
(XEN)    [<ffff82d08012b07f>] _spin_unlock+0x27/0x30
(XEN)    [<ffff82d0801b6103>] hvm_create_ioreq_server+0x3df/0x49a
(XEN)    [<ffff82d0801bcceb>] do_hvm_op+0x12bf/0x27a0
(XEN)    [<ffff82d08022b9bb>] syscall_enter+0xeb/0x145

The root cause of this issue is the fact that ioreq_server.lock is being
released twice - first in hvm_ioreq_server_init() and then in hvm_create_ioreq_server().
Drop the lock release from hvm_ioreq_server_init() as we don't take it here.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes from v1:
- Instead of protecting agains creating ioreq server while guest domain
  is dying remove stray ioreq_server.lock lock release
  from hvm_ioreq_server_init(). Rename the patch accordingly.
  [Paul Durrant]
---
 xen/arch/x86/hvm/hvm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0a20cbe..1587858 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -993,7 +993,6 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain *d,
     hvm_ioreq_server_free_rangesets(s, is_default);
 
  fail1:
-    spin_unlock(&d->arch.hvm_domain.ioreq_server.lock);
     return rc;
 }
 
-- 
1.9.3

Re: [PATCH for-4.5 v2] x86/hvm: remove stray ioreq_server.lock lock release from hvm_ioreq_server_init()

[Jan Beulich]

>>> On 26.09.14 at 15:27, <vkuznets@redhat.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -993,7 +993,6 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain *d,
>      hvm_ioreq_server_free_rangesets(s, is_default);
>  
>   fail1:
> -    spin_unlock(&d->arch.hvm_domain.ioreq_server.lock);
>      return rc;
>  }

I'd really like to see the fail1 label gone with this, perhaps at once
renaming the others to "fail_map" and "fail_add". And please also
shorten the title to e.g. "x86/hvm: remove stray lock release from
hvm_ioreq_server_init()" - after all there's only one lock being
released there right now, so this is unambiguous.

Jan