Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
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
--
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
Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
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-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
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
Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
On 05/01/2013 07:27:23 PM, Scott Wood wrote: On 05/01/2013 07:15:53 PM, Marcelo Tosatti wrote: 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...). Never mind on the parenthetical -- grabbing it themselves wouldn't work because they return RCU-protected data. -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
Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
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
--
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
Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
On Fri, Apr 26, 2013 at 07:53:38PM -0500, Scott Wood wrote:
> KVM core expects arch code to acquire the srcu lock when calling
> gfn_to_memslot and similar functions.
>
> Signed-off-by: Scott Wood
> ---
> arch/powerpc/kvm/44x_tlb.c |5 +
> arch/powerpc/kvm/booke.c| 19 +++
> arch/powerpc/kvm/e500_mmu.c |5 +
> 3 files changed, 29 insertions(+)
>
> diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
> index 5dd3ab4..ed03854 100644
> --- a/arch/powerpc/kvm/44x_tlb.c
> +++ b/arch/powerpc/kvm/44x_tlb.c
> @@ -441,6 +441,7 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra,
> u8 rs, u8 ws)
> struct kvmppc_vcpu_44x *vcpu_44x = to_44x(vcpu);
> struct kvmppc_44x_tlbe *tlbe;
> unsigned int gtlb_index;
> + int idx;
>
> gtlb_index = kvmppc_get_gpr(vcpu, ra);
> if (gtlb_index >= KVM44x_GUEST_TLB_SIZE) {
> @@ -473,6 +474,8 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra,
> u8 rs, u8 ws)
> return EMULATE_FAIL;
> }
>
> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
> if (tlbe_is_host_safe(vcpu, tlbe)) {
> gva_t eaddr;
> gpa_t gpaddr;
> @@ -489,6 +492,8 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra,
> u8 rs, u8 ws)
> kvmppc_mmu_map(vcpu, eaddr, gpaddr, gtlb_index);
> }
>
> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
> trace_kvm_gtlb_write(gtlb_index, tlbe->tid, tlbe->word0, tlbe->word1,
>tlbe->word2);
>
> 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).
Can you give more details of the issue? (not obvious)
--
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
