* [PATCH v3 2/2] powerpc: kvm: fix rare but potential deadlock scene
From: Liu Ping Fan @ 2013-11-08 7:29 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc; +Cc: Paul Mackerras, Alexander Graf
In-Reply-To: <1383895794-16164-1-git-send-email-pingfank@linux.vnet.ibm.com>
Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
realmode, so it can trigger the deadlock.
Suppose the following scene:
Two physical cpuM, cpuN, two VM instances A, B, each VM has a group of vcpus.
If on cpuM, vcpu_A_1 holds bitlock X (HPTE_V_HVLOCK), then is switched out,
and on cpuN, vcpu_A_2 try to lock X in realmode, then cpuN will be caught in
realmode for a long time.
What makes things even worse if the following happens,
On cpuM, bitlockX is hold, on cpuN, Y is hold.
vcpu_B_2 try to lock Y on cpuM in realmode
vcpu_A_2 try to lock X on cpuN in realmode
Oops! deadlock happens
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 4 ++--
arch/powerpc/kvm/book3s_64_mmu_hv.c | 5 +++--
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 20 ++++++++++++++++----
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index a818932..3d710ba 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -129,9 +129,9 @@ extern void kvmppc_mmu_flush_segments(struct kvm_vcpu *vcpu);
extern int kvmppc_book3s_hv_page_fault(struct kvm_run *run,
struct kvm_vcpu *vcpu, unsigned long addr,
unsigned long status);
-extern void kvmppc_hv_unlock_hpte(ulong *hptep, ulong *hpte_val);
+extern void kvmppc_hv_unlock_hpte(ulong *hptep, ulong *hpte_val, bool vmode);
extern long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr,
- unsigned long slb_v, unsigned long valid);
+ unsigned long slb_v, unsigned long valid, bool vmode);
extern void kvmppc_mmu_hpte_cache_map(struct kvm_vcpu *vcpu, struct hpte_cache *pte);
extern struct hpte_cache *kvmppc_mmu_hpte_cache_next(struct kvm_vcpu *vcpu);
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 97685e7..12d9635 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -475,13 +475,14 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
/* Find the HPTE in the hash table */
index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
- HPTE_V_VALID | HPTE_V_ABSENT);
+ HPTE_V_VALID | HPTE_V_ABSENT,
+ true);
if (index < 0)
return -ENOENT;
hptep = (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
v = hptep[0];
gr = kvm->arch.revmap[index].guest_rpte;
- kvmppc_hv_unlock_hpte(hptep, &v);
+ kvmppc_hv_unlock_hpte(hptep, &v, true);
gpte->eaddr = eaddr;
gpte->vpage = ((v & HPTE_V_AVPN) << 4) | ((eaddr >> 12) & 0xfff);
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 0ff9e91..18a9425 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -749,16 +749,22 @@ static int slb_base_page_shift[4] = {
20, /* 1M, unsupported */
};
-void kvmppc_hv_unlock_hpte(unsigned long *hptep, unsigned long *hpte_val)
+void kvmppc_hv_unlock_hpte(unsigned long *hptep, unsigned long *hpte_val,
+ bool vmode)
{
*hpte_val = *hpte_val & ~HPTE_V_HVLOCK;
asm volatile("lwsync" : : : "memory");
*hptep = *hpte_val;
+ if (unlikely(vmode))
+ preempt_enable();
}
EXPORT_SYMBOL(kvmppc_hv_unlock_hpte);
+/* If called from virtmode and success to lock, then the context will be set
+ * as preemption disabled
+ */
long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
- unsigned long valid)
+ unsigned long valid, bool vmode)
{
unsigned int i;
unsigned int pshift;
@@ -796,6 +802,9 @@ long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
avpn &= ~0x7fUL;
val |= avpn;
+ if (unlikely(vmode))
+ preempt_disable();
+
for (;;) {
hpte = (unsigned long *)(kvm->arch.hpt_virt + (hash << 7));
@@ -833,6 +842,9 @@ long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
val |= HPTE_V_SECONDARY;
hash = hash ^ kvm->arch.hpt_mask;
}
+
+ if (unlikely(vmode))
+ preempt_enable();
return -1;
}
EXPORT_SYMBOL(kvmppc_hv_find_lock_hpte);
@@ -864,7 +876,7 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu, unsigned long addr,
if (status & DSISR_NOHPTE)
valid |= HPTE_V_ABSENT;
- index = kvmppc_hv_find_lock_hpte(kvm, addr, slb_v, valid);
+ index = kvmppc_hv_find_lock_hpte(kvm, addr, slb_v, valid, false);
if (index < 0) {
if (status & DSISR_NOHPTE)
return status; /* there really was no HPTE */
@@ -875,7 +887,7 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu, unsigned long addr,
r = hpte[1];
rev = real_vmalloc_addr(&kvm->arch.revmap[index]);
gr = rev->guest_rpte;
- kvmppc_hv_unlock_hpte(hpte, &v);
+ kvmppc_hv_unlock_hpte(hpte, &v, false);
/* For not found, if the HPTE is valid by now, retry the instruction */
if ((status & DSISR_NOHPTE) && (v & HPTE_V_VALID))
--
1.8.1.4
^ permalink raw reply related
* [PATCH v3 0/2] powerpc kvm: fix deadlock scene
From: Liu Ping Fan @ 2013-11-08 7:29 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc; +Cc: Paul Mackerras, Alexander Graf
v2->v3:
introduce kvmppc_hv_unlock_hpte() to pair with kvmppc_hv_find_lock_hpte()
and hide the preemption detail inside this pair from the callers
Liu Ping Fan (2):
powerpc: kvm: pair kvmppc_hv_find_lock_hpte with _unlock_hpte
powerpc: kvm: fix rare but potential deadlock scene
arch/powerpc/include/asm/kvm_book3s.h | 3 ++-
arch/powerpc/kvm/book3s_64_mmu_hv.c | 10 ++++------
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 29 ++++++++++++++++++++++++-----
3 files changed, 30 insertions(+), 12 deletions(-)
--
1.8.1.4
^ permalink raw reply
* [PATCH v3 1/2] powerpc: kvm: pair kvmppc_hv_find_lock_hpte with _unlock_hpte
From: Liu Ping Fan @ 2013-11-08 7:29 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc; +Cc: Paul Mackerras, Alexander Graf
In-Reply-To: <1383895794-16164-1-git-send-email-pingfank@linux.vnet.ibm.com>
Highlight the lock pair for the reader. (and later it will the
place to hide the detail about preemption disable)
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 1 +
arch/powerpc/kvm/book3s_64_mmu_hv.c | 7 ++-----
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 13 ++++++++++---
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index fa19e2f..a818932 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -129,6 +129,7 @@ extern void kvmppc_mmu_flush_segments(struct kvm_vcpu *vcpu);
extern int kvmppc_book3s_hv_page_fault(struct kvm_run *run,
struct kvm_vcpu *vcpu, unsigned long addr,
unsigned long status);
+extern void kvmppc_hv_unlock_hpte(ulong *hptep, ulong *hpte_val);
extern long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr,
unsigned long slb_v, unsigned long valid);
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 842f081..97685e7 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -479,12 +479,9 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
if (index < 0)
return -ENOENT;
hptep = (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
- v = hptep[0] & ~HPTE_V_HVLOCK;
+ v = hptep[0];
gr = kvm->arch.revmap[index].guest_rpte;
-
- /* Unlock the HPTE */
- asm volatile("lwsync" : : : "memory");
- hptep[0] = v;
+ kvmppc_hv_unlock_hpte(hptep, &v);
gpte->eaddr = eaddr;
gpte->vpage = ((v & HPTE_V_AVPN) << 4) | ((eaddr >> 12) & 0xfff);
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 9c51544..0ff9e91 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -749,6 +749,14 @@ static int slb_base_page_shift[4] = {
20, /* 1M, unsupported */
};
+void kvmppc_hv_unlock_hpte(unsigned long *hptep, unsigned long *hpte_val)
+{
+ *hpte_val = *hpte_val & ~HPTE_V_HVLOCK;
+ asm volatile("lwsync" : : : "memory");
+ *hptep = *hpte_val;
+}
+EXPORT_SYMBOL(kvmppc_hv_unlock_hpte);
+
long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
unsigned long valid)
{
@@ -863,12 +871,11 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu, unsigned long addr,
return 0; /* for prot fault, HPTE disappeared */
}
hpte = (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
- v = hpte[0] & ~HPTE_V_HVLOCK;
+ v = hpte[0];
r = hpte[1];
rev = real_vmalloc_addr(&kvm->arch.revmap[index]);
gr = rev->guest_rpte;
-
- unlock_hpte(hpte, v);
+ kvmppc_hv_unlock_hpte(hpte, &v);
/* For not found, if the HPTE is valid by now, retry the instruction */
if ((status & DSISR_NOHPTE) && (v & HPTE_V_VALID))
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Liu ping fan @ 2013-11-08 4:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul Mackerras, linuxppc-dev, Alexander Graf, kvm-ppc
In-Reply-To: <1383883907.4776.215.camel@pasglop>
On Fri, Nov 8, 2013 at 12:11 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2013-11-08 at 15:05 +1100, Benjamin Herrenschmidt wrote:
>> On Fri, 2013-11-08 at 04:10 +0100, Alexander Graf wrote:
>> > On 08.11.2013, at 03:44, Liu Ping Fan <kernelfans@gmail.com> wrote:
>> >
>> > > syscall is a very common behavior inside guest, and this patch
>> > > optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
>> > > so hypervisor can return to guest without heavy exit, i.e, no need
>> > > to swap TLB, HTAB,.. etc
>> >
>> > The syscall exit you touch here only happens when you do an sc > 0
>> > with MSR_PR set inside the guest. The only case you realistically see
>> > this is when you run PR KVM inside of an HV KVM guest.
>> >
>> > I don't think we should optimize for that case. Instead, we should
>> > rather try to not bounce to the 1st hypervisor in the first place in
>> > that scenario :).
>>
>> Well, so unfortunately openstack CI uses PR inside HV pretty
>> heavily .... it *might* be worthwhile optimizing that path if the patch
>> is simple enough... I'd make that Paul's call.
>
> Note that this is a statement of value for the idea ... not the
> implementation ;-) From a quick look with Paulus, the patch is quite
> broken. I'll let Paul comment in details.
>
Thank you very much,
Regards,
Pingfan
^ permalink raw reply
* Re: [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Liu ping fan @ 2013-11-08 4:19 UTC (permalink / raw)
To: Alexander Graf; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <470E7CC0-6397-4343-89DA-6818531E6775@suse.de>
On Fri, Nov 8, 2013 at 11:10 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 08.11.2013, at 03:44, Liu Ping Fan <kernelfans@gmail.com> wrote:
>
>> syscall is a very common behavior inside guest, and this patch
>> optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
>> so hypervisor can return to guest without heavy exit, i.e, no need
>> to swap TLB, HTAB,.. etc
>
> The syscall exit you touch here only happens when you do an sc > 0 with MSR_PR set inside the guest. The only case you realistically see this is when you run PR KVM inside of an HV KVM guest.
>
Maybe I misunderstood the ISA spec, but refer for "6.5.14 System Call
Interrupt", no description about the MSR_PR when sc trigger a syscall
interrupt. So I think, guest application "sc 0" will also fall to the
kernel who owns hypervisor mode. Am I right?
> I don't think we should optimize for that case. Instead, we should rather try to not bounce to the 1st hypervisor in the first place in that scenario :).
>
Sorry, but just want to make clear about the idiom: 0 -> kernel run
with NV, and 1st -> kernel run on HV-KVM and provide PR-KVM to up
layer? Right?
When you say "try to not bounce to the 1st hypervisor ", what is the
exact meaning and how can we achieve this? I am a quite newer on
powerpc, and hope that I can get more clear figure about it :)
Thanks
Pingfan
>
> Alex
>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>> Compiled, but lack of bare metal, I have not tested it yet.
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 6 ------
>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 13 ++++++++++++-
>> 2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 62a2b5a..73dc852 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -628,12 +628,6 @@ static int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> /* hcall - punt to userspace */
>> int i;
>>
>> - if (vcpu->arch.shregs.msr & MSR_PR) {
>> - /* sc 1 from userspace - reflect to guest syscall */
>> - kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_SYSCALL);
>> - r = RESUME_GUEST;
>> - break;
>> - }
>> run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
>> for (i = 0; i < 9; ++i)
>> run->papr_hcall.args[i] = kvmppc_get_gpr(vcpu, 4 + i);
>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> index c71103b..9f626c3 100644
>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> @@ -1388,7 +1388,8 @@ kvmppc_hisi:
>> hcall_try_real_mode:
>> ld r3,VCPU_GPR(R3)(r9)
>> andi. r0,r11,MSR_PR
>> - bne guest_exit_cont
>> + /* sc 1 from userspace - reflect to guest syscall */
>> + bne sc_0_fast_return
>> clrrdi r3,r3,2
>> cmpldi r3,hcall_real_table_end - hcall_real_table
>> bge guest_exit_cont
>> @@ -1409,6 +1410,16 @@ hcall_try_real_mode:
>> ld r11,VCPU_MSR(r4)
>> b fast_guest_return
>>
>> +sc_0_fast_return:
>> + ld r10,VCPU_PC(r9)
>> + ld r11,VCPU_MSR(r9)
>> + mtspr SPRN_SRR0,r10
>> + mtspr SPRN_SRR1,r11
>> + li r10, BOOK3S_INTERRUPT_SYSCALL
>> + LOAD_REG_IMMEDIATE(r3,0xffffffff87a0ffff) /* zero 33:36,42:47 */
>> + and r11,r11,r3
>> + b fast_guest_return
>> +
>> /* We've attempted a real mode hcall, but it's punted it back
>> * to userspace. We need to restore some clobbered volatiles
>> * before resuming the pass-it-to-qemu path */
>> --
>> 1.8.1.4
>>
>
^ permalink raw reply
* Re: [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Benjamin Herrenschmidt @ 2013-11-08 4:11 UTC (permalink / raw)
To: Alexander Graf; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc, Liu Ping Fan
In-Reply-To: <1383883503.4776.214.camel@pasglop>
On Fri, 2013-11-08 at 15:05 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2013-11-08 at 04:10 +0100, Alexander Graf wrote:
> > On 08.11.2013, at 03:44, Liu Ping Fan <kernelfans@gmail.com> wrote:
> >
> > > syscall is a very common behavior inside guest, and this patch
> > > optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
> > > so hypervisor can return to guest without heavy exit, i.e, no need
> > > to swap TLB, HTAB,.. etc
> >
> > The syscall exit you touch here only happens when you do an sc > 0
> > with MSR_PR set inside the guest. The only case you realistically see
> > this is when you run PR KVM inside of an HV KVM guest.
> >
> > I don't think we should optimize for that case. Instead, we should
> > rather try to not bounce to the 1st hypervisor in the first place in
> > that scenario :).
>
> Well, so unfortunately openstack CI uses PR inside HV pretty
> heavily .... it *might* be worthwhile optimizing that path if the patch
> is simple enough... I'd make that Paul's call.
Note that this is a statement of value for the idea ... not the
implementation ;-) From a quick look with Paulus, the patch is quite
broken. I'll let Paul comment in details.
Cheers,
Ben.
> Cheers,
> Ben.
>
> >
> > Alex
> >
> > >
> > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> > > ---
> > > Compiled, but lack of bare metal, I have not tested it yet.
> > > ---
> > > arch/powerpc/kvm/book3s_hv.c | 6 ------
> > > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 13 ++++++++++++-
> > > 2 files changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_hv.c
> > b/arch/powerpc/kvm/book3s_hv.c
> > > index 62a2b5a..73dc852 100644
> > > --- a/arch/powerpc/kvm/book3s_hv.c
> > > +++ b/arch/powerpc/kvm/book3s_hv.c
> > > @@ -628,12 +628,6 @@ static int kvmppc_handle_exit(struct kvm_run
> > *run, struct kvm_vcpu *vcpu,
> > > /* hcall - punt to userspace */
> > > int i;
> > >
> > > - if (vcpu->arch.shregs.msr & MSR_PR) {
> > > - /* sc 1 from userspace - reflect to guest syscall */
> > > - kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_SYSCALL);
> > > - r = RESUME_GUEST;
> > > - break;
> > > - }
> > > run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
> > > for (i = 0; i < 9; ++i)
> > > run->papr_hcall.args[i] = kvmppc_get_gpr(vcpu, 4 + i);
> > > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > > index c71103b..9f626c3 100644
> > > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > > @@ -1388,7 +1388,8 @@ kvmppc_hisi:
> > > hcall_try_real_mode:
> > > ld r3,VCPU_GPR(R3)(r9)
> > > andi. r0,r11,MSR_PR
> > > - bne guest_exit_cont
> > > + /* sc 1 from userspace - reflect to guest syscall */
> > > + bne sc_0_fast_return
> > > clrrdi r3,r3,2
> > > cmpldi r3,hcall_real_table_end - hcall_real_table
> > > bge guest_exit_cont
> > > @@ -1409,6 +1410,16 @@ hcall_try_real_mode:
> > > ld r11,VCPU_MSR(r4)
> > > b fast_guest_return
> > >
> > > +sc_0_fast_return:
> > > + ld r10,VCPU_PC(r9)
> > > + ld r11,VCPU_MSR(r9)
> > > + mtspr SPRN_SRR0,r10
> > > + mtspr SPRN_SRR1,r11
> > > + li r10, BOOK3S_INTERRUPT_SYSCALL
> > > + LOAD_REG_IMMEDIATE(r3,0xffffffff87a0ffff) /* zero 33:36,42:47 */
> > > + and r11,r11,r3
> > > + b fast_guest_return
> > > +
> > > /* We've attempted a real mode hcall, but it's punted it back
> > > * to userspace. We need to restore some clobbered volatiles
> > > * before resuming the pass-it-to-qemu path */
> > > --
> > > 1.8.1.4
> > >
>
^ permalink raw reply
* Re: [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Benjamin Herrenschmidt @ 2013-11-08 4:05 UTC (permalink / raw)
To: Alexander Graf; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc, Liu Ping Fan
In-Reply-To: <470E7CC0-6397-4343-89DA-6818531E6775@suse.de>
On Fri, 2013-11-08 at 04:10 +0100, Alexander Graf wrote:
> On 08.11.2013, at 03:44, Liu Ping Fan <kernelfans@gmail.com> wrote:
>
> > syscall is a very common behavior inside guest, and this patch
> > optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
> > so hypervisor can return to guest without heavy exit, i.e, no need
> > to swap TLB, HTAB,.. etc
>
> The syscall exit you touch here only happens when you do an sc > 0
> with MSR_PR set inside the guest. The only case you realistically see
> this is when you run PR KVM inside of an HV KVM guest.
>
> I don't think we should optimize for that case. Instead, we should
> rather try to not bounce to the 1st hypervisor in the first place in
> that scenario :).
Well, so unfortunately openstack CI uses PR inside HV pretty
heavily .... it *might* be worthwhile optimizing that path if the patch
is simple enough... I'd make that Paul's call.
Cheers,
Ben.
>
> Alex
>
> >
> > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> > ---
> > Compiled, but lack of bare metal, I have not tested it yet.
> > ---
> > arch/powerpc/kvm/book3s_hv.c | 6 ------
> > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 13 ++++++++++++-
> > 2 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv.c
> b/arch/powerpc/kvm/book3s_hv.c
> > index 62a2b5a..73dc852 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -628,12 +628,6 @@ static int kvmppc_handle_exit(struct kvm_run
> *run, struct kvm_vcpu *vcpu,
> > /* hcall - punt to userspace */
> > int i;
> >
> > - if (vcpu->arch.shregs.msr & MSR_PR) {
> > - /* sc 1 from userspace - reflect to guest syscall */
> > - kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_SYSCALL);
> > - r = RESUME_GUEST;
> > - break;
> > - }
> > run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
> > for (i = 0; i < 9; ++i)
> > run->papr_hcall.args[i] = kvmppc_get_gpr(vcpu, 4 + i);
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index c71103b..9f626c3 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -1388,7 +1388,8 @@ kvmppc_hisi:
> > hcall_try_real_mode:
> > ld r3,VCPU_GPR(R3)(r9)
> > andi. r0,r11,MSR_PR
> > - bne guest_exit_cont
> > + /* sc 1 from userspace - reflect to guest syscall */
> > + bne sc_0_fast_return
> > clrrdi r3,r3,2
> > cmpldi r3,hcall_real_table_end - hcall_real_table
> > bge guest_exit_cont
> > @@ -1409,6 +1410,16 @@ hcall_try_real_mode:
> > ld r11,VCPU_MSR(r4)
> > b fast_guest_return
> >
> > +sc_0_fast_return:
> > + ld r10,VCPU_PC(r9)
> > + ld r11,VCPU_MSR(r9)
> > + mtspr SPRN_SRR0,r10
> > + mtspr SPRN_SRR1,r11
> > + li r10, BOOK3S_INTERRUPT_SYSCALL
> > + LOAD_REG_IMMEDIATE(r3,0xffffffff87a0ffff) /* zero 33:36,42:47 */
> > + and r11,r11,r3
> > + b fast_guest_return
> > +
> > /* We've attempted a real mode hcall, but it's punted it back
> > * to userspace. We need to restore some clobbered volatiles
> > * before resuming the pass-it-to-qemu path */
> > --
> > 1.8.1.4
> >
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Mathieu Desnoyers @ 2013-11-07 23:50 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Oleg Nesterov, LKML, Linux PPC dev,
Anton Blanchard, Frederic Weisbecker, Victor Kaplansky,
Paul E. McKenney, Linus Torvalds
In-Reply-To: <20131104112254.GK28601@twins.programming.kicks-ass.net>
* Peter Zijlstra (peterz@infradead.org) wrote:
[...]
Hi Peter,
Looking at this simplified version of perf's ring buffer
synchronization, I get concerned about the following issue:
> /*
> * One important detail is that the kbuf part and the kbuf_writer() are
> * strictly per cpu and we can thus rely on program order for those.
> *
> * Only the userspace consumer can possibly run on another cpu, and thus we
> * need to ensure data consistency for those.
> */
>
> struct buffer {
> u64 size;
> u64 tail;
> u64 head;
> void *data;
> };
>
> struct buffer *kbuf, *ubuf;
>
> /*
> * If there's space in the buffer; store the data @buf; otherwise
> * discard it.
> */
> void kbuf_write(int sz, void *buf)
> {
> u64 tail, head, offset;
>
> do {
> tail = ACCESS_ONCE(ubuf->tail);
> offset = head = kbuf->head;
> if (CIRC_SPACE(head, tail, kbuf->size) < sz) {
> /* discard @buf */
> return;
> }
> head += sz;
> } while (local_cmpxchg(&kbuf->head, offset, head) != offset)
>
Let's suppose we have a thread executing kbuf_write(), interrupted by an
IRQ or NMI right after a successful local_cmpxchg() (space reservation
in the buffer). If the nested execution context also calls kbuf_write(),
it will therefore update ubuf->head (below) with the second reserved
space, and only after that will it return to the original thread context
and continue executing kbuf_write(), thus overwriting ubuf->head with
the prior-to-last reserved offset.
All this probably works OK most of the times, when we have an event flow
guaranteeing that a following event will fix things up, but there
appears to be a risk of losing events near the end of the trace when
those are in nested execution contexts.
Thoughts ?
Thanks,
Mathieu
> /*
> * Ensure that if we see the userspace tail (ubuf->tail) such
> * that there is space to write @buf without overwriting data
> * userspace hasn't seen yet, we won't in fact store data before
> * that read completes.
> */
>
> smp_mb(); /* A, matches with D */
>
> memcpy(kbuf->data + offset, buf, sz);
>
> /*
> * Ensure that we write all the @buf data before we update the
> * userspace visible ubuf->head pointer.
> */
> smp_wmb(); /* B, matches with C */
>
> ubuf->head = kbuf->head;
> }
>
> /*
> * Consume the buffer data and update the tail pointer to indicate to
> * kernel space there's 'free' space.
> */
> void ubuf_read(void)
> {
> u64 head, tail;
>
> tail = ACCESS_ONCE(ubuf->tail);
> head = ACCESS_ONCE(ubuf->head);
>
> /*
> * Ensure we read the buffer boundaries before the actual buffer
> * data...
> */
> smp_rmb(); /* C, matches with B */
>
> while (tail != head) {
> obj = ubuf->data + tail;
> /* process obj */
> tail += obj->size;
> tail %= ubuf->size;
> }
>
> /*
> * Ensure all data reads are complete before we issue the
> * ubuf->tail update; once that update hits, kbuf_write() can
> * observe and overwrite data.
> */
> smp_mb(); /* D, matches with A */
>
> ubuf->tail = tail;
> }
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Alexander Graf @ 2013-11-08 3:10 UTC (permalink / raw)
To: Liu Ping Fan; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <1383878656-4196-1-git-send-email-pingfank@linux.vnet.ibm.com>
On 08.11.2013, at 03:44, Liu Ping Fan <kernelfans@gmail.com> wrote:
> syscall is a very common behavior inside guest, and this patch
> optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
> so hypervisor can return to guest without heavy exit, i.e, no need
> to swap TLB, HTAB,.. etc
The syscall exit you touch here only happens when you do an sc > 0 with =
MSR_PR set inside the guest. The only case you realistically see this is =
when you run PR KVM inside of an HV KVM guest.
I don't think we should optimize for that case. Instead, we should =
rather try to not bounce to the 1st hypervisor in the first place in =
that scenario :).
Alex
>=20
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> Compiled, but lack of bare metal, I have not tested it yet.
> ---
> arch/powerpc/kvm/book3s_hv.c | 6 ------
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 13 ++++++++++++-
> 2 files changed, 12 insertions(+), 7 deletions(-)
>=20
> diff --git a/arch/powerpc/kvm/book3s_hv.c =
b/arch/powerpc/kvm/book3s_hv.c
> index 62a2b5a..73dc852 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -628,12 +628,6 @@ static int kvmppc_handle_exit(struct kvm_run =
*run, struct kvm_vcpu *vcpu,
> /* hcall - punt to userspace */
> int i;
>=20
> - if (vcpu->arch.shregs.msr & MSR_PR) {
> - /* sc 1 from userspace - reflect to guest =
syscall */
> - kvmppc_book3s_queue_irqprio(vcpu, =
BOOK3S_INTERRUPT_SYSCALL);
> - r =3D RESUME_GUEST;
> - break;
> - }
> run->papr_hcall.nr =3D kvmppc_get_gpr(vcpu, 3);
> for (i =3D 0; i < 9; ++i)
> run->papr_hcall.args[i] =3D kvmppc_get_gpr(vcpu, =
4 + i);
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S =
b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index c71103b..9f626c3 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1388,7 +1388,8 @@ kvmppc_hisi:
> hcall_try_real_mode:
> ld r3,VCPU_GPR(R3)(r9)
> andi. r0,r11,MSR_PR
> - bne guest_exit_cont
> + /* sc 1 from userspace - reflect to guest syscall */
> + bne sc_0_fast_return
> clrrdi r3,r3,2
> cmpldi r3,hcall_real_table_end - hcall_real_table
> bge guest_exit_cont
> @@ -1409,6 +1410,16 @@ hcall_try_real_mode:
> ld r11,VCPU_MSR(r4)
> b fast_guest_return
>=20
> +sc_0_fast_return:
> + ld r10,VCPU_PC(r9)
> + ld r11,VCPU_MSR(r9)
> + mtspr SPRN_SRR0,r10
> + mtspr SPRN_SRR1,r11
> + li r10, BOOK3S_INTERRUPT_SYSCALL
> + LOAD_REG_IMMEDIATE(r3,0xffffffff87a0ffff) /* zero =
33:36,42:47 */
> + and r11,r11,r3
> + b fast_guest_return
> +
> /* We've attempted a real mode hcall, but it's punted it back
> * to userspace. We need to restore some clobbered volatiles
> * before resuming the pass-it-to-qemu path */
> --=20
> 1.8.1.4
>=20
^ permalink raw reply
* Re: [PATCH v11 3/3] DMA: Freescale: update driver to support 8-channel DMA engine
From: Dan Williams @ 2013-11-08 2:45 UTC (permalink / raw)
To: Hongbo Zhang
Cc: mark.rutland, devicetree, ian.campbell, pawel.moll,
Stephen Warren, Koul, Vinod, Linux Kernel Mailing List,
rob.herring, dmaengine, linuxppc-dev
In-Reply-To: <5278587F.7090500@freescale.com>
On Mon, Nov 4, 2013 at 6:31 PM, Hongbo Zhang <hongbo.zhang@freescale.com> wrote:
> Hi Vinod Koul and Dan Williams,
> Ping?
>
Not much to review from the dmaengine side, just one question below.
It would be helpful if you can send these to the new dmaengine
patchwork at dmaengine@vger.kernel.org with the Acks you have already
collected.
>
>
> On 10/17/2013 01:56 PM, Hongbo Zhang wrote:
>>
>> Hi Vinod,
>> I have gotten ACK from Mark for both the 1/3 and 2/3 patches.
>> Thanks.
>>
>>
>> On 09/26/2013 05:33 PM, hongbo.zhang@freescale.com wrote:
>>>
>>> From: Hongbo Zhang <hongbo.zhang@freescale.com>
>>>
>>> This patch adds support to 8-channel DMA engine, thus the driver works
>>> for both
>>> the new 8-channel and the legacy 4-channel DMA engines.
>>>
>>> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
>>> ---
>>> drivers/dma/Kconfig | 9 +++++----
>>> drivers/dma/fsldma.c | 9 ++++++---
>>> drivers/dma/fsldma.h | 2 +-
>>> 3 files changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>>> index 6825957..3979c65 100644
>>> --- a/drivers/dma/Kconfig
>>> +++ b/drivers/dma/Kconfig
>>> @@ -89,14 +89,15 @@ config AT_HDMAC
>>> Support the Atmel AHB DMA controller.
>>> config FSL_DMA
>>> - tristate "Freescale Elo and Elo Plus DMA support"
>>> + tristate "Freescale Elo series DMA support"
>>> depends on FSL_SOC
>>> select DMA_ENGINE
>>> select ASYNC_TX_ENABLE_CHANNEL_SWITCH
>>> ---help---
>>> - Enable support for the Freescale Elo and Elo Plus DMA controllers.
>>> - The Elo is the DMA controller on some 82xx and 83xx parts, and the
>>> - Elo Plus is the DMA controller on 85xx and 86xx parts.
>>> + Enable support for the Freescale Elo series DMA controllers.
>>> + The Elo is the DMA controller on some mpc82xx and mpc83xx parts,
>>> the
>>> + EloPlus is on mpc85xx and mpc86xx and Pxxx parts, and the Elo3 is
>>> on
>>> + some Txxx and Bxxx parts.
>>> config MPC512X_DMA
>>> tristate "Freescale MPC512x built-in DMA engine support"
>>> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
>>> index 49e8fbd..16a9a48 100644
>>> --- a/drivers/dma/fsldma.c
>>> +++ b/drivers/dma/fsldma.c
>>> @@ -1261,7 +1261,9 @@ static int fsl_dma_chan_probe(struct fsldma_device
>>> *fdev,
>>> WARN_ON(fdev->feature != chan->feature);
>>> chan->dev = fdev->dev;
>>> - chan->id = ((res.start - 0x100) & 0xfff) >> 7;
>>> + chan->id = (res.start & 0xfff) < 0x300 ?
>>> + ((res.start - 0x100) & 0xfff) >> 7 :
>>> + ((res.start - 0x200) & 0xfff) >> 7;
>>> if (chan->id >= FSL_DMA_MAX_CHANS_PER_DEVICE) {
Isn't it a bit fragile to have this based on the resource address?
Can't device tree tell you the channel id directly by an index into
the "dma0: dma@100300" node?
--
Dan
^ permalink raw reply
* [PATCH] powerpc: kvm: optimize "sc 0" as fast return
From: Liu Ping Fan @ 2013-11-08 2:44 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc; +Cc: Paul Mackerras, Alexander Graf
syscall is a very common behavior inside guest, and this patch
optimizes the path for the emulation of BOOK3S_INTERRUPT_SYSCALL,
so hypervisor can return to guest without heavy exit, i.e, no need
to swap TLB, HTAB,.. etc
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
Compiled, but lack of bare metal, I have not tested it yet.
---
arch/powerpc/kvm/book3s_hv.c | 6 ------
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 13 ++++++++++++-
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 62a2b5a..73dc852 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -628,12 +628,6 @@ static int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
/* hcall - punt to userspace */
int i;
- if (vcpu->arch.shregs.msr & MSR_PR) {
- /* sc 1 from userspace - reflect to guest syscall */
- kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_SYSCALL);
- r = RESUME_GUEST;
- break;
- }
run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
for (i = 0; i < 9; ++i)
run->papr_hcall.args[i] = kvmppc_get_gpr(vcpu, 4 + i);
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index c71103b..9f626c3 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1388,7 +1388,8 @@ kvmppc_hisi:
hcall_try_real_mode:
ld r3,VCPU_GPR(R3)(r9)
andi. r0,r11,MSR_PR
- bne guest_exit_cont
+ /* sc 1 from userspace - reflect to guest syscall */
+ bne sc_0_fast_return
clrrdi r3,r3,2
cmpldi r3,hcall_real_table_end - hcall_real_table
bge guest_exit_cont
@@ -1409,6 +1410,16 @@ hcall_try_real_mode:
ld r11,VCPU_MSR(r4)
b fast_guest_return
+sc_0_fast_return:
+ ld r10,VCPU_PC(r9)
+ ld r11,VCPU_MSR(r9)
+ mtspr SPRN_SRR0,r10
+ mtspr SPRN_SRR1,r11
+ li r10, BOOK3S_INTERRUPT_SYSCALL
+ LOAD_REG_IMMEDIATE(r3,0xffffffff87a0ffff) /* zero 33:36,42:47 */
+ and r11,r11,r3
+ b fast_guest_return
+
/* We've attempted a real mode hcall, but it's punted it back
* to userspace. We need to restore some clobbered volatiles
* before resuming the pass-it-to-qemu path */
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH v2 2/2] powerpc/kvm: remove redundant assignment
From: Liu ping fan @ 2013-11-08 2:11 UTC (permalink / raw)
To: Alexander Graf; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <CF6D863C-3E0A-4BEF-B985-77B336BA92F1@suse.de>
On Thu, Nov 7, 2013 at 6:06 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote:
>
>> ret is assigned twice with the same value, so remove the later one.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> Acked-by: Paul Mackerras <paulus@samba.org>
>
> I suppose my last request for a patch description was slightly too abbrev=
iated :). Sorry about that.
>
> Imagine you are a Linux-stable maintainer. You have about 5000 patches in=
front of you and you want to figure out whether a patch should get backpor=
ted into a stable tree or not.
>
> It's very easy to read through the patch description.
> It's reasonably easy to do a git show on the patch.
> It's very hard to look at the actual surrounding code that was changed.
>
> If I open a text editor on the file, I immediately see what you're saying=
:
>
>> ret =3D RESUME_GUEST;
>> preempt_disable();
>> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK))
>> cpu_relax();
>> if ((hptep[0] & ~HPTE_V_HVLOCK) !=3D hpte[0] || hptep[1] !=3D hp=
te[1] ||
>> rev->guest_rpte !=3D hpte[2])
>> /* HPTE has been changed under us; let the guest retry *=
/
>> goto out_unlock;
>> hpte[0] =3D (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID;
>>
>> rmap =3D &memslot->arch.rmap[gfn - memslot->base_gfn];
>> lock_rmap(rmap);
>>
>> /* Check if we might have been invalidated; let the guest retry =
if so */
>> ret =3D RESUME_GUEST;
>
> However, that scope is not given in the actual patch itself. If you look =
at the diff below, you have no idea whether the patch is fixing a bug or ju=
st removes duplication and doesn't actually have any effect. In fact, the c=
ompiled assembly should be the same with this patch and without. But you ca=
n't tell from the diff below.
>
> So what I would like to see in the patch description is something that ma=
kes it easy to understand what's going on without the need to check out the=
source file. Something like
>
>> We redundantly set ret to RESUME_GUEST twice without changing it in betw=
een. Only do it once:
>>
>> ret =3D RESUME_GUEST;
>> preempt_disable();
>> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK))
>> cpu_relax();
>> if ((hptep[0] & ~HPTE_V_HVLOCK) !=3D hpte[0] || hptep[1] !=3D hp=
te[1] ||
>> rev->guest_rpte !=3D hpte[2])
>> /* HPTE has been changed under us; let the guest retry *=
/
>> goto out_unlock;
>> hpte[0] =3D (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID;
>>
>> rmap =3D &memslot->arch.rmap[gfn - memslot->base_gfn];
>> lock_rmap(rmap);
>>
>> /* Check if we might have been invalidated; let the guest retry =
if so */
>> ret =3D RESUME_GUEST;
>
>
> If I look at that patch description it immediately tells me "Ah, no need =
to worry, it's not a critical bug I need to backport". If you have a better=
idea how to express that I'm more than happy to take that too. Otherwise j=
ust let me know whether you like the description above and I'll modify it t=
o the one that includes the code snippet when applying the patch.
>
Oh, yes. Thank you very much..
And I had a better understanding of the heavy work of maintainers :)
Will keep this in mind.
Best regards,
Pingfan
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/kvm: fix rare but potential deadlock scene
From: Liu ping fan @ 2013-11-08 2:07 UTC (permalink / raw)
To: Alexander Graf; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <7536FFB1-D186-42AC-9DF5-A46A72CB8F1D@suse.de>
On Thu, Nov 7, 2013 at 5:54 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote:
>
>> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
>> realmode, so it can trigger the deadlock.
>>
>> Suppose the following scene:
>>
>> Two physical cpuM, cpuN, two VM instances A, B, each VM has a group of v=
cpus.
>>
>> If on cpuM, vcpu_A_1 holds bitlock X (HPTE_V_HVLOCK), then is switched o=
ut,
>> and on cpuN, vcpu_A_2 try to lock X in realmode, then cpuN will be caugh=
t in
>> realmode for a long time.
>>
>> What makes things even worse if the following happens,
>> On cpuM, bitlockX is hold, on cpuN, Y is hold.
>> vcpu_B_2 try to lock Y on cpuM in realmode
>> vcpu_A_2 try to lock X on cpuN in realmode
>>
>> Oops! deadlock happens
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Very nice catch :).
>
> I think it makes a lot of sense to document the fact that kvmppc_hv_find_=
lock_hpte() should be called with preemption disabled in a comment above th=
e function, so that next time when someone potentially calls it, he knows t=
hat he needs to put preempt_disable() around it.
>
Ok, I will document them in v3
> Thanks a lot for finding this pretty subtle issue. May I ask how you got =
there? Did you actually see systems deadlock because of this?
>
Intuition :). then I begin try to model a scene which causes the
deadlock. And fortunately, I find a case to verify my suspension.
Regards,
Pingfan
>
> Alex
>
>> ---
>> arch/powerpc/kvm/book3s_64_mmu_hv.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book=
3s_64_mmu_hv.c
>> index 043eec8..dbc1478 100644
>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> @@ -474,10 +474,13 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kv=
m_vcpu *vcpu, gva_t eaddr,
>> }
>>
>> /* Find the HPTE in the hash table */
>> + preempt_disable();
>> index =3D kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
>> HPTE_V_VALID | HPTE_V_ABSENT);
>> - if (index < 0)
>> + if (index < 0) {
>> + preempt_enable();
>> return -ENOENT;
>> + }
>> hptep =3D (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
>> v =3D hptep[0] & ~HPTE_V_HVLOCK;
>> gr =3D kvm->arch.revmap[index].guest_rpte;
>> @@ -485,6 +488,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_=
vcpu *vcpu, gva_t eaddr,
>> /* Unlock the HPTE */
>> asm volatile("lwsync" : : : "memory");
>> hptep[0] =3D v;
>> + preempt_enable();
>>
>> gpte->eaddr =3D eaddr;
>> gpte->vpage =3D ((v & HPTE_V_AVPN) << 4) | ((eaddr >> 12) & 0xfff)=
;
>> --
>> 1.8.1.4
>>
>
^ permalink raw reply
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
From: Kevin Hao @ 2013-11-08 1:54 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1383845691.23598.125.camel@snotra.buserror.net>
[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]
On Thu, Nov 07, 2013 at 11:34:51AM -0600, Scott Wood wrote:
> On Thu, 2013-11-07 at 15:17 +0800, Kevin Hao wrote:
> > It makes no sense to initialize the mpic ipi for the SoC which has
> > doorbell support. So set the smp_85xx_ops.probe to NULL for this
> > case. Since the smp_85xx_ops.probe is also used in function
> > smp_85xx_setup_cpu() to check if we need to invoke
> > mpic_setup_this_cpu(), we introduce a new setup_cpu function
> > smp_85xx_basic_setup() to remove this dependency.
>
> Is there any harm caused by setting up the IPIs?
No real harm. Just make no sense to do so and it does cause confusion
when you cat /proc/interrupts and get something like this:
507: 0 0 OpenPIC 2043 Edge ipi call function
508: 0 0 OpenPIC 2044 Edge ipi reschedule
509: 0 0 OpenPIC 2045 Edge ipi call function single
DBL: 7053 10137 Doorbell interrupts
>
> What about other MPIC setup, such as setting the current task priority
> register?
This is done by the invoking of function mpic_setup_this_cpu() in
smp_85xx_setup_cpu().
> BTW, what exactly is probe() supposed to be doing?
It is supposed to do the platform specific smp initialization and then return
the CPU count.
> It looks like its
> main effect (with smp_mpic_probe) is to request IPIs, but the caller
> seems to treat it mainly as a way to determine CPU count.
>
> I looked at the caller of .probe() (which is smp_prepare_cpus()) to see
> what happens when probe is NULL, and the handling of max_cpus doesn't
> make much sense. At first I was concerned by the gratuitous difference
> between smp_mpic_probe() using cpu_possible_mask versus
> smp_prepare_cpus() using NR_CPUS, but the value isn't even used (all the
> code that consumed max_cpus after setting it has been removed), and the
> value passed in to smp_prepare_cpus() is ignored.
Yes, the return value of .probe makes no sense now. Actually there is already
a patch to remove the check of the return value of .probe in function
smp_prepare_cpus().
http://patchwork.ozlabs.org/patch/260574/
Thanks,
Kevin
>
> -Scott
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.
From: Mark Brown @ 2013-11-07 20:38 UTC (permalink / raw)
To: Li Xiubo
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
linux-doc@vger.kernel.org, tiwai@suse.de, timur@tabi.org,
perex@perex.cz, Huan Wang, LW@KARO-electronics.de,
linux@arm.linux.org.uk, Shawn Guo, grant.likely@linaro.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
ian.campbell@citrix.com, pawel.moll@arm.com,
swarren@wwwdotorg.org, rob.herring@calxeda.com, Zhengxiong Jin,
oskar@scara.com, Fabio Estevam, lgirdwood@gmail.com,
linux-kernel@vger.kernel.org, rob@landley.net, Guangyu Chen,
shawn.guo@linaro.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1DD289F6464F0949A2FCA5AA6DC23F828783E6@039-SN2MPN1-013.039d.mgd.msft.net>
[-- Attachment #1: Type: text/plain, Size: 908 bytes --]
On Thu, Nov 07, 2013 at 03:01:02AM +0000, Li Xiubo wrote:
> > > The SGTL5000 is based on regulators and when it is disabled, there
> > > will be an error returns directly while the SGTL5000 codec is probing.
> > What makes you say this?
> static int ldo_regulator_register(struct snd_soc_codec *codec,
> struct regulator_init_data *init_data,
> int voltage)
If the regulator is not used in the system then why is the driver
getting as far as trying to register it? Surely this is a system
configuration error? This all sounds like some problem with either the
system integration or the driver which is causing it to try to register
the regulator needlessly - you should be fixing that problem, not adding
ifdefs. I'm still unclear on exactly what the issue is so it's hard to
say exactly what the best way forwards is.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
From: Scott Wood @ 2013-11-07 17:34 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
In-Reply-To: <1383808637-26769-1-git-send-email-haokexin@gmail.com>
On Thu, 2013-11-07 at 15:17 +0800, Kevin Hao wrote:
> It makes no sense to initialize the mpic ipi for the SoC which has
> doorbell support. So set the smp_85xx_ops.probe to NULL for this
> case. Since the smp_85xx_ops.probe is also used in function
> smp_85xx_setup_cpu() to check if we need to invoke
> mpic_setup_this_cpu(), we introduce a new setup_cpu function
> smp_85xx_basic_setup() to remove this dependency.
Is there any harm caused by setting up the IPIs?
What about other MPIC setup, such as setting the current task priority
register?
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>
> Boot test on p2020rdb and p5020ds.
>
> arch/powerpc/platforms/85xx/smp.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
> index 281b7f01df63..d3b310f87ce9 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -388,15 +388,18 @@ static void mpc85xx_smp_machine_kexec(struct kimage *image)
> }
> #endif /* CONFIG_KEXEC */
>
> -static void smp_85xx_setup_cpu(int cpu_nr)
> +static void smp_85xx_basic_setup(int cpu_nr)
> {
> - if (smp_85xx_ops.probe == smp_mpic_probe)
> - mpic_setup_this_cpu();
> -
> if (cpu_has_feature(CPU_FTR_DBELL))
> doorbell_setup_this_cpu();
> }
>
> +static void smp_85xx_setup_cpu(int cpu_nr)
> +{
> + mpic_setup_this_cpu();
> + smp_85xx_basic_setup(cpu_nr);
> +}
> +
> static const struct of_device_id mpc85xx_smp_guts_ids[] = {
> { .compatible = "fsl,mpc8572-guts", },
> { .compatible = "fsl,p1020-guts", },
> @@ -411,13 +414,14 @@ void __init mpc85xx_smp_init(void)
> {
> struct device_node *np;
>
> - smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu;
>
> np = of_find_node_by_type(NULL, "open-pic");
> if (np) {
> smp_85xx_ops.probe = smp_mpic_probe;
> + smp_85xx_ops.setup_cpu = smp_85xx_setup_cpu;
> smp_85xx_ops.message_pass = smp_mpic_message_pass;
> - }
> + } else
> + smp_85xx_ops.setup_cpu = smp_85xx_basic_setup;
>
> if (cpu_has_feature(CPU_FTR_DBELL)) {
> /*
> @@ -426,6 +430,7 @@ void __init mpc85xx_smp_init(void)
> */
> smp_85xx_ops.message_pass = NULL;
> smp_85xx_ops.cause_ipi = doorbell_cause_ipi;
> + smp_85xx_ops.probe = NULL;
> }
BTW, what exactly is probe() supposed to be doing? It looks like its
main effect (with smp_mpic_probe) is to request IPIs, but the caller
seems to treat it mainly as a way to determine CPU count.
I looked at the caller of .probe() (which is smp_prepare_cpus()) to see
what happens when probe is NULL, and the handling of max_cpus doesn't
make much sense. At first I was concerned by the gratuitous difference
between smp_mpic_probe() using cpu_possible_mask versus
smp_prepare_cpus() using NR_CPUS, but the value isn't even used (all the
code that consumed max_cpus after setting it has been removed), and the
value passed in to smp_prepare_cpus() is ignored.
-Scott
^ permalink raw reply
* Re: [PATCH 4/7] IBM Akebono: Add support to the OHCI platform driver for Akebono
From: Alan Stern @ 2013-11-07 15:04 UTC (permalink / raw)
To: Alistair Popple; +Cc: linux-usb, linuxppc-dev
In-Reply-To: <1462812.9fJW2bi87G@mexican>
On Thu, 7 Nov 2013, Alistair Popple wrote:
> Thanks. Based on the discussion for the EHCI driver I would like to change the
> compatibility string to "usb-ochi" (instead of "ibm,akebono-ohci"). Are you
> still happy for me to add the Acked-by with the alternate compatibility (and
> of course the formatting fix)? No other drivers currently use "usb-ochi" so it
> shouldn't require any merging of drivers.
Yes, go ahead (as long as you use the right spelling, as Ben pointed
out).
Alan Stern
^ permalink raw reply
* re: powerpc: Hugetlb for BookE
From: Dan Carpenter @ 2013-11-07 13:12 UTC (permalink / raw)
To: beckyb; +Cc: linuxppc-dev
Hello Becky Bruce,
This is a semi-automatic email about new static checker warnings.
The patch 41151e77a4d9: "powerpc: Hugetlb for BookE" from Jun 28,
2011, leads to the following Smatch complaint:
arch/powerpc/mm/hugetlbpage-book3e.c:120 flush_hugetlb_page()
warn: variable dereferenced before check 'vma' (see line 117)
arch/powerpc/mm/hugetlbpage-book3e.c
116 {
117 struct hstate *hstate = hstate_file(vma->vm_file);
^^^^^^^^^^^^
Dereference.
118 unsigned long tsize = huge_page_shift(hstate) - 10;
119
120 __flush_tlb_page(vma ? vma->vm_mm : NULL, vmaddr, tsize, 0);
^^^
Check is too late.
121
122 }
Also in the caller:
arch/powerpc/mm/tlb_nohash.c
305 void flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr)
306 {
307 #ifdef CONFIG_HUGETLB_PAGE
308 if (is_vm_hugetlb_page(vma))
^^^^^^^^^^^^^^^^^^^^^^
Dereferenced inside is_vm_hugetlb_page().
309 flush_hugetlb_page(vma, vmaddr);
310 #endif
311
312 __flush_tlb_page(vma ? vma->vm_mm : NULL, vmaddr,
313 mmu_get_tsize(mmu_virtual_psize), 0);
314 }
regards,
dan carpenter
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Peter Zijlstra @ 2013-11-07 13:36 UTC (permalink / raw)
To: Will Deacon
Cc: Michael Neuling, Tony Luck, Mathieu Desnoyers, Heiko Carstens,
Oleg Nesterov, LKML, Linux PPC dev, Geert Uytterhoeven,
Anton Blanchard, Frederic Weisbecker, Victor Kaplansky,
Russell King, Paul E. McKenney, Linus Torvalds,
Martin Schwidefsky
In-Reply-To: <20131107111741.GD13139@mudshark.cambridge.arm.com>
On Thu, Nov 07, 2013 at 11:17:41AM +0000, Will Deacon wrote:
> Hi Peter,
>
> Couple of minor fixes on the arm64 side...
>
> On Wed, Nov 06, 2013 at 01:57:36PM +0000, Peter Zijlstra wrote:
> > --- a/arch/arm64/include/asm/barrier.h
> > +++ b/arch/arm64/include/asm/barrier.h
> > @@ -35,11 +35,59 @@
> > #define smp_mb() barrier()
> > #define smp_rmb() barrier()
> > #define smp_wmb() barrier()
> > +
> > +#define smp_store_release(p, v) \
> > +do { \
> > + compiletime_assert_atomic_type(*p); \
> > + smp_mb(); \
> > + ACCESS_ONCE(*p) = (v); \
> > +} while (0)
> > +
> > +#define smp_load_acquire(p) \
> > +({ \
> > + typeof(*p) ___p1 = ACCESS_ONCE(*p); \
> > + compiletime_assert_atomic_type(*p); \
> > + smp_mb(); \
> > + ___p1; \
> > +})
> > +
> > #else
> > +
> > #define smp_mb() asm volatile("dmb ish" : : : "memory")
> > #define smp_rmb() asm volatile("dmb ishld" : : : "memory")
> > #define smp_wmb() asm volatile("dmb ishst" : : : "memory")
> > -#endif
>
> Why are you getting rid of this #endif?
oops..
> > +#define smp_store_release(p, v) \
> > +do { \
> > + compiletime_assert_atomic_type(*p); \
> > + switch (sizeof(*p)) { \
> > + case 4: \
> > + asm volatile ("stlr %w1, [%0]" \
> > + : "=Q" (*p) : "r" (v) : "memory"); \
> > + break; \
> > + case 8: \
> > + asm volatile ("stlr %1, [%0]" \
> > + : "=Q" (*p) : "r" (v) : "memory"); \
> > + break; \
> > + } \
> > +} while (0)
> > +
> > +#define smp_load_acquire(p) \
> > +({ \
> > + typeof(*p) ___p1; \
> > + compiletime_assert_atomic_type(*p); \
> > + switch (sizeof(*p)) { \
> > + case 4: \
> > + asm volatile ("ldar %w0, [%1]" \
> > + : "=r" (___p1) : "Q" (*p) : "memory"); \
> > + break; \
> > + case 8: \
> > + asm volatile ("ldar %0, [%1]" \
> > + : "=r" (___p1) : "Q" (*p) : "memory"); \
> > + break; \
> > + } \
> > + ___p1; \
> > +})
>
> You don't need the square brackets when using the "Q" constraint (otherwise
> it will expand to something like [[x0]], which gas won't accept).
>
> With those changes, for the general idea and arm/arm64 parts:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
Thanks, I did that split-up I talked about yesterday, I was going to
compile them for all archs I have a compiler for before posting again.
---
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -62,11 +62,11 @@ do { \
compiletime_assert_atomic_type(*p); \
switch (sizeof(*p)) { \
case 4: \
- asm volatile ("stlr %w1, [%0]" \
+ asm volatile ("stlr %w1, %0" \
: "=Q" (*p) : "r" (v) : "memory"); \
break; \
case 8: \
- asm volatile ("stlr %1, [%0]" \
+ asm volatile ("stlr %1, %0" \
: "=Q" (*p) : "r" (v) : "memory"); \
break; \
} \
@@ -78,17 +78,19 @@ do { \
compiletime_assert_atomic_type(*p); \
switch (sizeof(*p)) { \
case 4: \
- asm volatile ("ldar %w0, [%1]" \
+ asm volatile ("ldar %w0, %1" \
: "=r" (___p1) : "Q" (*p) : "memory"); \
break; \
case 8: \
- asm volatile ("ldar %0, [%1]" \
+ asm volatile ("ldar %0, %1" \
: "=r" (___p1) : "Q" (*p) : "memory"); \
break; \
} \
___p1; \
})
+#endif
+
#define read_barrier_depends() do { } while(0)
#define smp_read_barrier_depends() do { } while(0)
^ permalink raw reply
* [PATCH v2] powerpc: memcpy optimization for 64bit LE
From: Philippe Bergheaud @ 2013-11-07 13:01 UTC (permalink / raw)
To: Michael Neuling; +Cc: Linuxppc-dev, anton
In-Reply-To: <CAEjGV6yjBdD5vjLE0tVKJv5EBS8OhkLNmquz0wL8oYk_REETBA@mail.gmail.com>
Unaligned stores take alignment exceptions on POWER7 running in little-endian.
This is a dumb little-endian base memcpy that prevents unaligned stores.
Once booted the feature fixup code switches over to the VMX copy loops
(which are already endian safe).
The question is what we do before that switch over. The base 64bit
memcpy takes alignment exceptions on POWER7 so we can't use it as is.
Fixing the causes of alignment exception would slow it down, because
we'd need to ensure all loads and stores are aligned either through
rotate tricks or bytewise loads and stores. Either would be bad for
all other 64bit platforms.
Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/string.h | 4 ----
arch/powerpc/kernel/ppc_ksyms.c | 2 --
arch/powerpc/lib/Makefile | 2 --
arch/powerpc/lib/memcpy_64.S | 19 +++++++++++++++++++
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/string.h b/arch/powerpc/include/asm/string.h
index 0dffad6..e40010a 100644
--- a/arch/powerpc/include/asm/string.h
+++ b/arch/powerpc/include/asm/string.h
@@ -10,9 +10,7 @@
#define __HAVE_ARCH_STRNCMP
#define __HAVE_ARCH_STRCAT
#define __HAVE_ARCH_MEMSET
-#ifdef __BIG_ENDIAN__
#define __HAVE_ARCH_MEMCPY
-#endif
#define __HAVE_ARCH_MEMMOVE
#define __HAVE_ARCH_MEMCMP
#define __HAVE_ARCH_MEMCHR
@@ -24,9 +22,7 @@ extern int strcmp(const char *,const char *);
extern int strncmp(const char *, const char *, __kernel_size_t);
extern char * strcat(char *, const char *);
extern void * memset(void *,int,__kernel_size_t);
-#ifdef __BIG_ENDIAN__
extern void * memcpy(void *,const void *,__kernel_size_t);
-#endif
extern void * memmove(void *,const void *,__kernel_size_t);
extern int memcmp(const void *,const void *,__kernel_size_t);
extern void * memchr(const void *,int,__kernel_size_t);
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index 526ad5c..0c2dd60 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -147,9 +147,7 @@ EXPORT_SYMBOL(__ucmpdi2);
#endif
long long __bswapdi2(long long);
EXPORT_SYMBOL(__bswapdi2);
-#ifdef __BIG_ENDIAN__
EXPORT_SYMBOL(memcpy);
-#endif
EXPORT_SYMBOL(memset);
EXPORT_SYMBOL(memmove);
EXPORT_SYMBOL(memcmp);
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 5310132..6670361 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -23,9 +23,7 @@ obj-y += checksum_$(CONFIG_WORD_SIZE).o
obj-$(CONFIG_PPC64) += checksum_wrappers_64.o
endif
-ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),)
obj-$(CONFIG_PPC64) += memcpy_power7.o memcpy_64.o
-endif
obj-$(CONFIG_PPC_EMULATE_SSTEP) += sstep.o ldstfp.o
diff --git a/arch/powerpc/lib/memcpy_64.S b/arch/powerpc/lib/memcpy_64.S
index d2bbbc8..358cf74 100644
--- a/arch/powerpc/lib/memcpy_64.S
+++ b/arch/powerpc/lib/memcpy_64.S
@@ -12,10 +12,28 @@
.align 7
_GLOBAL(memcpy)
BEGIN_FTR_SECTION
+#ifdef __LITTLE_ENDIAN__
+ cmpdi cr7,r5,0 /* dumb little-endian memcpy */
+#else
std r3,48(r1) /* save destination pointer for return value */
+#endif
FTR_SECTION_ELSE
b memcpy_power7
ALT_FTR_SECTION_END_IFCLR(CPU_FTR_VMX_COPY)
+#ifdef __LITTLE_ENDIAN__
+ addi r5,r5,-1
+ addi r9,r3,-1
+ add r5,r3,r5
+ subf r5,r9,r5
+ addi r4,r4,-1
+ mtctr r5
+ beqlr cr7
+1:
+ lbzu r10,1(r4)
+ stbu r10,1(r9)
+ bdnz 1b
+ blr
+#else
PPC_MTOCRF(0x01,r5)
cmpldi cr1,r5,16
neg r6,r3 # LS 3 bits = # bytes to 8-byte dest bdry
@@ -201,3 +219,4 @@ END_FTR_SECTION_IFCLR(CPU_FTR_UNALIGNED_LD_STD)
stb r0,0(r3)
4: ld r3,48(r1) /* return dest pointer */
blr
+#endif
-- 1.7.10.4
^ permalink raw reply related
* Re: [PATCH RFC v5 3/5] dma: of: Add common xlate function for matching by channel id
From: Alexander Popov @ 2013-11-07 11:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devicetree, Lars-Peter Clausen, Vinod Koul, Gerhard Sittig,
Alexander Popov, Dan Williams, Anatolij Gustschin, linuxppc-dev
In-Reply-To: <201311010852.05709.arnd@arndb.de>
2013/11/1 Arnd Bergmann <arnd@arndb.de>:
> On Friday 01 November 2013, Alexander Popov wrote:
>> + * of_dma_xlate_by_chan_id - Translate dt property to DMA channel by channel id
>> + * @dma_spec: pointer to DMA specifier as found in the device tree
>> + * @of_dma: pointer to DMA controller data
>
> This seems rather clumsy, now that we have added the dma_get_slave_channel interface.
> Can you try using that instead of dma_request_channel() now?
>
Thanks, Arnd.
I'll return with the fixed version.
Best regards,
Alexander.
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Will Deacon @ 2013-11-07 11:17 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Tony Luck, Mathieu Desnoyers, Heiko Carstens,
Oleg Nesterov, LKML, Linux PPC dev, Geert Uytterhoeven,
Anton Blanchard, Frederic Weisbecker, Victor Kaplansky,
Russell King, Paul E. McKenney, Linus Torvalds,
Martin Schwidefsky
In-Reply-To: <20131106135736.GK10651@twins.programming.kicks-ass.net>
Hi Peter,
Couple of minor fixes on the arm64 side...
On Wed, Nov 06, 2013 at 01:57:36PM +0000, Peter Zijlstra wrote:
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -35,11 +35,59 @@
> #define smp_mb() barrier()
> #define smp_rmb() barrier()
> #define smp_wmb() barrier()
> +
> +#define smp_store_release(p, v) \
> +do { \
> + compiletime_assert_atomic_type(*p); \
> + smp_mb(); \
> + ACCESS_ONCE(*p) = (v); \
> +} while (0)
> +
> +#define smp_load_acquire(p) \
> +({ \
> + typeof(*p) ___p1 = ACCESS_ONCE(*p); \
> + compiletime_assert_atomic_type(*p); \
> + smp_mb(); \
> + ___p1; \
> +})
> +
> #else
> +
> #define smp_mb() asm volatile("dmb ish" : : : "memory")
> #define smp_rmb() asm volatile("dmb ishld" : : : "memory")
> #define smp_wmb() asm volatile("dmb ishst" : : : "memory")
> -#endif
Why are you getting rid of this #endif?
> +#define smp_store_release(p, v) \
> +do { \
> + compiletime_assert_atomic_type(*p); \
> + switch (sizeof(*p)) { \
> + case 4: \
> + asm volatile ("stlr %w1, [%0]" \
> + : "=Q" (*p) : "r" (v) : "memory"); \
> + break; \
> + case 8: \
> + asm volatile ("stlr %1, [%0]" \
> + : "=Q" (*p) : "r" (v) : "memory"); \
> + break; \
> + } \
> +} while (0)
> +
> +#define smp_load_acquire(p) \
> +({ \
> + typeof(*p) ___p1; \
> + compiletime_assert_atomic_type(*p); \
> + switch (sizeof(*p)) { \
> + case 4: \
> + asm volatile ("ldar %w0, [%1]" \
> + : "=r" (___p1) : "Q" (*p) : "memory"); \
> + break; \
> + case 8: \
> + asm volatile ("ldar %0, [%1]" \
> + : "=r" (___p1) : "Q" (*p) : "memory"); \
> + break; \
> + } \
> + ___p1; \
> +})
You don't need the square brackets when using the "Q" constraint (otherwise
it will expand to something like [[x0]], which gas won't accept).
With those changes, for the general idea and arm/arm64 parts:
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/kvm: remove redundant assignment
From: Alexander Graf @ 2013-11-07 10:06 UTC (permalink / raw)
To: Liu Ping Fan; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <1383805375-14766-2-git-send-email-pingfank@linux.vnet.ibm.com>
On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote:
> ret is assigned twice with the same value, so remove the later one.
>=20
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> Acked-by: Paul Mackerras <paulus@samba.org>
I suppose my last request for a patch description was slightly too =
abbreviated :). Sorry about that.
Imagine you are a Linux-stable maintainer. You have about 5000 patches =
in front of you and you want to figure out whether a patch should get =
backported into a stable tree or not.
It's very easy to read through the patch description.
It's reasonably easy to do a git show on the patch.
It's very hard to look at the actual surrounding code that was changed.
If I open a text editor on the file, I immediately see what you're =
saying:
> ret =3D RESUME_GUEST;
> preempt_disable();
> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK))
> cpu_relax();
> if ((hptep[0] & ~HPTE_V_HVLOCK) !=3D hpte[0] || hptep[1] !=3D =
hpte[1] ||
> rev->guest_rpte !=3D hpte[2])
> /* HPTE has been changed under us; let the guest retry =
*/
> goto out_unlock;
> hpte[0] =3D (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID;
>=20
> rmap =3D &memslot->arch.rmap[gfn - memslot->base_gfn];
> lock_rmap(rmap);
>=20
> /* Check if we might have been invalidated; let the guest =
retry if so */
> ret =3D RESUME_GUEST;
However, that scope is not given in the actual patch itself. If you look =
at the diff below, you have no idea whether the patch is fixing a bug or =
just removes duplication and doesn't actually have any effect. In fact, =
the compiled assembly should be the same with this patch and without. =
But you can't tell from the diff below.
So what I would like to see in the patch description is something that =
makes it easy to understand what's going on without the need to check =
out the source file. Something like
> We redundantly set ret to RESUME_GUEST twice without changing it in =
between. Only do it once:
>=20
> ret =3D RESUME_GUEST;
> preempt_disable();
> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK))
> cpu_relax();
> if ((hptep[0] & ~HPTE_V_HVLOCK) !=3D hpte[0] || hptep[1] !=3D =
hpte[1] ||
> rev->guest_rpte !=3D hpte[2])
> /* HPTE has been changed under us; let the guest retry =
*/
> goto out_unlock;
> hpte[0] =3D (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID;
>=20
> rmap =3D &memslot->arch.rmap[gfn - memslot->base_gfn];
> lock_rmap(rmap);
>=20
> /* Check if we might have been invalidated; let the guest =
retry if so */
> ret =3D RESUME_GUEST;
If I look at that patch description it immediately tells me "Ah, no need =
to worry, it's not a critical bug I need to backport". If you have a =
better idea how to express that I'm more than happy to take that too. =
Otherwise just let me know whether you like the description above and =
I'll modify it to the one that includes the code snippet when applying =
the patch.
Thanks a lot,
Alex
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 -
> 1 file changed, 1 deletion(-)
>=20
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c =
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index dbc1478..9b97b42 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -733,7 +733,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run =
*run, struct kvm_vcpu *vcpu,
> lock_rmap(rmap);
>=20
> /* Check if we might have been invalidated; let the guest retry =
if so */
> - ret =3D RESUME_GUEST;
> if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {
> unlock_rmap(rmap);
> goto out_unlock;
> --=20
> 1.8.1.4
>=20
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/kvm: fix rare but potential deadlock scene
From: Alexander Graf @ 2013-11-07 9:54 UTC (permalink / raw)
To: Liu Ping Fan; +Cc: Paul Mackerras, linuxppc-dev, kvm-ppc
In-Reply-To: <1383805375-14766-1-git-send-email-pingfank@linux.vnet.ibm.com>
On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote:
> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
> realmode, so it can trigger the deadlock.
>=20
> Suppose the following scene:
>=20
> Two physical cpuM, cpuN, two VM instances A, B, each VM has a group of =
vcpus.
>=20
> If on cpuM, vcpu_A_1 holds bitlock X (HPTE_V_HVLOCK), then is switched =
out,
> and on cpuN, vcpu_A_2 try to lock X in realmode, then cpuN will be =
caught in
> realmode for a long time.
>=20
> What makes things even worse if the following happens,
> On cpuM, bitlockX is hold, on cpuN, Y is hold.
> vcpu_B_2 try to lock Y on cpuM in realmode
> vcpu_A_2 try to lock X on cpuN in realmode
>=20
> Oops! deadlock happens
>=20
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Very nice catch :).
I think it makes a lot of sense to document the fact that =
kvmppc_hv_find_lock_hpte() should be called with preemption disabled in =
a comment above the function, so that next time when someone potentially =
calls it, he knows that he needs to put preempt_disable() around it.
Thanks a lot for finding this pretty subtle issue. May I ask how you got =
there? Did you actually see systems deadlock because of this?
Alex
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>=20
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c =
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 043eec8..dbc1478 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -474,10 +474,13 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct =
kvm_vcpu *vcpu, gva_t eaddr,
> }
>=20
> /* Find the HPTE in the hash table */
> + preempt_disable();
> index =3D kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
> HPTE_V_VALID | HPTE_V_ABSENT);
> - if (index < 0)
> + if (index < 0) {
> + preempt_enable();
> return -ENOENT;
> + }
> hptep =3D (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
> v =3D hptep[0] & ~HPTE_V_HVLOCK;
> gr =3D kvm->arch.revmap[index].guest_rpte;
> @@ -485,6 +488,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct =
kvm_vcpu *vcpu, gva_t eaddr,
> /* Unlock the HPTE */
> asm volatile("lwsync" : : : "memory");
> hptep[0] =3D v;
> + preempt_enable();
>=20
> gpte->eaddr =3D eaddr;
> gpte->vpage =3D ((v & HPTE_V_AVPN) << 4) | ((eaddr >> 12) & =
0xfff);
> --=20
> 1.8.1.4
>=20
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment
From: Benjamin Herrenschmidt @ 2013-11-07 8:36 UTC (permalink / raw)
To: Alexander Graf
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, Liu Ping Fan
In-Reply-To: <D41E9B02-613E-498D-B68B-18FCAC21637F@suse.de>
On Thu, 2013-11-07 at 09:14 +0100, Alexander Graf wrote:
> > And ? An explanation isn't going to be clearer than the code in that
> > case ...
>
> It's pretty non-obvious when you do a git show on that patch in 1 year
> from now, as the redundancy is out of scope of what the diff shows.
And ? How would an explanation help ?
Either it's redundant or it's not ... but only look at the code can
prove it. An explanation won't because if the patch is wrong, so will be
the explanation.
Cheers,
Ben.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox