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

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

[Scott Wood]

KVM core expects arch code to acquire the srcu lock when calling
gfn_to_memslot and similar functions.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 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;
+	}
+
+	if (need_srcu)
+		idx = srcu_read_lock(&vcpu->kvm->srcu);
+
 	switch (exit_nr) {
 	case BOOKE_INTERRUPT_MACHINE_CHECK:
 		printk("MACHINE CHECK: %lx\n", mfspr(SPRN_MCSR));
@@ -1138,6 +1154,9 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		BUG();
 	}
 
+	if (need_srcu)
+		srcu_read_unlock(&vcpu->kvm->srcu, idx);
+
 	/*
 	 * To avoid clobbering exit_reason, only check for signals if we
 	 * aren't already exiting to userspace for some other reason.
diff --git a/arch/powerpc/kvm/e500_mmu.c b/arch/powerpc/kvm/e500_mmu.c
index c41a5a9..6d6f153 100644
--- a/arch/powerpc/kvm/e500_mmu.c
+++ b/arch/powerpc/kvm/e500_mmu.c
@@ -396,6 +396,7 @@ int kvmppc_e500_emul_tlbwe(struct kvm_vcpu *vcpu)
 	struct kvm_book3e_206_tlb_entry *gtlbe;
 	int tlbsel, esel;
 	int recal = 0;
+	int idx;
 
 	tlbsel = get_tlb_tlbsel(vcpu);
 	esel = get_tlb_esel(vcpu, tlbsel);
@@ -430,6 +431,8 @@ int kvmppc_e500_emul_tlbwe(struct kvm_vcpu *vcpu)
 			kvmppc_set_tlb1map_range(vcpu, gtlbe);
 	}
 
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
+
 	/* Invalidate shadow mappings for the about-to-be-clobbered TLBE. */
 	if (tlbe_is_host_safe(vcpu, gtlbe)) {
 		u64 eaddr = get_tlb_eaddr(gtlbe);
@@ -444,6 +447,8 @@ int kvmppc_e500_emul_tlbwe(struct kvm_vcpu *vcpu)
 		kvmppc_mmu_map(vcpu, eaddr, raddr, index_of(tlbsel, esel));
 	}
 
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
+
 	kvmppc_set_exit_type(vcpu, EMULATED_TLBWE_EXITS);
 	return EMULATE_DONE;
 }
-- 
1.7.10.4

[PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write

[Scott Wood]

These functions do an srcu_dereference without acquiring the srcu lock
themselves.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/kvm/powerpc.c |   23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 31084c6..270773f 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -622,6 +622,8 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu *vcpu,
 int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
                        unsigned int rt, unsigned int bytes, int is_bigendian)
 {
+	int idx, ret;
+
 	if (bytes > sizeof(run->mmio.data)) {
 		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
 		       run->mmio.len);
@@ -637,8 +639,14 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	vcpu->mmio_is_write = 0;
 	vcpu->arch.mmio_sign_extend = 0;
 
-	if (!kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
-			     bytes, &run->mmio.data)) {
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
+
+	ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+			      bytes, &run->mmio.data);
+
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
+
+	if (!ret) {
 		kvmppc_complete_mmio_load(vcpu, run);
 		vcpu->mmio_needed = 0;
 		return EMULATE_DONE;
@@ -663,6 +671,7 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
                         u64 val, unsigned int bytes, int is_bigendian)
 {
 	void *data = run->mmio.data;
+	int idx, ret;
 
 	if (bytes > sizeof(run->mmio.data)) {
 		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
@@ -692,8 +701,14 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		}
 	}
 
-	if (!kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
-			      bytes, &run->mmio.data)) {
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
+
+	ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+			       bytes, &run->mmio.data);
+
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
+
+	if (!ret) {
 		vcpu->mmio_needed = 0;
 		return EMULATE_DONE;
 	}
-- 
1.7.10.4

[PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Scott Wood]

Nobody is actually calling these functions with slots_lock held.
The srcu read lock, OTOH, is required.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 virt/kvm/kvm_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5da9f02..54a14fa 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2828,7 +2828,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
 	return off;
 }
 
-/* kvm_io_bus_write - called under kvm->slots_lock */
+/* kvm_io_bus_write - called under kvm->srcu read lock */
 int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		     int len, const void *val)
 {
@@ -2856,7 +2856,7 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 	return -EOPNOTSUPP;
 }
 
-/* kvm_io_bus_read - called under kvm->slots_lock */
+/* kvm_io_bus_read - called under kvm->srcu read lock */
 int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		    int len, void *val)
 {
-- 
1.7.10.4

Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Alexander Graf]

On 27.04.2013, at 02:53, Scott Wood wrote:

> Nobody is actually calling these functions with slots_lock held.
> The srcu read lock, OTOH, is required.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Marcelo, could you please ack?


Alex

