On Wed, Nov 06, 2013 at 02:02:07PM +0800, Liu ping fan wrote: > On Wed, Nov 6, 2013 at 1:04 PM, Paul Mackerras <paulus@xxxxxxxxx> wrote: > > On Tue, Nov 05, 2013 at 03:42:43PM +0800, Liu Ping Fan wrote: > >> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and > >> realmode, so it can trigger the deadlock. > > > > Good catch, we should have preemption disabled while ever we have a > > HPTE locked. > > > >> @@ -474,8 +474,10 @@ 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); > >> + preempt_enable(); > > > > Which means we need to add the preempt_enable after unlocking the > > HPTE, not here. > > > Yes. Sorry, but I am not sure about whether we can call > preempt_disable/enable() in realmode. I think since thread_info is > allocated with linear address, so we can use preempt_disable/enable() > inside kvmppc_hv_find_lock_hpte(), right? Your analysis correctly pointed out that we can get a deadlock if we can be preempted while holding a lock on a HPTE. That means that we have to disable preemption before taking an HPTE lock and keep it disabled until after we unlock the HPTE. Since the point of kvmppc_hv_find_lock_hpte() is to lock the HPTE and return with it locked, we can't have the preempt_enable() inside it. The preempt_enable() has to come after we have unlocked the HPTE. That is also why we can't have the preempt_enable() where your patch put it; it needs to be about 9 lines further down, after the statement hptep[0] = v. (We also need to make sure to re-enable preemption in the index < 0 case.) Regards, Paul. -- 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
