On 02.05.2013, at 02:27, 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?
Could you please do that and respin?
Alex
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
