* [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
@ 2013-04-27 0:53 Scott Wood
2013-04-27 0:53 ` [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write Scott Wood
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Scott Wood @ 2013-04-27 0:53 UTC (permalink / raw)
To: Alexander Graf; +Cc: kvm-ppc, kvm, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write
2013-04-27 0:53 [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Scott Wood
@ 2013-04-27 0:53 ` Scott Wood
2013-05-02 11:22 ` Alexander Graf
2013-04-27 0:53 ` [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write Scott Wood
2013-05-02 0:15 ` [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Marcelo Tosatti
2 siblings, 1 reply; 15+ messages in thread
From: Scott Wood @ 2013-04-27 0:53 UTC (permalink / raw)
To: Alexander Graf; +Cc: kvm-ppc, kvm, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-04-27 0:53 [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Scott Wood
2013-04-27 0:53 ` [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write Scott Wood
@ 2013-04-27 0:53 ` Scott Wood
2013-05-01 23:24 ` Alexander Graf
2013-05-02 0:15 ` [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Marcelo Tosatti
2 siblings, 1 reply; 15+ messages in thread
From: Scott Wood @ 2013-04-27 0:53 UTC (permalink / raw)
To: Alexander Graf; +Cc: kvm-ppc, kvm, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-04-27 0:53 ` [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write Scott Wood
@ 2013-05-01 23:24 ` Alexander Graf
2013-05-02 7:18 ` Gleb Natapov
0 siblings, 1 reply; 15+ messages in thread
From: Alexander Graf @ 2013-05-01 23:24 UTC (permalink / raw)
To: Scott Wood; +Cc: kvm-ppc, kvm@vger.kernel.org mailing list, Marcelo Tosatti
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-05-01 23:24 ` Alexander Graf
@ 2013-05-02 7:18 ` Gleb Natapov
2013-05-02 10:53 ` Alexander Graf
0 siblings, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2013-05-02 7:18 UTC (permalink / raw)
To: Alexander Graf
Cc: Scott Wood, kvm-ppc, kvm@vger.kernel.org mailing list,
Marcelo Tosatti
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-05-02 7:18 ` Gleb Natapov
@ 2013-05-02 10:53 ` Alexander Graf
2013-05-02 11:00 ` Gleb Natapov
0 siblings, 1 reply; 15+ messages in thread
From: Alexander Graf @ 2013-05-02 10:53 UTC (permalink / raw)
To: Gleb Natapov
Cc: Scott Wood, kvm-ppc, kvm@vger.kernel.org mailing list,
Marcelo Tosatti
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-05-02 10:53 ` Alexander Graf
@ 2013-05-02 11:00 ` Gleb Natapov
2013-05-02 11:25 ` Alexander Graf
0 siblings, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2013-05-02 11:00 UTC (permalink / raw)
To: Alexander Graf
Cc: Scott Wood, kvm-ppc, kvm@vger.kernel.org mailing list,
Marcelo Tosatti
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write
2013-05-02 11:00 ` Gleb Natapov
@ 2013-05-02 11:25 ` Alexander Graf
0 siblings, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2013-05-02 11:25 UTC (permalink / raw)
To: Gleb Natapov
Cc: Scott Wood, kvm-ppc, kvm@vger.kernel.org mailing list,
Marcelo Tosatti
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-04-27 0:53 [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Scott Wood
2013-04-27 0:53 ` [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write Scott Wood
2013-04-27 0:53 ` [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write Scott Wood
@ 2013-05-02 0:15 ` Marcelo Tosatti
2013-05-02 0:27 ` Scott Wood
2 siblings, 1 reply; 15+ messages in thread
From: Marcelo Tosatti @ 2013-05-02 0:15 UTC (permalink / raw)
To: Scott Wood; +Cc: Alexander Graf, kvm-ppc, kvm
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)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-05-02 0:15 ` [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Marcelo Tosatti
@ 2013-05-02 0:27 ` Scott Wood
2013-05-02 0:30 ` Scott Wood
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Scott Wood @ 2013-05-02 0:27 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Alexander Graf, kvm-ppc, kvm
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-05-02 0:27 ` Scott Wood
@ 2013-05-02 0:30 ` Scott Wood
2013-05-02 11:20 ` Alexander Graf
2013-05-02 14:37 ` Marcelo Tosatti
2 siblings, 0 replies; 15+ messages in thread
From: Scott Wood @ 2013-05-02 0:30 UTC (permalink / raw)
To: Scott Wood; +Cc: Marcelo Tosatti, Alexander Graf, kvm-ppc, kvm
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-05-02 0:27 ` Scott Wood
2013-05-02 0:30 ` Scott Wood
@ 2013-05-02 11:20 ` Alexander Graf
2013-05-02 14:37 ` Marcelo Tosatti
2 siblings, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2013-05-02 11:20 UTC (permalink / raw)
To: Scott Wood; +Cc: Marcelo Tosatti, kvm-ppc, kvm
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-05-02 0:27 ` Scott Wood
2013-05-02 0:30 ` Scott Wood
2013-05-02 11:20 ` Alexander Graf
@ 2013-05-02 14:37 ` Marcelo Tosatti
2013-05-02 16:47 ` Scott Wood
2 siblings, 1 reply; 15+ messages in thread
From: Marcelo Tosatti @ 2013-05-02 14:37 UTC (permalink / raw)
To: Scott Wood; +Cc: Alexander Graf, kvm-ppc, kvm
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?
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions
2013-05-02 14:37 ` Marcelo Tosatti
@ 2013-05-02 16:47 ` Scott Wood
0 siblings, 0 replies; 15+ messages in thread
From: Scott Wood @ 2013-05-02 16:47 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Alexander Graf, kvm-ppc, kvm
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-05-02 16:47 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-27 0:53 [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Scott Wood
2013-04-27 0:53 ` [PATCH 2/3] kvm/ppc: Hold srcu lock when calling kvm_io_bus_read/write Scott Wood
2013-05-02 11:22 ` Alexander Graf
2013-04-27 0:53 ` [PATCH 3/3] kvm: Fix obsolete comment about locking for kvm_io_bus_read/write Scott Wood
2013-05-01 23:24 ` Alexander Graf
2013-05-02 7:18 ` Gleb Natapov
2013-05-02 10:53 ` Alexander Graf
2013-05-02 11:00 ` Gleb Natapov
2013-05-02 11:25 ` Alexander Graf
2013-05-02 0:15 ` [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions Marcelo Tosatti
2013-05-02 0:27 ` Scott Wood
2013-05-02 0:30 ` Scott Wood
2013-05-02 11:20 ` Alexander Graf
2013-05-02 14:37 ` Marcelo Tosatti
2013-05-02 16:47 ` Scott Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox