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.
> >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)
Is that helpful?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
