From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Date: Wed, 1 May 2013 19:27:23 -0500 Message-ID: <1367454443.29231.20@snotra> References: <20130502001553.GA2363@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: Alexander Graf , , To: Marcelo Tosatti Return-path: In-Reply-To: <20130502001553.GA2363@amt.cnet> (from mtosatti@redhat.com on Wed May 1 19:15:53 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 05/01/2013 07:15:53 PM, Marcelo Tosatti wrote: > On Fri, Apr 26, 2013 at 07:53:38PM -0500, Scott Wood wrote: > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > > index 1020119..506c87d 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -832,6 +832,8 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > > { > > int r = RESUME_HOST; > > int s; > > + int idx = 0; /* silence bogus uninitialized warning */ > > + bool need_srcu = false; > > > > /* update before a new last_exit_type is rewritten */ > > kvmppc_update_timing_stats(vcpu); > > @@ -847,6 +849,20 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > > run->exit_reason = KVM_EXIT_UNKNOWN; > > run->ready_for_interrupt_injection = 1; > > > > + /* > > + * Don't get the srcu lock unconditionally, because kvm_ppc_pv() > > + * can call kvm_vcpu_block(), and kvm_ppc_pv() is shared with > > + * book3s, so dropping the srcu lock there would be awkward. > > + */ > > + switch (exit_nr) { > > + case BOOKE_INTERRUPT_ITLB_MISS: > > + case BOOKE_INTERRUPT_DTLB_MISS: > > + need_srcu = true; > > + } > > This is not good practice (codepaths should either hold srcu or not > hold > it, unconditionally). How is it different from moving the srcu lock into individual cases of the switch? I just did it this way to make it easier to add new exception types if necessary (e.g. at the time I thought I'd end up adding exceptions which lead to instruction emulation, but I ended up acquiring the lock further down the path in that case). > Can you give more details of the issue? (not obvious) ITLB/DTLB miss call things like gfn_to_memslot() which need the lock (but don't grab it themselves -- that seems like the real bad practice here...). The syscall exceptions can't have the SRCU lock held, because they call kvmppc_kvm_pv which can call kvm_vcpu_block() (yes, you can sleep with SRCU, but not indefinitely...). kvmppc_kvm_pv is shared with book3s code, so adding code to drop the srcu lock there would be a problem since book3s doesn't hold the SRCU lock then... -Scott