* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 10:53 ` [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains Roger Pau Monne
@ 2014-10-15 11:31 ` Andrew Cooper
2014-10-15 11:52 ` Roger Pau Monné
2014-10-16 8:21 ` Jan Beulich
2014-10-16 9:20 ` Tim Deegan
2 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2014-10-15 11:31 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Tim Deegan, Jan Beulich
On 15/10/14 11:53, 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>
Hmm. I suspect there might be more of these issues scattered around
with the other DOMCTL/SYSCTL hypercalls, none of which have ever been
used from an HVM guest before.
> ---
> 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));
It appears as if there is some indentation issue here.
Also, can you pull current into a struct vcpu *curr on the stack?
~Andrew
> paging_lock(d);
>
> 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 )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 11:31 ` Andrew Cooper
@ 2014-10-15 11:52 ` Roger Pau Monné
2014-10-15 11:55 ` Andrew Cooper
0 siblings, 1 reply; 18+ messages in thread
From: Roger Pau Monné @ 2014-10-15 11:52 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Tim Deegan, Jan Beulich
El 15/10/14 a les 13.31, Andrew Cooper ha escrit:
> On 15/10/14 11:53, Roger Pau Monne wrote:
>> ---
>> 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));
>
> It appears as if there is some indentation issue here.
Yes, an extra space.
> Also, can you pull current into a struct vcpu *curr on the stack?
We only use ->domain from current, would you prefer me to pull
current->domain instead?
Roger.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 11:52 ` Roger Pau Monné
@ 2014-10-15 11:55 ` Andrew Cooper
2014-10-15 11:58 ` Roger Pau Monné
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2014-10-15 11:55 UTC (permalink / raw)
To: Roger Pau Monné, xen-devel; +Cc: Tim Deegan, Jan Beulich
On 15/10/14 12:52, Roger Pau Monné wrote:
> El 15/10/14 a les 13.31, Andrew Cooper ha escrit:
>> On 15/10/14 11:53, Roger Pau Monne wrote:
>>> ---
>>> 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));
>> It appears as if there is some indentation issue here.
> Yes, an extra space.
>
>> Also, can you pull current into a struct vcpu *curr on the stack?
> We only use ->domain from current, would you prefer me to pull
> current->domain instead?
>
> Roger.
You use current, and current->domain.
current degrades eventually into a macro which bitmasks the stack
pointer. Looking at the generated asm, GCC is not fantastic at eliding
recalculations of it. Therefore, it is better to manually optimise it.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 11:55 ` Andrew Cooper
@ 2014-10-15 11:58 ` Roger Pau Monné
0 siblings, 0 replies; 18+ messages in thread
From: Roger Pau Monné @ 2014-10-15 11:58 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Tim Deegan, Jan Beulich
El 15/10/14 a les 13.55, Andrew Cooper ha escrit:
> On 15/10/14 12:52, Roger Pau Monné wrote:
>> El 15/10/14 a les 13.31, Andrew Cooper ha escrit:
>>> On 15/10/14 11:53, Roger Pau Monne wrote:
>>>> ---
>>>> 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));
>>> It appears as if there is some indentation issue here.
>> Yes, an extra space.
>>
>>> Also, can you pull current into a struct vcpu *curr on the stack?
>> We only use ->domain from current, would you prefer me to pull
>> current->domain instead?
>>
>> Roger.
>
> You use current, and current->domain.
The only direct usage of current is for has_hvm_container_vcpu, which
can be replaced with has_hvm_container_domain.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 10:53 ` [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains Roger Pau Monne
2014-10-15 11:31 ` Andrew Cooper
@ 2014-10-16 8:21 ` Jan Beulich
2014-10-16 9:20 ` Tim Deegan
2 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-10-16 8:21 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Tim Deegan
>>> On 15.10.14 at 12:53, <roger.pau@citrix.com> 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:
I agree with Andrew that you should audit all code paths for this
problem, and either fix others as well or make clear in the
description that no further similar issues were found.
> --- 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));
Apart from the indentation issue already pointed out, the if() also
violates our coding style (also again on all further instances).
> paging_lock(d);
I further wonder whether introducing a helper doing the paired
locks/unlocks wouldn't be worthwhile. And of course I wonder
about performance implications - the P2M lock is already a pretty
contended one iirc.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-15 10:53 ` [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains Roger Pau Monne
2014-10-15 11:31 ` Andrew Cooper
2014-10-16 8:21 ` Jan Beulich
@ 2014-10-16 9:20 ` Tim Deegan
2014-10-16 10:03 ` Roger Pau Monné
2014-10-16 14:15 ` Jan Beulich
2 siblings, 2 replies; 18+ messages in thread
From: Tim Deegan @ 2014-10-16 9:20 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Jan Beulich
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?
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)
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-16 9:20 ` Tim Deegan
@ 2014-10-16 10:03 ` Roger Pau Monné
2014-11-13 15:46 ` Tim Deegan
2014-10-16 14:15 ` Jan Beulich
1 sibling, 1 reply; 18+ messages in thread
From: Roger Pau Monné @ 2014-10-16 10:03 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, Jan Beulich
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-16 10:03 ` Roger Pau Monné
@ 2014-11-13 15:46 ` Tim Deegan
2014-11-13 16:17 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Tim Deegan @ 2014-11-13 15:46 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Jan Beulich
At 12:03 +0200 on 16 Oct (1413457382), Roger Pau Monné wrote:
> El 16/10/14 a les 11.20, Tim Deegan ha escrit:
> > 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.
Sorry for the delay getting back to this -- I'm aware that I
discouraged this patch without suggesting an alternative!
I've looked through the hypervisor for callers of copy_to_guest() and
similar functions and they're almost all either:
- at the head or tail of a hypercall; or
- in code which doesn't touch any other domain (e.g. ctxt switch).
The only interesting cases are:
- paging_log_dirty_op(), which as you pointed out writes the bitmap
back to the caller while still holding the target's paging lock.
- shadow_track_dirty_vram(), likewise.
- hap_track_dirty_vram(), which allocates a local buffer to build
the bitmap in and then writes it back after dropping the lock
(and so should be OK).
- XEN_DOMCTL_getmemlist, which has a similar issue with
d->page_alloc_lock, for which reason it's already restricted to
non-paging-mode-external guests (and so also OK).
shadow_track_dirty_vram() can probably be made to behave like
hap_track_dirty_vram() -- at the moment it needs the lock because it's
copying from a shared master bitmap, but it could either copy it twice
(ugh) or do some RCU-like thing to allow the copies to happen outside
the paging lock.
That leaves paging_log_dirty_op(). The inner loops of that function
are already set up to be preempted for softirqs, &c. I wonder could
we arrange for it to map the first N pages of bitmap before taking any
locks, and then drop the locks after that many pages, and unmap them
again.
It wouldn't even need to actually set up a hypercall continuation if
!hypercall_preempt_check(); it can simply map a new batch and carry
on.
Does that sounds plausible? If so, then we can leave the lock
constraints as they are, which would make me very happy. :)
Cheers,
Tim.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-11-13 15:46 ` Tim Deegan
@ 2014-11-13 16:17 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-11-13 16:17 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, roger.pau
>>> On 13.11.14 at 16:46, <tim@xen.org> wrote:
> That leaves paging_log_dirty_op(). The inner loops of that function
> are already set up to be preempted for softirqs, &c. I wonder could
> we arrange for it to map the first N pages of bitmap before taking any
> locks, and then drop the locks after that many pages, and unmap them
> again.
>
> It wouldn't even need to actually set up a hypercall continuation if
> !hypercall_preempt_check(); it can simply map a new batch and carry
> on.
>
> Does that sounds plausible? If so, then we can leave the lock
> constraints as they are, which would make me very happy. :)
Apart from the already not too easily readable code becoming even
more convoluted, one possible extra problem I see is that
d->arch.paging.preempt modifications are currently being protected
by the paging lock. I.e. acquiring that lock later and dropping it
intermediately would further complicate the state tracking here. But
maybe this could be dealt with by setting
d->arch.paging.preempt.dom to current->domain earlier (i.e.
before even starting)?
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-16 9:20 ` Tim Deegan
2014-10-16 10:03 ` Roger Pau Monné
@ 2014-10-16 14:15 ` Jan Beulich
2014-10-16 15:05 ` Tim Deegan
1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2014-10-16 14:15 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, Roger Pau Monne
>>> On 16.10.14 at 11:20, <tim@xen.org> 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@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.
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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains
2014-10-16 14:15 ` Jan Beulich
@ 2014-10-16 15:05 ` Tim Deegan
0 siblings, 0 replies; 18+ messages in thread
From: Tim Deegan @ 2014-10-16 15:05 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monne
At 15:15 +0100 on 16 Oct (1413468955), Jan Beulich wrote:
> > 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?
That was my first thought, but in fact there are still deadlocks to
watch out for if we allow the ordering to be different for foreign
locks, e.g.:
- CPU 0 holds domA's paging lock, spins on domB's p2m lock.
- CPU 1 holds domB's paging lock, spins on domA's p2m lock.
- CPU 2 holds domA's p2m lock, spins on domA's paging lock.
- CPU 3 holds domB's p2m lock, spins on domB's paging lock.
The current simple rules avoid having to think about that stuff at
all, which I like.
Most hypercalls will finish their work and drop locks before they
write reults back to the caller; unfortunately things like lgd bitmaps
have potentially very large amounts of writing to do.
If it's just this operation, and maybe one or two more, maybe we can
reshuffle them to avoid doing the writeback with other locks held.
Tim.
^ permalink raw reply [flat|nested] 18+ messages in thread