> ---
> virt/kvm/kvm_main.c |    4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5da9f02..54a14fa 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2828,7 +2828,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
> 	return off;
> }
> 
> -/* kvm_io_bus_write - called under kvm->slots_lock */
> +/* kvm_io_bus_write - called under kvm->srcu read lock */
> int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> 		     int len, const void *val)
> {
> @@ -2856,7 +2856,7 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> 	return -EOPNOTSUPP;
> }
> 
> -/* kvm_io_bus_read - called under kvm->slots_lock */
> +/* kvm_io_bus_read - called under kvm->srcu read lock */
> int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> 		    int len, void *val)
> {
> -- 
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@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

[Marcelo Tosatti]

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 <scottwood@freescale.com>
> ---
>  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)

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

[Scott Wood]

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

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

[Scott Wood]

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

Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Gleb Natapov]

On Thu, May 02, 2013 at 01:24:15AM +0200, Alexander Graf wrote:
> 
> On 27.04.2013, at 02:53, Scott Wood wrote:
> 
> > Nobody is actually calling these functions with slots_lock held.
> > The srcu read lock, OTOH, is required.
> > 
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> 
> Marcelo, could you please ack?
> 
Is something in the series depends on this patch? If not this should go
directly into kvm.git, not via ppc tree.

> 
> Alex
> 
> > ---
> > virt/kvm/kvm_main.c |    4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 5da9f02..54a14fa 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2828,7 +2828,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
> > 	return off;
> > }
> > 
> > -/* kvm_io_bus_write - called under kvm->slots_lock */
> > +/* kvm_io_bus_write - called under kvm->srcu read lock */
> > int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> > 		     int len, const void *val)
> > {
> > @@ -2856,7 +2856,7 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> > 	return -EOPNOTSUPP;
> > }
> > 
> > -/* kvm_io_bus_read - called under kvm->slots_lock */
> > +/* kvm_io_bus_read - called under kvm->srcu read lock */
> > int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> > 		    int len, void *val)
> > {
> > -- 
> > 1.7.10.4
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Alexander Graf]

On 02.05.2013, at 09:18, Gleb Natapov wrote:

> On Thu, May 02, 2013 at 01:24:15AM +0200, Alexander Graf wrote:
>> 
>> On 27.04.2013, at 02:53, Scott Wood wrote:
>> 
>>> Nobody is actually calling these functions with slots_lock held.
>>> The srcu read lock, OTOH, is required.
>>> 
>>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>> 
>> Marcelo, could you please ack?
>> 
> Is something in the series depends on this patch? If not this should go
> directly into kvm.git, not via ppc tree.

If that's easier for you, I'm perfectly fine with that.


Alex

Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Gleb Natapov]

On Thu, May 02, 2013 at 12:53:44PM +0200, Alexander Graf wrote:
> 
> On 02.05.2013, at 09:18, Gleb Natapov wrote:
> 
> > On Thu, May 02, 2013 at 01:24:15AM +0200, Alexander Graf wrote:
> >> 
> >> On 27.04.2013, at 02:53, Scott Wood wrote:
> >> 
> >>> Nobody is actually calling these functions with slots_lock held.
> >>> The srcu read lock, OTOH, is required.
> >>> 
> >>> Signed-off-by: Scott Wood <scottwood@freescale.com>
> >> 
> >> Marcelo, could you please ack?
> >> 
> > Is something in the series depends on this patch? If not this should go
> > directly into kvm.git, not via ppc tree.
> 
> If that's easier for you, I'm perfectly fine with that.
> 
It really is. In the case of this patch it is not a big deal of course, but it
helps with tracking which changes an architecture code actually depends
on, and what are just generic fixes. When fixes for the common code are
hidden in the middle of a ppc patch set they are easy to miss. 

--
			Gleb.

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

[Alexander Graf]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write

[Alexander Graf]

On 27.04.2013, at 02:53, Scott Wood wrote:

> These functions do an srcu_dereference without acquiring the srcu lock
> themselves.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Thanks, applied to kvm-ppc-queue.


Alex

Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write

[Alexander Graf]

On 02.05.2013, at 13:00, Gleb Natapov wrote:

> On Thu, May 02, 2013 at 12:53:44PM +0200, Alexander Graf wrote:
>> 
>> On 02.05.2013, at 09:18, Gleb Natapov wrote:
>> 
>>> On Thu, May 02, 2013 at 01:24:15AM +0200, Alexander Graf wrote:
>>>> 
>>>> On 27.04.2013, at 02:53, Scott Wood wrote:
>>>> 
>>>>> Nobody is actually calling these functions with slots_lock held.
>>>>> The srcu read lock, OTOH, is required.
>>>>> 
>>>>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>>>> 
>>>> Marcelo, could you please ack?
>>>> 
>>> Is something in the series depends on this patch? If not this should go
>>> directly into kvm.git, not via ppc tree.
>> 
>> If that's easier for you, I'm perfectly fine with that.
>> 
> It really is. In the case of this patch it is not a big deal of course, but it
> helps with tracking which changes an architecture code actually depends
> on, and what are just generic fixes. When fixes for the common code are
> hidden in the middle of a ppc patch set they are easy to miss. 

I agree :). That's why Scott sent the other generic patch separately. I suppose he just figured that a comment change isn't too big of a deal.


Alex

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

[Marcelo Tosatti]

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?

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

[Scott Wood]

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