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

["Roger Pau Monné" <roger.pau@citrix.com>]

El 16/10/14 a les 11.20, Tim Deegan ha escrit:
> 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@citrix.com>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> ---
>>  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?

Yes, the functions that need the caller p2m lock is
clear_user_hvm/copy_to_user_hvm. If I'm not mistaken we explicitly
stated that PVH is not going to use PoD, but since we are there we can
try to fix this function so it can work with pure HVM domains that can
indeed use PoD.

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

I'm certainly open to ideas, this was the naive way I've found to fixing it.

Roger.