[PATCH RFC 0/2] xen/pvh: enable migration on PVH Dom0

[PATCH RFC 0/2] xen/pvh: enable migration on PVH Dom0

[Roger Pau Monne]

This series enables migration of guests (either PV or HVM) from a PVH Dom0. 
The first patch fixes a locking order problem in paging_log_dirty_op and the 
second one makes the mmu_update hypercall available to PVH guests.

Tested on a FreeBSD PVH Dom0 by doing > 200 migrations (and still ongoing).

I don't intend for this to be included in Xen 4.5.

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

[Roger Pau Monne]

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);
 
     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@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH RFC 2/2] xen/pvh: enable mmu_update hypercall

[Roger Pau Monne]

This is needed for performing save/restore of PV guests.

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/hvm/hvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f0e1edc..cf84e49 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4804,6 +4804,7 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
     [ __HYPERVISOR_grant_table_op ]  = (hvm_hypercall_t *)hvm_grant_table_op,
     HYPERCALL(vcpu_op),
     HYPERCALL(mmuext_op),
+    HYPERCALL(mmu_update),
     HYPERCALL(xsm_op),
     HYPERCALL(sched_op),
     HYPERCALL(event_channel_op),
-- 
1.9.3 (Apple Git-50)


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

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

[Andrew Cooper]

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

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

[Roger Pau Monné]

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.

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

[Andrew Cooper]

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

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

[Roger Pau Monné]

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

Re: [PATCH RFC 2/2] xen/pvh: enable mmu_update hypercall

[Tim Deegan]

At 12:53 +0200 on 15 Oct (1413374025), Roger Pau Monne wrote:
> This is needed for performing save/restore of PV guests.

On IRC I suggested that this would be OK as long as there were other
checks to make sure that the target of all these ops is PV (in
particular that a PVH/HVM guest can't end up calling PV MM operations
on itself).

Did it turn out that all those checks are already there?

Tim.

> 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/hvm/hvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f0e1edc..cf84e49 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4804,6 +4804,7 @@ static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls] = {
>      [ __HYPERVISOR_grant_table_op ]  = (hvm_hypercall_t *)hvm_grant_table_op,
>      HYPERCALL(vcpu_op),
>      HYPERCALL(mmuext_op),
> +    HYPERCALL(mmu_update),
>      HYPERCALL(xsm_op),
>      HYPERCALL(sched_op),
>      HYPERCALL(event_channel_op),
> --
> 1.9.3 (Apple Git-50)
> 

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

[Jan Beulich]

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

Re: [PATCH RFC 2/2] xen/pvh: enable mmu_update hypercall

[Jan Beulich]

>>> On 16.10.14 at 09:53, <tim@xen.org> wrote:
> At 12:53 +0200 on 15 Oct (1413374025), Roger Pau Monne wrote:
>> This is needed for performing save/restore of PV guests.
> 
> On IRC I suggested that this would be OK as long as there were other
> checks to make sure that the target of all these ops is PV (in
> particular that a PVH/HVM guest can't end up calling PV MM operations
> on itself).

And not just that - I can't even see how this would work at present:
paging_write_guest_entry() uses
v->arch.paging.mode->write_guest_entry, yet that actor gets filled
by shadow code only. I don't currently see how for PVH, requiring
HAP, this wouldn't end up in NULL dereferences. Am I overlooking
some (non-grep-able) initialization of this and .cmpxchg_guest_entry?

Jan

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

[Tim Deegan]

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

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

[Roger Pau Monné]

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.

Re: [PATCH RFC 2/2] xen/pvh: enable mmu_update hypercall

[Roger Pau Monné]

El 16/10/14 a les 10.45, Jan Beulich ha escrit:
>>>> On 16.10.14 at 09:53, <tim@xen.org> wrote:
>> At 12:53 +0200 on 15 Oct (1413374025), Roger Pau Monne wrote:
>>> This is needed for performing save/restore of PV guests.
>>
>> On IRC I suggested that this would be OK as long as there were other
>> checks to make sure that the target of all these ops is PV (in
>> particular that a PVH/HVM guest can't end up calling PV MM operations
>> on itself).

Silly question, but shouldn't all this checks already be in place in
case a PV Dom0 tries to execute mmu_update hypercalls against an HVM guest?

> And not just that - I can't even see how this would work at present:
> paging_write_guest_entry() uses
> v->arch.paging.mode->write_guest_entry, yet that actor gets filled
> by shadow code only. I don't currently see how for PVH, requiring
> HAP, this wouldn't end up in NULL dereferences. Am I overlooking
> some (non-grep-able) initialization of this and .cmpxchg_guest_entry?

It "works" because this is only used by the migration code, and the page
that's modified is never of the type PGT_writable_page. Should I look
into implementing this operations for HAP, or should I just prevent it's
usage from do_mmu_update if the caller turns out to be a HAP guest?

Roger.

Re: [PATCH RFC 2/2] xen/pvh: enable mmu_update hypercall

[Jan Beulich]

>>> On 16.10.14 at 13:30, <roger.pau@citrix.com> wrote:
> El 16/10/14 a les 10.45, Jan Beulich ha escrit:
>> And not just that - I can't even see how this would work at present:
>> paging_write_guest_entry() uses
>> v->arch.paging.mode->write_guest_entry, yet that actor gets filled
>> by shadow code only. I don't currently see how for PVH, requiring
>> HAP, this wouldn't end up in NULL dereferences. Am I overlooking
>> some (non-grep-able) initialization of this and .cmpxchg_guest_entry?
> 
> It "works" because this is only used by the migration code, and the page
> that's modified is never of the type PGT_writable_page. Should I look
> into implementing this operations for HAP, or should I just prevent it's
> usage from do_mmu_update if the caller turns out to be a HAP guest?

Aiui the only operation you really need is MMU_MACHPHYS_UPDATE,
in which case I'd suggest simply denying requests of the other two
kinds for the time being (and not just for HAP callers, but for any
paging_mode_external() ones).

Jan

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

[Jan Beulich]

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

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

[Tim Deegan]

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.

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

[Tim Deegan]

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.

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

[Jan Beulich]

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