Re: [Xen-devel] [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains

[Tim Deegan <tim@xxxxxxx>]

At 12:53 +0200 on 15 Oct (1413374024), Roger Pau Monne wrote:
> Due to locking order, the p2m lock must be taken before the paging lock, or
> else the following panic occurs when trying to use logdirty ops from a PVH
> Dom0:
> 
> (XEN) mm locking order violation: 292 > 222
> (XEN) Xen BUG at mm-locks.h:140
> (XEN) ----[ Xen-4.5-unstable  x86_64  debug=y  Not tainted ]----
> (XEN) CPU:    1
> (XEN) RIP:    e008:[<ffff82d0801e9ea5>] get_page_from_gfn_p2m+0xb0/0x286
> (XEN) RFLAGS: 0000000000010282   CONTEXT: hypervisor
> (XEN) rax: 0000000000000000   rbx: ffff83019a1f7884   rcx: 0000000000000000
> (XEN) rdx: ffff83019a1f0000   rsi: 000000000000000a   rdi: ffff82d0802926c0
> (XEN) rbp: ffff83019a1f77f8   rsp: ffff83019a1f7798   r8:  ffff83019e830000
> (XEN) r9:  0000000000000003   r10: 00000000000000de   r11: 0000000000000003
> (XEN) r12: ffff83019a1f77c4   r13: ffff83019a138820   r14: ffff83019a1f7974
> (XEN) r15: 0000000000057431   cr0: 0000000080050033   cr4: 00000000000026f0
> (XEN) cr3: 000000019ea86000   cr2: 000000080205d000
> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
> [...]
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0801e9ea5>] get_page_from_gfn_p2m+0xb0/0x286
> (XEN)    [<ffff82d0802223e9>] hap_p2m_ga_to_gfn_4_levels+0x59/0x2b7
> (XEN)    [<ffff82d080222663>] hap_gva_to_gfn_4_levels+0x1c/0x29
> (XEN)    [<ffff82d0801edf67>] paging_gva_to_gfn+0xb8/0xce
> (XEN)    [<ffff82d0801b9bc0>] clear_user_hvm+0xd7/0x324
> (XEN)    [<ffff82d0801e8776>] paging_log_dirty_op+0x358/0x552
> (XEN)    [<ffff82d0801e8d47>] paging_domctl+0x140/0x177
> (XEN)    [<ffff82d08015ccca>] arch_do_domctl+0x212/0x269e
> (XEN)    [<ffff82d08010487c>] do_domctl+0x195d/0x1cd1
> (XEN)    [<ffff82d0801bafba>] hvm_do_hypercall+0x1b8/0x31c
> (XEN)    [<ffff82d0801e0d3f>] vmx_vmexit_handler+0xf91/0x1a5f
> (XEN)    [<ffff82d0801e7a51>] vmx_asm_vmexit_handler+0x41/0xc0
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 1:
> (XEN) Xen BUG at mm-locks.h:140
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/arch/x86/mm/paging.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 6b788f7..5af6309 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -412,6 +412,8 @@ static int paging_log_dirty_op(struct domain *d,
> 
>      if ( !resuming )
>          domain_pause(d);
> +     if (has_hvm_container_vcpu(current))
> +         p2m_lock(p2m_get_hostp2m(current->domain));
>      paging_lock(d);

Ah, I see.  This is the _caller_'s p2m lock but the _target_'s paging
lock.  Holding the caller's p2m lock to unstick this seems a bit of a
strange answer -- the paging op might be quite a long one.  And having
all vcpus take their own p2m locks before remote paging locks (and
probably other MM locks too operations) is going to be quite messy.

I guess the tricky path is clear_user_hvm, writing the bitmap back to
the caller's memory.  I wonder whether we could use unlocked p2m
lookups for that.  Probably not, because what if the caller's memory is
PoD, etc?

Getting hold of all the bitmap pages before taking the lock would be
messy too.

So this may end up being the least bad answer but I'd like to see if
we can think of something better first.

Tim.

>      if ( !d->arch.paging.preempt.dom )
> @@ -421,6 +423,8 @@ static int paging_log_dirty_op(struct domain *d,
>                d->arch.paging.preempt.op != sc->op )
>      {
>          paging_unlock(d);
> +        if (has_hvm_container_vcpu(current))
> +            p2m_unlock(p2m_get_hostp2m(current->domain));
>          ASSERT(!resuming);
>          domain_unpause(d);
>          return -EBUSY;
> @@ -533,6 +537,8 @@ static int paging_log_dirty_op(struct domain *d,
>      }
> 
>      paging_unlock(d);
> +    if (has_hvm_container_vcpu(current))
> +        p2m_unlock(p2m_get_hostp2m(current->domain));
> 
>      if ( rv )
>      {
> @@ -555,6 +561,8 @@ static int paging_log_dirty_op(struct domain *d,
>   out:
>      d->arch.paging.preempt.dom = NULL;
>      paging_unlock(d);
> +    if (has_hvm_container_vcpu(current))
> +        p2m_unlock(p2m_get_hostp2m(current->domain));
>      domain_unpause(d);
> 
>      if ( l1 )
> --
> 1.9.3 (Apple Git-50)
> 

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