Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions

[Scott Wood <scottwood@freescale.com>]

On 05/02/2013 09:37:53 AM, Marcelo Tosatti wrote:
> On Wed, May 01, 2013 at 07:27:23PM -0500, Scott Wood wrote:
> > 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).
> 
> Question: is this piece of code accessing this data structure?
> Answer: it depends on a given runtime configuration.
> 
> Its confusing.

I'll move the locking into the individual cases that need it.  It's not  
"configuration" but rather, "which event are we handling?"

> > >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
> 
> Its OK to nest srcu calls as long as there are properly ordered  
> releases:
> 
> idx1 = srcu_read_lock()
> idx2 = srcu_read_lock()
> 
> srcu_read_unlock(idx2)
> srcu_read_unlock(idx1)

That's not the issue.  The issue is we want to make sure we're not  
locked when we call kvm_vcpu_block(), because it can block for a very  
long time.  But we can't just unlock, because the code is also called  
by book3s without the lock held.  I don't want to go adding locks to  
book3s as well.  Better to limit the locking to be closer to where it's  
actually needed.

-Scott