On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@xxxxxxxxx> wrote:
> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
> realmode, so it can trigger the deadlock.
>
> Suppose the following scene:
>
> Two physical cpuM, cpuN, two VM instances A, B, each VM has a group of vcpus.
>
> If on cpuM, vcpu_A_1 holds bitlock X (HPTE_V_HVLOCK), then is switched out,
> and on cpuN, vcpu_A_2 try to lock X in realmode, then cpuN will be caught in
> realmode for a long time.
>
> What makes things even worse if the following happens,
> On cpuM, bitlockX is hold, on cpuN, Y is hold.
> vcpu_B_2 try to lock Y on cpuM in realmode
> vcpu_A_2 try to lock X on cpuN in realmode
>
> Oops! deadlock happens
>
> Signed-off-by: Liu Ping Fan <pingfank@xxxxxxxxxxxxxxxxxx>
Very nice catch :).
I think it makes a lot of sense to document the fact that kvmppc_hv_find_lock_hpte() should be called with preemption disabled in a comment above the function, so that next time when someone potentially calls it, he knows that he needs to put preempt_disable() around it.
Thanks a lot for finding this pretty subtle issue. May I ask how you got there? Did you actually see systems deadlock because of this?
Alex
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 043eec8..dbc1478 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -474,10 +474,13 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> }
>
> /* Find the HPTE in the hash table */
> + preempt_disable();
> index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
> HPTE_V_VALID | HPTE_V_ABSENT);
> - if (index < 0)
> + if (index < 0) {
> + preempt_enable();
> return -ENOENT;
> + }
> hptep = (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
> v = hptep[0] & ~HPTE_V_HVLOCK;
> gr = kvm->arch.revmap[index].guest_rpte;
> @@ -485,6 +488,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> /* Unlock the HPTE */
> asm volatile("lwsync" : : : "memory");
> hptep[0] = v;
> + preempt_enable();
>
> gpte->eaddr = eaddr;
> gpte->vpage = ((v & HPTE_V_AVPN) << 4) | ((eaddr >> 12) & 0xfff);
> --
> 1.8.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
