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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.10.14 at 11:20, <tim@xxxxxxx> wrote:
> 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.

But isn't the lock order enforcement meant to do its job only within
a single domain? I.e. isn't it a bug when it chokes on one domain's
P2M lock getting acquired while another domain's paging lock is
already being held?

Which isn't to say that I'd be particularly happy to see such
crossover locking getting added in the first place, as it would seem
to have ample room for introducing subtle deadlocks.

Jan

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