* Re: [PATCH RFC 2/2] mm, sched: trigger might_sleep() in might_fault() when pagefaults are disabled
From: Michael S. Tsirkin @ 2014-11-27 17:24 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger,
David.Laight, paulus, schwidefsky, akpm, linuxppc-dev, tglx
In-Reply-To: <1417108217-42687-3-git-send-email-dahi@linux.vnet.ibm.com>
On Thu, Nov 27, 2014 at 06:10:17PM +0100, David Hildenbrand wrote:
> Commit 662bbcb2747c2422cf98d3d97619509379eee466 removed might_sleep() checks
> for all user access code (that uses might_fault()).
>
> The reason was to disable wrong "sleep in atomic" warnings in the following
> scenario:
> pagefault_disable();
> rc = copy_to_user(...);
> pagefault_enable();
>
> Which is valid, as pagefault_disable() increments the preempt counter and
> therefore disables the pagefault handler. copy_to_user() will not sleep and return
> an invalid return code if a page is not available.
>
> However, as all might_sleep() checks are removed, CONFIG_DEBUG_ATOMIC_SLEEP
> would no longer detect the following scenario:
> spin_lock(&lock);
> rc = copy_to_user(...);
> spin_unlock(&lock);
>
> If the kernel is compiled with preemption turned on, the preempt counter would
> be incremented and copy_to_user() would never sleep. However, with preemption
> turned off, the preempt counter will not be touched, we will therefore sleep in
> atomic context. We really want to enable CONFIG_DEBUG_ATOMIC_SLEEP checks for
> user access functions again, otherwise horrible deadlocks might be hard to debug.
>
> Root of all evil is that pagefault_disable() acted almost as preempt_disable(),
> depending on preemption being turned on/off.
>
> As we now have a fixed pagefault_disable() implementation in place, that uses
> own bits in the preempt counter, we can reenable might_sleep() checks.
>
> This patch reverts commit 662bbcb2747c2422cf98d3d97619509379eee466 taking care
> of the !MMU optimization and the new pagefault_disabled() check.
>
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
> include/linux/kernel.h | 9 +++++++--
> mm/memory.c | 15 ++++-----------
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..64b5f93 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -225,9 +225,14 @@ static inline u32 reciprocal_scale(u32 val, u32 ep_ro)
> return (u32)(((u64) val * ep_ro) >> 32);
> }
>
> -#if defined(CONFIG_MMU) && \
> - (defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP))
> +#if defined(CONFIG_MMU) && defined(CONFIG_PROVE_LOCKING)
> void might_fault(void);
> +#elif defined(CONFIG_MMU) && defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> +static inline void might_fault(void)
> +{
> + if (unlikely(!pagefault_disabled()))
> + __might_sleep(__FILE__, __LINE__, 0);
This __FILE__/__FILE__ will always point at kernel.h
You want a macro to wrap this up.
> +}
> #else
> static inline void might_fault(void) { }
> #endif
> diff --git a/mm/memory.c b/mm/memory.c
> index 3e50383..0e59db9 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3699,7 +3699,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
> up_read(&mm->mmap_sem);
> }
>
> -#if defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> +#ifdef CONFIG_PROVE_LOCKING
> void might_fault(void)
> {
> /*
> @@ -3711,17 +3711,10 @@ void might_fault(void)
> if (segment_eq(get_fs(), KERNEL_DS))
> return;
>
> - /*
> - * it would be nicer only to annotate paths which are not under
> - * pagefault_disable, however that requires a larger audit and
> - * providing helpers like get_user_atomic.
> - */
> - if (in_atomic())
> - return;
> -
> - __might_sleep(__FILE__, __LINE__, 0);
> + if (unlikely(!pagefault_disabled()))
> + __might_sleep(__FILE__, __LINE__, 0);
>
> - if (current->mm)
> + if (!in_atomic() && current->mm)
> might_lock_read(¤t->mm->mmap_sem);
> }
> EXPORT_SYMBOL(might_fault);
> --
> 1.8.5.5
^ permalink raw reply
* Re: [PATCH RFC 2/2] mm, sched: trigger might_sleep() in might_fault() when pagefaults are disabled
From: Michael S. Tsirkin @ 2014-11-27 17:32 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger,
David.Laight, paulus, schwidefsky, akpm, linuxppc-dev, tglx
In-Reply-To: <20141127172449.GA30380@redhat.com>
On Thu, Nov 27, 2014 at 07:24:49PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 27, 2014 at 06:10:17PM +0100, David Hildenbrand wrote:
> > Commit 662bbcb2747c2422cf98d3d97619509379eee466 removed might_sleep() checks
> > for all user access code (that uses might_fault()).
> >
> > The reason was to disable wrong "sleep in atomic" warnings in the following
> > scenario:
> > pagefault_disable();
> > rc = copy_to_user(...);
> > pagefault_enable();
> >
> > Which is valid, as pagefault_disable() increments the preempt counter and
> > therefore disables the pagefault handler. copy_to_user() will not sleep and return
> > an invalid return code if a page is not available.
> >
> > However, as all might_sleep() checks are removed, CONFIG_DEBUG_ATOMIC_SLEEP
> > would no longer detect the following scenario:
> > spin_lock(&lock);
> > rc = copy_to_user(...);
> > spin_unlock(&lock);
> >
> > If the kernel is compiled with preemption turned on, the preempt counter would
> > be incremented and copy_to_user() would never sleep. However, with preemption
> > turned off, the preempt counter will not be touched, we will therefore sleep in
> > atomic context. We really want to enable CONFIG_DEBUG_ATOMIC_SLEEP checks for
> > user access functions again, otherwise horrible deadlocks might be hard to debug.
> >
> > Root of all evil is that pagefault_disable() acted almost as preempt_disable(),
> > depending on preemption being turned on/off.
> >
> > As we now have a fixed pagefault_disable() implementation in place, that uses
> > own bits in the preempt counter, we can reenable might_sleep() checks.
> >
> > This patch reverts commit 662bbcb2747c2422cf98d3d97619509379eee466 taking care
> > of the !MMU optimization and the new pagefault_disabled() check.
> >
> > Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > ---
> > include/linux/kernel.h | 9 +++++++--
> > mm/memory.c | 15 ++++-----------
> > 2 files changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 3d770f55..64b5f93 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -225,9 +225,14 @@ static inline u32 reciprocal_scale(u32 val, u32 ep_ro)
> > return (u32)(((u64) val * ep_ro) >> 32);
> > }
> >
> > -#if defined(CONFIG_MMU) && \
> > - (defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP))
> > +#if defined(CONFIG_MMU) && defined(CONFIG_PROVE_LOCKING)
> > void might_fault(void);
> > +#elif defined(CONFIG_MMU) && defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> > +static inline void might_fault(void)
> > +{
> > + if (unlikely(!pagefault_disabled()))
> > + __might_sleep(__FILE__, __LINE__, 0);
>
> This __FILE__/__FILE__ will always point at kernel.h
>
> You want a macro to wrap this up.
>
> > +}
> > #else
> > static inline void might_fault(void) { }
> > #endif
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 3e50383..0e59db9 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3699,7 +3699,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
> > up_read(&mm->mmap_sem);
> > }
> >
> > -#if defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> > +#ifdef CONFIG_PROVE_LOCKING
> > void might_fault(void)
> > {
> > /*
> > @@ -3711,17 +3711,10 @@ void might_fault(void)
> > if (segment_eq(get_fs(), KERNEL_DS))
> > return;
> >
> > - /*
> > - * it would be nicer only to annotate paths which are not under
> > - * pagefault_disable, however that requires a larger audit and
> > - * providing helpers like get_user_atomic.
> > - */
> > - if (in_atomic())
> > - return;
> > -
> > - __might_sleep(__FILE__, __LINE__, 0);
> > + if (unlikely(!pagefault_disabled()))
> > + __might_sleep(__FILE__, __LINE__, 0);
> >
Same here: so maybe make might_fault a wrapper
around __might_fault as well.
> > - if (current->mm)
> > + if (!in_atomic() && current->mm)
> > might_lock_read(¤t->mm->mmap_sem);
> > }
> > EXPORT_SYMBOL(might_fault);
> > --
> > 1.8.5.5
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Peter Bergner @ 2014-11-27 17:41 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Anton Blanchard, paulus
In-Reply-To: <20141127160829.GA26139@gate.crashing.org>
On Thu, 2014-11-27 at 10:08 -0600, Segher Boessenkool wrote:
> On Wed, Nov 26, 2014 at 05:50:27PM -0600, Peter Bergner wrote:
> > Nope, you don't get a SIGILL when executing 64-bit instructions in
> > 32-bit mode, so it'll happily just execute the instruction, doing
> > a full 64-bit compare. I'm guessing that the upper 32-bits of both
> > r3 and r4 contain zeros, so we're probably just getting lucky.
>
> You will get a SIGILL if you run on 32-bit hardware.
Ha, I completely forgot about 32-bit hardware. Anyway, I looked
at the ISA, and cmpdi and cmpwi are just extended mnemonics for
cmpi, with cmpdi setting the L field to 1. Probably on 32-bit
hardware, the hardware is just ignoring the L bit being set and
doing a cmpwi for us???
Peter
^ permalink raw reply
* Re: [PATCH RFC 2/2] mm, sched: trigger might_sleep() in might_fault() when pagefaults are disabled
From: David Hildenbrand @ 2014-11-27 18:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger,
David.Laight, paulus, schwidefsky, akpm, linuxppc-dev, tglx
In-Reply-To: <20141127173218.GA30419@redhat.com>
> > > -
> > > - __might_sleep(__FILE__, __LINE__, 0);
> > > + if (unlikely(!pagefault_disabled()))
> > > + __might_sleep(__FILE__, __LINE__, 0);
> > >
>
> Same here: so maybe make might_fault a wrapper
> around __might_fault as well.
Yes, I also noticed that. It was part of the original code.
For now I kept this revert as close as possible to
the original patch.
Better fix this in an add-on patch? Or directly in this commit? At least for
the in-header function it is easy to fix in this patch.
Thanks!
^ permalink raw reply
* Re: [PATCH RFC 2/2] mm, sched: trigger might_sleep() in might_fault() when pagefaults are disabled
From: Michael S. Tsirkin @ 2014-11-27 18:27 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger,
David.Laight, paulus, schwidefsky, akpm, linuxppc-dev, tglx
In-Reply-To: <20141127190842.75a9cce3@thinkpad-w530>
On Thu, Nov 27, 2014 at 07:08:42PM +0100, David Hildenbrand wrote:
> > > > -
> > > > - __might_sleep(__FILE__, __LINE__, 0);
> > > > + if (unlikely(!pagefault_disabled()))
> > > > + __might_sleep(__FILE__, __LINE__, 0);
> > > >
> >
> > Same here: so maybe make might_fault a wrapper
> > around __might_fault as well.
>
> Yes, I also noticed that. It was part of the original code.
> For now I kept this revert as close as possible to
> the original patch.
>
> Better fix this in an add-on patch? Or directly in this commit?
IMHO it's up to you really.
> At least for
> the in-header function it is easy to fix in this patch.
>
> Thanks!
Right.
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Andreas Schwab @ 2014-11-27 18:20 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Anton Blanchard, paulus
In-Reply-To: <20141127160829.GA26139@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Wed, Nov 26, 2014 at 05:50:27PM -0600, Peter Bergner wrote:
>> On Thu, 2014-11-27 at 09:38 +1100, Michael Ellerman wrote:
>> > On Thu, 2014-11-27 at 08:11 +1100, Anton Blanchard wrote:
>> > > I used some 64 bit instructions when adding the 32 bit getcpu VDSO
>> > > function. Fix it.
>> >
>> > Ouch. The symptom is a SIGILL I presume?
>>
>> Nope, you don't get a SIGILL when executing 64-bit instructions in
>> 32-bit mode, so it'll happily just execute the instruction, doing
>> a full 64-bit compare. I'm guessing that the upper 32-bits of both
>> r3 and r4 contain zeros, so we're probably just getting lucky.
>
> You will get a SIGILL if you run on 32-bit hardware.
Not on the 7447A, fwiw.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Segher Boessenkool @ 2014-11-27 20:50 UTC (permalink / raw)
To: Peter Bergner; +Cc: linuxppc-dev, Anton Blanchard, paulus
In-Reply-To: <1417110100.16862.36.camel@otta>
On Thu, Nov 27, 2014 at 11:41:40AM -0600, Peter Bergner wrote:
> On Thu, 2014-11-27 at 10:08 -0600, Segher Boessenkool wrote:
> > On Wed, Nov 26, 2014 at 05:50:27PM -0600, Peter Bergner wrote:
> > > Nope, you don't get a SIGILL when executing 64-bit instructions in
> > > 32-bit mode, so it'll happily just execute the instruction, doing
> > > a full 64-bit compare. I'm guessing that the upper 32-bits of both
> > > r3 and r4 contain zeros, so we're probably just getting lucky.
> >
> > You will get a SIGILL if you run on 32-bit hardware.
>
> Ha, I completely forgot about 32-bit hardware. Anyway, I looked
> at the ISA, and cmpdi and cmpwi are just extended mnemonics for
> cmpi, with cmpdi setting the L field to 1. Probably on 32-bit
> hardware, the hardware is just ignoring the L bit being set and
> doing a cmpwi for us???
Huh. Yes, maybe some implementations do that.
The good news is that those then compute the correct thing ;-)
Can QEMU help catch such bugs more reliably?
Segher
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Thomas Gleixner @ 2014-11-27 21:52 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, Michael S. Tsirkin, Heiko Carstens, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141127161905.7c6220ee@thinkpad-w530>
On Thu, 27 Nov 2014, David Hildenbrand wrote:
> > OTOH, there is no reason why we need to disable preemption over that
> > page_fault_disabled() region. There are code pathes which really do
> > not require to disable preemption for that.
> >
> > We have that seperated in preempt-rt for obvious reasons and IIRC
> > Peter Zijlstra tried to distangle it in mainline some time ago. I
> > forgot why that never got merged.
> >
>
> Of course, we can completely separate that in our page fault code by doing
> pagefault_disabled() checks instead of in_atomic() checks (even in add on
> patches later).
>
> > We tie way too much stuff on the preemption count already, which is a
> > mightmare because we have no clear distinction of protection
> > scopes.
>
> Although it might not be optimal, but keeping a separate counter for
> pagefault_disable() as part of the preemption counter seems to be the only
> doable thing right now.
It needs to be seperate, if it should be useful. Otherwise we just
have a extra accounting in preempt_count() which does exactly the same
thing as we have now: disabling preemption.
Now you might say, that we could mask out that part when checking
preempt_count, but that wont work on x86 as x86 has the preempt
counter as a per cpu variable and not as a per thread one.
But if you want to distangle pagefault disable from preempt disable
then you must move it to the thread, because it is a property of the
thread. preempt count is very much a per cpu counter as you can only
go through schedule when it becomes 0.
Btw, I find the x86 representation way more clear, because it
documents that preempt count is a per cpu BKL and not a magic thread
property. And sadly that is how preempt count is used ...
> I am not sure if a completely separated counter is even possible,
> increasing the size of thread_info.
And adding a ulong to thread_info is going to create exactly which
problem?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v2 3/4] powernv: cpuidle: Redesign idle states management
From: Paul Mackerras @ 2014-11-27 23:50 UTC (permalink / raw)
To: Shreyas B. Prabhu; +Cc: linux-pm, Rafael J. Wysocki, linux-kernel, linuxppc-dev
In-Reply-To: <1416914279-30384-4-git-send-email-shreyas@linux.vnet.ibm.com>
On Tue, Nov 25, 2014 at 04:47:58PM +0530, Shreyas B. Prabhu wrote:
[snip]
> +2:
> + /* Sleep or winkle */
> + li r7,1
> + mfspr r8,SPRN_PIR
> + /*
> + * The last 3 bits of PIR represents the thread id of a cpu
> + * in power8. This will need adjusting for power7.
> + */
> + andi. r8,r8,0x07 /* Get thread id into r8 */
> + rotld r7,r7,r8
I would suggest adding another u8 field to the paca to store our
thread bit, and initialize it to 1 << (cpu_id % threads_per_core)
early on. That will handle the POWER7 case correctly and reduce these
four instructions to one.
> +
> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13)
> +lwarx_loop1:
> + lwarx r15,0,r14
> + andc r15,r15,r7 /* Clear thread bit */
> +
> + andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
> + beq last_thread
> +
> + /* Not the last thread to goto sleep */
> + stwcx. r15,0,r14
> + bne- lwarx_loop1
> + b common_enter
> +
> +last_thread:
> + LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround)
> + lbz r3,0(r3)
> + cmpwi r3,1
> + bne common_enter
> + /*
> + * Last thread of the core entering sleep. Last thread needs to execute
> + * the hardware bug workaround code. Before that, set the lock bit to
> + * avoid the race of other threads waking up and undoing workaround
> + * before workaround is applied.
> + */
> + ori r15,r15,PNV_CORE_IDLE_LOCK_BIT
> + stwcx. r15,0,r14
> + bne- lwarx_loop1
> +
> + /* Fast sleep workaround */
> + li r3,1
> + li r4,1
> + li r0,OPAL_CONFIG_CPU_IDLE_STATE
> + bl opal_call_realmode
> +
> + /* Clear Lock bit */
> + andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
> + stw r15,0(r14)
In this case we know the result of the andi. will be 0, so this could
be just li r0,0; stw r0,0(r14).
> +
> +common_enter: /* common code for all the threads entering sleep */
> + IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>
> _GLOBAL(power7_idle)
> /* Now check if user or arch enabled NAP mode */
> @@ -141,49 +191,16 @@ _GLOBAL(power7_idle)
>
> _GLOBAL(power7_nap)
> mr r4,r3
> - li r3,0
> + li r3,PNV_THREAD_NAP
> b power7_powersave_common
> /* No return */
>
> _GLOBAL(power7_sleep)
> - li r3,1
> + li r3,PNV_THREAD_SLEEP
> li r4,1
> b power7_powersave_common
> /* No return */
>
> -/*
> - * Make opal call in realmode. This is a generic function to be called
> - * from realmode from reset vector. It handles endianess.
> - *
> - * r13 - paca pointer
> - * r1 - stack pointer
> - * r3 - opal token
> - */
> -opal_call_realmode:
> - mflr r12
> - std r12,_LINK(r1)
> - ld r2,PACATOC(r13)
> - /* Set opal return address */
> - LOAD_REG_ADDR(r0,return_from_opal_call)
> - mtlr r0
> - /* Handle endian-ness */
> - li r0,MSR_LE
> - mfmsr r12
> - andc r12,r12,r0
> - mtspr SPRN_HSRR1,r12
> - mr r0,r3 /* Move opal token to r0 */
> - LOAD_REG_ADDR(r11,opal)
> - ld r12,8(r11)
> - ld r2,0(r11)
> - mtspr SPRN_HSRR0,r12
> - hrfid
> -
> -return_from_opal_call:
> - FIXUP_ENDIAN
> - ld r0,_LINK(r1)
> - mtlr r0
> - blr
> -
> #define CHECK_HMI_INTERRUPT \
> mfspr r0,SPRN_SRR1; \
> BEGIN_FTR_SECTION_NESTED(66); \
> @@ -196,10 +213,8 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \
> /* Invoke opal call to handle hmi */ \
> ld r2,PACATOC(r13); \
> ld r1,PACAR1(r13); \
> - std r3,ORIG_GPR3(r1); /* Save original r3 */ \
> - li r3,OPAL_HANDLE_HMI; /* Pass opal token argument*/ \
> + li r0,OPAL_HANDLE_HMI; /* Pass opal token argument*/ \
> bl opal_call_realmode; \
> - ld r3,ORIG_GPR3(r1); /* Restore original r3 */ \
> 20: nop;
>
>
> @@ -210,12 +225,91 @@ _GLOBAL(power7_wakeup_tb_loss)
> BEGIN_FTR_SECTION
> CHECK_HMI_INTERRUPT
> END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
> +
> + li r7,1
> + mfspr r8,SPRN_PIR
> + /*
> + * The last 3 bits of PIR represents the thread id of a cpu
> + * in power8. This will need adjusting for power7.
> + */
> + andi. r8,r8,0x07 /* Get thread id into r8 */
> + rotld r7,r7,r8
> + /* r7 now has 'thread_id'th bit set */
> +
> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13)
> +lwarx_loop2:
> + lwarx r15,0,r14
> + andi. r9,r15,PNV_CORE_IDLE_LOCK_BIT
> + /*
> + * Lock bit is set in one of the 2 cases-
> + * a. In the sleep/winkle enter path, the last thread is executing
> + * fastsleep workaround code.
> + * b. In the wake up path, another thread is executing fastsleep
> + * workaround undo code or resyncing timebase or restoring context
> + * In either case loop until the lock bit is cleared.
> + */
> + bne lwarx_loop2
> +
> + cmpwi cr2,r15,0
> + or r15,r15,r7 /* Set thread bit */
> +
> + beq cr2,first_thread
> +
> + /* Not first thread in core to wake up */
> + stwcx. r15,0,r14
> + bne- lwarx_loop2
> + b common_exit
> +
> +first_thread:
> + /* First thread in core to wakeup */
> + ori r15,r15,PNV_CORE_IDLE_LOCK_BIT
> + stwcx. r15,0,r14
> + bne- lwarx_loop2
> +
> + LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround)
> + lbz r3,0(r3)
> + cmpwi r3,1
> + /* skip fastsleep workaround if its not needed */
> + bne timebase_resync
> +
> + /* Undo fast sleep workaround */
> + mfcr r16 /* Backup CR into a non-volatile register */
> + li r3,1
> + li r4,0
> + li r0,OPAL_CONFIG_CPU_IDLE_STATE
> + bl opal_call_realmode
> + mtcr r16 /* Restore CR */
> +
> + /* Do timebase resync if we are waking up from sleep. Use cr1 value
> + * set in exceptions-64s.S */
> + ble cr1,clear_lock
> +
> +timebase_resync:
> /* Time base re-sync */
> - li r3,OPAL_RESYNC_TIMEBASE
> + li r0,OPAL_RESYNC_TIMEBASE
> bl opal_call_realmode;
So if pnv_need_fastsleep_workaround is zero, we always do the timebase
resync, but if pnv_need_fastsleep_workaround is one, we only do the
timebase resync if we had a loss of state. Is that really what you
meant?
> -
> /* TODO: Check r3 for failure */
>
> +clear_lock:
> + andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
> + stw r15,0(r14)
> +
> +common_exit:
> + li r5,PNV_THREAD_RUNNING
> + stb r5,PACA_THREAD_IDLE_STATE(r13)
> +
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + li r0,KVM_HWTHREAD_IN_KERNEL
> + stb r0,HSTATE_HWTHREAD_STATE(r13)
> + /* Order setting hwthread_state vs. testing hwthread_req */
> + sync
> + lbz r0,HSTATE_HWTHREAD_REQ(r13)
> + cmpwi r0,0
> + beq 6f
> + b kvm_start_guest
> +6:
> +#endif
I'd prefer not to duplicate this code. Could you instead branch back
to the code in exceptions-64s.S? Or call this code via a bl and get
back to exceptions-64s.S via a blr.
> +
> REST_NVGPRS(r1)
> REST_GPR(2, r1)
> ld r3,_CCR(r1)
> diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
> index feb549a..b2aa93b 100644
> --- a/arch/powerpc/platforms/powernv/opal-wrappers.S
> +++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
> @@ -158,6 +158,43 @@ opal_tracepoint_return:
> blr
> #endif
>
> +/*
> + * Make opal call in realmode. This is a generic function to be called
> + * from realmode. It handles endianness.
> + *
> + * r13 - paca pointer
> + * r1 - stack pointer
> + * r0 - opal token
> + */
> +_GLOBAL(opal_call_realmode)
> + mflr r12
> + std r12,_LINK(r1)
This is a bug waiting to happen. Using _LINK(r1) was OK in this
code's previous location, since there we know there is a
INT_FRAME_SIZE-sized stack frame and the _LINK field is basically
unused. Now that you're making this available to call from anywhere,
you can't trash the caller's stack frame like this. You need to use
PPC_LR_STKOFF(r1) instead.
> + ld r2,PACATOC(r13)
> + /* Set opal return address */
> + LOAD_REG_ADDR(r12,return_from_opal_call)
> + mtlr r12
> +
> + mfmsr r12
> +#ifdef __LITTLE_ENDIAN__
> + /* Handle endian-ness */
> + li r11,MSR_LE
> + andc r12,r12,r11
> +#endif
> + mtspr SPRN_HSRR1,r12
> + LOAD_REG_ADDR(r11,opal)
> + ld r12,8(r11)
> + ld r2,0(r11)
> + mtspr SPRN_HSRR0,r12
> + hrfid
> +
> +return_from_opal_call:
> +#ifdef __LITTLE_ENDIAN__
> + FIXUP_ENDIAN
> +#endif
> + ld r12,_LINK(r1)
> + mtlr r12
> + blr
Paul.
^ permalink raw reply
* Re: powerpc/powernv: Add debugfs file to grab opalv3 trace data
From: Michael Ellerman @ 2014-11-28 0:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev, Rusty Russell
In-Reply-To: <1416975004.5089.48.camel@kernel.crashing.org>
On Wed, 2014-26-11 at 04:10:04 UTC, Benjamin Herrenschmidt wrote:
> This adds files in debugfs that can be used to retrieve the
> OPALv3 firmware "live binary traces" which can then be parsed
> using a userspace tool.
>
> Mostly from Rusty with some updates by myself (BenH)
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Conspicuous review of patch from maintainer follows ...
Meta-comment: given we need a userspace tool to read the trace anyway, could we
make this a lot simpler by just letting userspace mmap the trace buffers?
> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> index f241acc..315a825 100644
> --- a/arch/powerpc/platforms/powernv/Makefile
> +++ b/arch/powerpc/platforms/powernv/Makefile
> @@ -1,7 +1,7 @@
> obj-y += setup.o opal-wrappers.o opal.o opal-async.o
> obj-y += opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
> obj-y += rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> -obj-y += opal-msglog.o opal-hmi.o
> +obj-y += opal-msglog.o opal-hmi.o opal-trace.o
Should depend on CONFIG_DEBUG_FS at least no?
> diff --git a/arch/powerpc/platforms/powernv/opal-trace-types.h b/arch/powerpc/platforms/powernv/opal-trace-types.h
> new file mode 100644
> index 0000000..3bd8ac2
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-trace-types.h
> @@ -0,0 +1,58 @@
> +/* API for kernel to read trace buffer. */
Copyright/GPL header ?
/*
* Copyright 201x, Purple Monkey Dishwasher, IBM Corporation.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version
* 2 of the License, or (at your option) any later version.
*/
> +#ifndef __OPAL_TRACE_TYPES_H
> +#define __OPAL_TRACE_TYPES_H
We usually do __POWERNV_FOO_H for these. But they're all a bit of a mess.
> +#define TRACE_REPEAT 1
> +#define TRACE_OVERFLOW 2
> +#define TRACE_OPAL 3
> +#define TRACE_FSP 4
Linux tracepoints defines a bunch of TRACE_FOO macros, obviously REPEAT is the
only one that's likely to clash. But still might be worth namespacing.
> +/* One per cpu, plus one for NMIs */
> +struct tracebuf {
> + /* Mask to apply to get buffer offset. */
> + __be64 mask;
> + /* This where the buffer starts. */
> + __be64 start;
> + /* This is where writer has written to. */
> + __be64 end;
> + /* This is where the writer wrote to previously. */
> + __be64 last;
> + /* This is where the reader is up to. */
> + __be64 rpos;
> + /* If the last one we read was a repeat, this shows how many. */
> + __be32 last_repeat;
So I think start/end/last/rpos/last_repeat are all full virtual addresses right?
> + /* Maximum possible size of a record. */
> + __be32 max_size;
> +
> + char buf[/* TBUF_SZ + max_size */];
We don't have TBUF_SZ.
> +};
> +
> +/* Common header for all trace entries. */
> +struct trace_hdr {
> + __be64 timestamp;
> + u8 type;
> + u8 len_div_8;
> + __be16 cpu;
> + u8 unused[4];
> +};
> +
The comment below is attached to trace_repeat, but it seems like it should go
with hdr no ?
> +/* Note: all other entries must be at least as large as this! */
> +struct trace_repeat {
> + __be64 timestamp; /* Last repeat happened at this timestamp */
> + u8 type; /* == TRACE_REPEAT */
> + u8 len_div_8;
> + __be16 cpu;
> + __be16 prev_len;
> + __be16 num; /* Starts at 1, ie. 1 repeat, or two traces. */
> + /* Note that the count can be one short, if read races a repeat. */
> +};
> +
> +struct trace_overflow {
> + __be64 unused64; /* Timestamp is unused */
> + u8 type; /* == TRACE_OVERFLOW */
> + u8 len_div_8;
> + u8 unused[6]; /* ie. hdr.cpu is indeterminate */
> + __be64 bytes_missed;
> +};
Some lining up of comments would help readability of those.
> diff --git a/arch/powerpc/platforms/powernv/opal-trace.c b/arch/powerpc/platforms/powernv/opal-trace.c
> new file mode 100644
> index 0000000..6529756
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-trace.c
> @@ -0,0 +1,202 @@
> +/*
> + * Copyright (C) 2013 Rusty Russell, IBM Corporation
> + *
GPL ?
> + * Simple debugfs file firmware_trace to read out OPALv3 trace
^
now called opal-trace
> + * ringbuffers.
> + */
<blank>
> +#include <linux/mutex.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <asm/debug.h>
> +#include <asm/opal.h>
> +
> +#include "opal-trace-types.h"
> +
> +static DEFINE_MUTEX(tracelock);
> +static struct tracebuf **opal_tb;
> +static size_t opal_num_tb;
> +static __be64 *opal_tmask_p;
_p ? pointer, or phys?
> +
> +/* Maximum possible size of record (since len is 8 bits). */
> +union max_trace {
> + struct trace_hdr hdr;
> + struct trace_overflow overflow;
> + struct trace_repeat repeat;
> + char buf[255 * 8];
> +};
> +static union max_trace trace;
I *think* this is only used in opal_trace_read(), so it'd be nice if it was in
there.
> +static bool trace_empty(const struct tracebuf *tb)
> +{
> + const struct trace_repeat *rep;
> +
> + if (tb->rpos == tb->end)
> + return true;
> +
> + /*
> + * If we have a single element only, and it's a repeat buffer
> + * we've already seen every repeat for (yet which may be
> + * incremented in future), we're also empty.
> + */
> + rep = (void *)tb->buf + (be64_to_cpu(tb->rpos & tb->mask));
> + if (be64_to_cpu(tb->end) != be64_to_cpu(tb->rpos) + sizeof(*rep))
> + return false;
> +
> + if (rep->type != TRACE_REPEAT)
> + return false;
> +
> + if (be16_to_cpu(rep->num) != be32_to_cpu(tb->last_repeat))
> + return false;
> +
> + return true;
> +}
> +
> +/* You can't read in parallel, so some locking required in caller. */
> +static bool trace_get(union max_trace *t, struct tracebuf *tb)
> +{
> + u64 start, rpos;
> +
> + if (trace_empty(tb))
> + return false;
> +
> +again:
> + /*
> + * The actual buffer is slightly larger than tbsize, so this
> + * memcpy is always valid.
We don't seem to have tbsize anymore?
> + */
> + memcpy(t, tb->buf + be64_to_cpu(tb->rpos & tb->mask),
> + be32_to_cpu(tb->max_size));
> +
> + rmb(); /* read barrier, so we read tb->start after copying record. */
> +
> + start = be64_to_cpu(tb->start);
> + rpos = be64_to_cpu(tb->rpos);
> +
> + /* Now, was that overwritten? */
> + if (rpos < start) {
> + /* Create overflow record. */
> + t->overflow.unused64 = 0;
> + t->overflow.type = TRACE_OVERFLOW;
> + t->overflow.len_div_8 = sizeof(t->overflow) / 8;
> + t->overflow.bytes_missed = cpu_to_be64(start - rpos);
> + tb->rpos = cpu_to_be64(start);
> + return true;
> + }
> +
> + /* Repeat entries need special handling */
> + if (t->hdr.type == TRACE_REPEAT) {
> + u32 num = be16_to_cpu(t->repeat.num);
> +
> + /* In case we've read some already... */
> + t->repeat.num = cpu_to_be16(num - be32_to_cpu(tb->last_repeat));
> +
> + /* Record how many repeats we saw this time. */
> + tb->last_repeat = cpu_to_be32(num);
> +
> + /* Don't report an empty repeat buffer. */
> + if (t->repeat.num == 0) {
> + /*
> + * This can't be the last buffer, otherwise
> + * trace_empty would have returned true.
> + */
> + BUG_ON(be64_to_cpu(tb->end) <= rpos + t->hdr.len_div_8 * 8);
Can we just WARN_ON() and bail, seeing as this is for debug. I'd hate to panic
a customer system by dumping the trace buffer.
> + /* Skip to next entry. */
> + tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
> + goto again;
> + }
> + } else {
> + tb->last_repeat = 0;
> + tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
> + }
> +
> + return true;
> +}
> +
> +/* Horrible polling interface, designed for dumping. */
> +static ssize_t opal_trace_read(struct file *file, char __user *ubuf,
> + size_t count, loff_t *ppos)
> +{
> + ssize_t err;
> + unsigned int i;
> +
> + err = mutex_lock_interruptible(&tracelock);
> + if (err)
> + return err;
> +
> + for (i = 0; i < opal_num_tb; i++) {
OK so I understand this now. You have multiple trace buffers, but you don't
care about maintaining that separation. You just merge all the streams here
into a single output stream.
> + if (trace_get(&trace, opal_tb[i])) {
> + size_t len = trace.hdr.len_div_8 * 8;
> + if (len > count)
> + len = count;
> + if (copy_to_user(ubuf, &trace, len) != 0)
> + err = -EFAULT;
> + else
> + err = len;
> + break;
> + }
> + }
> +
> + mutex_unlock(&tracelock);
> + return err;
> +}
> +
> +static const struct file_operations opal_trace_fops = {
> + .read = opal_trace_read,
> + .open = simple_open,
> + .llseek = noop_llseek,
> +};
> +
> +static int opal_tmask_set(void *data, u64 val)
> +{
> + *(__be64 *)data = cpu_to_be64(val);
> + return 0;
> +}
> +static int opal_tmask_get(void *data, u64 *val)
> +{
> + *val = be64_to_cpup((__be64 *)data);
> + return 0;
> +}
> +DEFINE_SIMPLE_ATTRIBUTE(opal_tmask, opal_tmask_get, opal_tmask_set, "%llx\n");
What is a tmask ?
I assume it's some mask of things we want traced?
> +static int opal_trace_init(void)
> +{
> + const __be64 *traces;
> + int len, i, rc;
> + u64 tmask_phys;
> +
> + if (!opal_node)
> + return -ENODEV;
> +
> + traces = of_get_property(opal_node, "ibm,opal-traces", &len);
> + if (!traces) {
> + pr_warning("%s: OPAL node property \"ibm,opal-traces\""
> + " not found\n", __func__);
> + return -ENODEV;
> + }
> +
> + opal_num_tb = len / (sizeof(__be64) * 2);
So I won't say the "b" word, but it'd be nice to have at least a comment on
what the device tree property contains.
> + if (!opal_num_tb) {
> + pr_warning("%s: OPAL traces property has invalid length %i\n",
> + __func__, len);
> + return -EINVAL;
> + }
> + opal_tb = kmalloc(sizeof(*opal_tb) * opal_num_tb, GFP_KERNEL);
> + for (i = 0; i < opal_num_tb; i++)
> + opal_tb[i] = __va(be64_to_cpu(traces[i*2]));
Just __va() ? ie. it's already in the linear mapping somewhere?
> + debugfs_create_file("opal-trace", S_IRUSR, powerpc_debugfs_root,
> + NULL, &opal_trace_fops);
> + rc = of_property_read_u64(opal_node, "ibm,opal-trace-mask",
> + &tmask_phys);
> + if (!rc)
> + opal_tmask_p = __va(tmask_phys);
> + if (opal_tmask_p)
If tmask_phys was 0, opal_tmask_p is now 0xc00..00, so is that what we want to check?
> + debugfs_create_file("opal-trace-mask", S_IRUSR | S_IWUSR,
> + powerpc_debugfs_root, opal_tmask_p,
> + &opal_tmask);
> + return 0;
> +}
> +module_init(opal_trace_init);
> +
cheers
^ permalink raw reply
* Re: [RESEND, V3] powerpc, xmon: Enable HW instruction breakpoint on POWER8
From: Michael Ellerman @ 2014-11-28 0:18 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: linuxppc-dev, mikey
In-Reply-To: <5476DDE3.10705@linux.vnet.ibm.com>
On Thu, 2014-11-27 at 13:46 +0530, Anshuman Khandual wrote:
> On 11/26/2014 01:55 PM, Michael Ellerman wrote:
> > Something like this, untested:
>
> Yeah it is working on LPAR and also on bare metal platform as well. The new patch
> will use some of your suggested code, so can I add your signed-off-by to the patch
> as well ?
Thanks for testing. Yes please add my:
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Benjamin Herrenschmidt @ 2014-11-28 0:56 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417090721-25298-2-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
> This patch create the infrastructure to handle the CR based
> local_* atomic operations. Local atomic operations are fast
> and highly reentrant per CPU counters. Used for percpu
> variable updates. Local atomic operations only guarantee
> variable modification atomicity wrt the CPU which owns the
> data and these needs to be executed in a preemption safe way.
>
> Here is the design of this patch. Since local_* operations
> are only need to be atomic to interrupts (IIUC), patch uses
> one of the Condition Register (CR) fields as a flag variable. When
> entering the local_*, specific bit in the CR5 field is set
> and on exit, bit is cleared. CR bit checking is done in the
> interrupt return path. If CR5[EQ] bit set and if we return
> to kernel, we reset to start of local_* operation.
>
> Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
> instruction pair is used for local_* operations, which are heavy
> on cycle count and they dont support a local variant. So to
> see whether the new implementation helps, used a modified
> version of Rusty's benchmark code on local_t.
>
> https://lkml.org/lkml/2008/12/16/450
>
> Modifications:
> - increated the working set size from 1MB to 8MB,
> - removed cpu_local_inc test.
>
> Test ran
> - on POWER8 1S Scale out System 2.0GHz
> - on OPAL v3 with v3.18-rc4 patch kernel as Host
>
> Here are the values with the patch.
>
> Time in ns per iteration
>
> inc add read add_return
> atomic_long 67 67 18 69
> irqsave/rest 39 39 23 39
> trivalue 39 39 29 49
> local_t 26 26 24 26
>
> Since CR5 is used as a flag, have added CFLAGS to avoid CR5
> for the kernel compilation and CR5 is zeroed at the kernel
> entry.
>
> Tested the patch in a
> - pSeries LPAR,
> - Host with patched/unmodified guest kernel
>
> To check whether userspace see any CR5 corruption, ran a simple
> test which does,
> - set CR5 field,
> - while(1)
> - sleep or gettimeofday
> - chk bit set
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> - I really appreciate feedback on the patchset.
> - Kindly comment if I should try with any other benchmark or
> workload to check the numbers.
> - Also, kindly recommand any know stress test for CR
>
> Makefile | 6 ++
> arch/powerpc/include/asm/exception-64s.h | 21 +++++-
> arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
> arch/powerpc/kernel/exceptions-64s.S | 2 +-
> arch/powerpc/kernel/head_64.S | 8 +++
> 5 files changed, 138 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 00d618b..2e271ad 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -706,6 +706,12 @@ endif
>
> KBUILD_CFLAGS += $(call cc-option, -fno-var-tracking-assignments)
>
> +ifdef CONFIG_PPC64
> +# We need this flag to force compiler not to use CR5, since
> +# local_t type code is based on this.
> +KBUILD_CFLAGS += -ffixed-cr5
> +endif
> +
> ifdef CONFIG_DEBUG_INFO
> ifdef CONFIG_DEBUG_INFO_SPLIT
> KBUILD_CFLAGS += $(call cc-option, -gsplit-dwarf, -g)
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 77f52b2..c42919a 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -306,7 +306,26 @@ do_kvm_##n: \
> std r10,0(r1); /* make stack chain pointer */ \
> std r0,GPR0(r1); /* save r0 in stackframe */ \
> std r10,GPR1(r1); /* save r1 in stackframe */ \
> - beq 4f; /* if from kernel mode */ \
> +BEGIN_FTR_SECTION; \
> + lis r9,4096; /* Create a mask with HV and PR */ \
> + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
> + mr r10,r9; /* to check for Hyp state */ \
> + ori r9,r9,16384; \
> + and r9,r12,r9; \
> + cmpd cr3,r10,r9; \
> + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
> + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
> +FTR_SECTION_ELSE; \
Can't we just unconditionally clear at as long as we do that after we've
saved it ? In that case, it's just a matter for the fixup code to check
the saved version rather than the actual CR..
> + beq 4f; /* if kernel mode branch */ \
> + li r10,0; /* Clear CR5 incase of coming */ \
> + mtcrf 0x04,r10; /* from user. */ \
> + nop; /* This part of code is for */ \
> + nop; /* kernel with MSR[HV]=0, */ \
> + nop; /* MSR[PR]=0, so just chk for */ \
> + nop; /* MSR[PR] */ \
> + nop; \
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \
> +66: beq 4f; /* if from kernel mode */ \
> ACCOUNT_CPU_USER_ENTRY(r9, r10); \
> SAVE_PPR(area, r9, r10); \
> 4: EXCEPTION_PROLOG_COMMON_2(area) \
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 0905c8d..e42bb99 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -68,7 +68,26 @@ system_call_common:
> 2: std r2,GPR2(r1)
> std r3,GPR3(r1)
> mfcr r2
> - std r4,GPR4(r1)
> +BEGIN_FTR_SECTION
> + lis r10,4096
> + rldicr r10,r10,32,31
> + mr r11,r10
> + ori r10,r10,16384
> + and r10,r12,r10
> + cmpd r11,r10
> + beq 67f
> + mtcrf 0x04,r11
> +FTR_SECTION_ELSE
> + beq 67f
> + li r11,0
> + mtcrf 0x04,r11
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +67: std r4,GPR4(r1)
> std r5,GPR5(r1)
> std r6,GPR6(r1)
> std r7,GPR7(r1)
> @@ -224,8 +243,26 @@ syscall_exit:
> BEGIN_FTR_SECTION
> stdcx. r0,0,r1 /* to clear the reservation */
> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> +BEGIN_FTR_SECTION
> + lis r4,4096
> + rldicr r4,r4,32,31
> + mr r6,r4
> + ori r4,r4,16384
> + and r4,r8,r4
> + cmpd cr3,r6,r4
> + beq cr3,65f
> + mtcr r5
> +FTR_SECTION_ELSE
> andi. r6,r8,MSR_PR
> - ld r4,_LINK(r1)
> + beq 65f
> + mtcr r5
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +65: ld r4,_LINK(r1)
>
> beq- 1f
> ACCOUNT_CPU_USER_EXIT(r11, r12)
> @@ -234,7 +271,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> 1: ld r2,GPR2(r1)
> ld r1,GPR1(r1)
> mtlr r4
> +#ifdef CONFIG_PPC64
> + mtcrf 0xFB,r5
> +#else
> mtcr r5
> +#endif
> mtspr SPRN_SRR0,r7
> mtspr SPRN_SRR1,r8
> RFI
> @@ -804,7 +845,66 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> */
> .globl fast_exception_return
> fast_exception_return:
> - ld r3,_MSR(r1)
> +
> + /*
> + * Now that we are about to exit from interrupt, lets check for
> + * cr5 eq bit. If it is set, then we may be in the middle of
> + * local_t update. In this case, we should rewind the NIP
> + * accordingly.
> + */
> + mfcr r3
> + andi. r4,r3,0x200
> + beq 63f
> +
> + /*
> + * Now that the bit is set, lets check for return to User
> + */
> + ld r4,_MSR(r1)
> +BEGIN_FTR_SECTION
> + li r3,4096
> + rldicr r3,r3,32,31
> + mr r5,r3
> + ori r3,r3,16384
> + and r3,r4,r3
> + cmpd r5,r3
> + bne 63f
> +FTR_SECTION_ELSE
> + andi. r3,r4,MSR_PR
> + bne 63f
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +
> + /*
> + * Looks like we are returning to Kernel, so
> + * lets get the NIP and search the ex_table.
> + * Change the NIP based on the return value
> + */
> +lookup_ex_table:
> + ld r3,_NIP(r1)
> + bl search_exception_tables
> + cmpli 0,1,r3,0
> + bne 62f
> +
> + /*
> + * This is a panic case. Reason is that, we
> + * have the CR5 bit set, but we are not in
> + * local_* code and we are returning to Kernel.
> + */
> + ld r3,_NIP(r1)
> + mfcr r4
> + EMIT_BUG_ENTRY lookup_ex_table, __FILE__,__LINE__,BUGFLAG_WARNING
> +
> + /*
> + * Now save the return fixup address as NIP
> + */
> +62: ld r4,8(r3)
> + std r4,_NIP(r1)
> + crclr 22
> +63: ld r3,_MSR(r1)
> ld r4,_CTR(r1)
> ld r0,_LINK(r1)
> mtctr r4
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 72e783e..edb75a9 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
> rldicl r10,r10,48,1; /* clear MSR_EE */ \
> rotldi r10,r10,16; \
> mtspr SPRN_##_H##SRR1,r10; \
> -2: mtcrf 0x80,r9; \
> +2: mtcrf 0x90,r9; \
> ld r9,PACA_EXGEN+EX_R9(r13); \
> ld r10,PACA_EXGEN+EX_R10(r13); \
> ld r11,PACA_EXGEN+EX_R11(r13); \
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index d48125d..02e49b3 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -347,6 +347,14 @@ __mmu_off:
> *
> */
> __start_initialization_multiplatform:
> +
> + /*
> + * Before we do anything, lets clear CR5 field,
> + * so that we will have a clean start at entry
> + */
> + li r11,0
> + mtcrf 0x04,r11
> +
> /* Make sure we are running in 64 bits mode */
> bl enable_64b_mode
>
^ permalink raw reply
* Re: powerpc/powernv: Add debugfs file to grab opalv3 trace data
From: Benjamin Herrenschmidt @ 2014-11-28 1:09 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Rusty Russell
In-Reply-To: <20141128001131.F199414019D@ozlabs.org>
On Fri, 2014-11-28 at 11:11 +1100, Michael Ellerman wrote:
> On Wed, 2014-26-11 at 04:10:04 UTC, Benjamin Herrenschmidt wrote:
> > This adds files in debugfs that can be used to retrieve the
> > OPALv3 firmware "live binary traces" which can then be parsed
> > using a userspace tool.
> >
> > Mostly from Rusty with some updates by myself (BenH)
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Conspicuous review of patch from maintainer follows ...
>
> Meta-comment: given we need a userspace tool to read the trace anyway, could we
> make this a lot simpler by just letting userspace mmap the trace buffers?
>
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index f241acc..315a825 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -1,7 +1,7 @@
> > obj-y += setup.o opal-wrappers.o opal.o opal-async.o
> > obj-y += opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
> > obj-y += rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> > -obj-y += opal-msglog.o opal-hmi.o
> > +obj-y += opal-msglog.o opal-hmi.o opal-trace.o
>
> Should depend on CONFIG_DEBUG_FS at least no?
>
> > diff --git a/arch/powerpc/platforms/powernv/opal-trace-types.h b/arch/powerpc/platforms/powernv/opal-trace-types.h
> > new file mode 100644
> > index 0000000..3bd8ac2
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/opal-trace-types.h
> > @@ -0,0 +1,58 @@
> > +/* API for kernel to read trace buffer. */
>
> Copyright/GPL header ?
>
> /*
> * Copyright 201x, Purple Monkey Dishwasher, IBM Corporation.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> * as published by the Free Software Foundation; either version
> * 2 of the License, or (at your option) any later version.
> */
>
> > +#ifndef __OPAL_TRACE_TYPES_H
> > +#define __OPAL_TRACE_TYPES_H
>
> We usually do __POWERNV_FOO_H for these. But they're all a bit of a mess.
>
> > +#define TRACE_REPEAT 1
> > +#define TRACE_OVERFLOW 2
> > +#define TRACE_OPAL 3
> > +#define TRACE_FSP 4
>
> Linux tracepoints defines a bunch of TRACE_FOO macros, obviously REPEAT is the
> only one that's likely to clash. But still might be worth namespacing.
>
> > +/* One per cpu, plus one for NMIs */
> > +struct tracebuf {
> > + /* Mask to apply to get buffer offset. */
> > + __be64 mask;
> > + /* This where the buffer starts. */
> > + __be64 start;
> > + /* This is where writer has written to. */
> > + __be64 end;
> > + /* This is where the writer wrote to previously. */
> > + __be64 last;
> > + /* This is where the reader is up to. */
> > + __be64 rpos;
> > + /* If the last one we read was a repeat, this shows how many. */
> > + __be32 last_repeat;
>
> So I think start/end/last/rpos/last_repeat are all full virtual addresses right?
No, offsets. I'll let Rusty comment on the rest :)
> > + /* Maximum possible size of a record. */
> > + __be32 max_size;
> > +
> > + char buf[/* TBUF_SZ + max_size */];
>
> We don't have TBUF_SZ.
>
> > +};
> > +
> > +/* Common header for all trace entries. */
> > +struct trace_hdr {
> > + __be64 timestamp;
> > + u8 type;
> > + u8 len_div_8;
> > + __be16 cpu;
> > + u8 unused[4];
> > +};
> > +
>
> The comment below is attached to trace_repeat, but it seems like it should go
> with hdr no ?
>
> > +/* Note: all other entries must be at least as large as this! */
> > +struct trace_repeat {
> > + __be64 timestamp; /* Last repeat happened at this timestamp */
> > + u8 type; /* == TRACE_REPEAT */
> > + u8 len_div_8;
> > + __be16 cpu;
> > + __be16 prev_len;
> > + __be16 num; /* Starts at 1, ie. 1 repeat, or two traces. */
> > + /* Note that the count can be one short, if read races a repeat. */
> > +};
> > +
> > +struct trace_overflow {
> > + __be64 unused64; /* Timestamp is unused */
> > + u8 type; /* == TRACE_OVERFLOW */
> > + u8 len_div_8;
> > + u8 unused[6]; /* ie. hdr.cpu is indeterminate */
> > + __be64 bytes_missed;
> > +};
>
> Some lining up of comments would help readability of those.
>
> > diff --git a/arch/powerpc/platforms/powernv/opal-trace.c b/arch/powerpc/platforms/powernv/opal-trace.c
> > new file mode 100644
> > index 0000000..6529756
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/opal-trace.c
> > @@ -0,0 +1,202 @@
> > +/*
> > + * Copyright (C) 2013 Rusty Russell, IBM Corporation
> > + *
>
> GPL ?
>
> > + * Simple debugfs file firmware_trace to read out OPALv3 trace
> ^
> now called opal-trace
> > + * ringbuffers.
> > + */
>
> <blank>
>
> > +#include <linux/mutex.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <asm/debug.h>
> > +#include <asm/opal.h>
> > +
> > +#include "opal-trace-types.h"
> > +
> > +static DEFINE_MUTEX(tracelock);
> > +static struct tracebuf **opal_tb;
> > +static size_t opal_num_tb;
> > +static __be64 *opal_tmask_p;
>
> _p ? pointer, or phys?
>
> > +
> > +/* Maximum possible size of record (since len is 8 bits). */
> > +union max_trace {
> > + struct trace_hdr hdr;
> > + struct trace_overflow overflow;
> > + struct trace_repeat repeat;
> > + char buf[255 * 8];
> > +};
> > +static union max_trace trace;
>
> I *think* this is only used in opal_trace_read(), so it'd be nice if it was in
> there.
>
> > +static bool trace_empty(const struct tracebuf *tb)
> > +{
> > + const struct trace_repeat *rep;
> > +
> > + if (tb->rpos == tb->end)
> > + return true;
> > +
> > + /*
> > + * If we have a single element only, and it's a repeat buffer
> > + * we've already seen every repeat for (yet which may be
> > + * incremented in future), we're also empty.
> > + */
> > + rep = (void *)tb->buf + (be64_to_cpu(tb->rpos & tb->mask));
> > + if (be64_to_cpu(tb->end) != be64_to_cpu(tb->rpos) + sizeof(*rep))
> > + return false;
> > +
> > + if (rep->type != TRACE_REPEAT)
> > + return false;
> > +
> > + if (be16_to_cpu(rep->num) != be32_to_cpu(tb->last_repeat))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/* You can't read in parallel, so some locking required in caller. */
> > +static bool trace_get(union max_trace *t, struct tracebuf *tb)
> > +{
> > + u64 start, rpos;
> > +
> > + if (trace_empty(tb))
> > + return false;
> > +
> > +again:
> > + /*
> > + * The actual buffer is slightly larger than tbsize, so this
> > + * memcpy is always valid.
>
> We don't seem to have tbsize anymore?
>
> > + */
> > + memcpy(t, tb->buf + be64_to_cpu(tb->rpos & tb->mask),
> > + be32_to_cpu(tb->max_size));
> > +
> > + rmb(); /* read barrier, so we read tb->start after copying record. */
> > +
> > + start = be64_to_cpu(tb->start);
> > + rpos = be64_to_cpu(tb->rpos);
> > +
> > + /* Now, was that overwritten? */
> > + if (rpos < start) {
> > + /* Create overflow record. */
> > + t->overflow.unused64 = 0;
> > + t->overflow.type = TRACE_OVERFLOW;
> > + t->overflow.len_div_8 = sizeof(t->overflow) / 8;
> > + t->overflow.bytes_missed = cpu_to_be64(start - rpos);
> > + tb->rpos = cpu_to_be64(start);
> > + return true;
> > + }
> > +
> > + /* Repeat entries need special handling */
> > + if (t->hdr.type == TRACE_REPEAT) {
> > + u32 num = be16_to_cpu(t->repeat.num);
> > +
> > + /* In case we've read some already... */
> > + t->repeat.num = cpu_to_be16(num - be32_to_cpu(tb->last_repeat));
> > +
> > + /* Record how many repeats we saw this time. */
> > + tb->last_repeat = cpu_to_be32(num);
> > +
> > + /* Don't report an empty repeat buffer. */
> > + if (t->repeat.num == 0) {
> > + /*
> > + * This can't be the last buffer, otherwise
> > + * trace_empty would have returned true.
> > + */
> > + BUG_ON(be64_to_cpu(tb->end) <= rpos + t->hdr.len_div_8 * 8);
>
> Can we just WARN_ON() and bail, seeing as this is for debug. I'd hate to panic
> a customer system by dumping the trace buffer.
>
> > + /* Skip to next entry. */
> > + tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
> > + goto again;
> > + }
> > + } else {
> > + tb->last_repeat = 0;
> > + tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
> > + }
> > +
> > + return true;
> > +}
> > +
> > +/* Horrible polling interface, designed for dumping. */
> > +static ssize_t opal_trace_read(struct file *file, char __user *ubuf,
> > + size_t count, loff_t *ppos)
> > +{
> > + ssize_t err;
> > + unsigned int i;
> > +
> > + err = mutex_lock_interruptible(&tracelock);
> > + if (err)
> > + return err;
> > +
> > + for (i = 0; i < opal_num_tb; i++) {
>
> OK so I understand this now. You have multiple trace buffers, but you don't
> care about maintaining that separation. You just merge all the streams here
> into a single output stream.
>
> > + if (trace_get(&trace, opal_tb[i])) {
> > + size_t len = trace.hdr.len_div_8 * 8;
> > + if (len > count)
> > + len = count;
> > + if (copy_to_user(ubuf, &trace, len) != 0)
> > + err = -EFAULT;
> > + else
> > + err = len;
> > + break;
> > + }
> > + }
> > +
> > + mutex_unlock(&tracelock);
> > + return err;
> > +}
> > +
> > +static const struct file_operations opal_trace_fops = {
> > + .read = opal_trace_read,
> > + .open = simple_open,
> > + .llseek = noop_llseek,
> > +};
> > +
> > +static int opal_tmask_set(void *data, u64 val)
> > +{
> > + *(__be64 *)data = cpu_to_be64(val);
> > + return 0;
> > +}
> > +static int opal_tmask_get(void *data, u64 *val)
> > +{
> > + *val = be64_to_cpup((__be64 *)data);
> > + return 0;
> > +}
> > +DEFINE_SIMPLE_ATTRIBUTE(opal_tmask, opal_tmask_get, opal_tmask_set, "%llx\n");
>
> What is a tmask ?
>
> I assume it's some mask of things we want traced?
>
> > +static int opal_trace_init(void)
> > +{
> > + const __be64 *traces;
> > + int len, i, rc;
> > + u64 tmask_phys;
> > +
> > + if (!opal_node)
> > + return -ENODEV;
> > +
> > + traces = of_get_property(opal_node, "ibm,opal-traces", &len);
> > + if (!traces) {
> > + pr_warning("%s: OPAL node property \"ibm,opal-traces\""
> > + " not found\n", __func__);
> > + return -ENODEV;
> > + }
> > +
> > + opal_num_tb = len / (sizeof(__be64) * 2);
>
> So I won't say the "b" word, but it'd be nice to have at least a comment on
> what the device tree property contains.
>
> > + if (!opal_num_tb) {
> > + pr_warning("%s: OPAL traces property has invalid length %i\n",
> > + __func__, len);
> > + return -EINVAL;
> > + }
> > + opal_tb = kmalloc(sizeof(*opal_tb) * opal_num_tb, GFP_KERNEL);
> > + for (i = 0; i < opal_num_tb; i++)
> > + opal_tb[i] = __va(be64_to_cpu(traces[i*2]));
>
> Just __va() ? ie. it's already in the linear mapping somewhere?
>
> > + debugfs_create_file("opal-trace", S_IRUSR, powerpc_debugfs_root,
> > + NULL, &opal_trace_fops);
> > + rc = of_property_read_u64(opal_node, "ibm,opal-trace-mask",
> > + &tmask_phys);
> > + if (!rc)
> > + opal_tmask_p = __va(tmask_phys);
> > + if (opal_tmask_p)
>
> If tmask_phys was 0, opal_tmask_p is now 0xc00..00, so is that what we want to check?
>
> > + debugfs_create_file("opal-trace-mask", S_IRUSR | S_IWUSR,
> > + powerpc_debugfs_root, opal_tmask_p,
> > + &opal_tmask);
> > + return 0;
> > +}
> > +module_init(opal_trace_init);
> > +
>
> cheers
^ permalink raw reply
* Re: [PATCH REPOST 3/3] powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
From: Benjamin Herrenschmidt @ 2014-11-28 1:49 UTC (permalink / raw)
To: Greg Kurz; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20141127102812.7d1e625b@bahia.local>
On Thu, 2014-11-27 at 10:28 +0100, Greg Kurz wrote:
> On Thu, 27 Nov 2014 10:39:23 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > On Mon, 2014-11-17 at 18:42 +0100, Greg Kurz wrote:
> > > The first argument to vphn_unpack_associativity() is a const long *, but the
> > > parsing code expects __be64 values actually. This is inconsistent. We should
> > > either pass a const __be64 * or change vphn_unpack_associativity() so that
> > > it fixes endianness by itself.
> > >
> > > This patch does the latter, since the caller doesn't need to know about
> > > endianness and this allows to fix significant 64-bit values only. Please
> > > note that the previous code was able to cope with 32-bit fields being split
> > > accross two consecutives 64-bit values. Since PAPR+ doesn't say this cannot
> > > happen, the behaviour was kept. It requires extra checking to know when fixing
> > > is needed though.
> >
> > While I agree with moving the endian fixing down, the patch makes me
> > nervous. Note that I don't fully understand the format of what we are
> > parsing here so I might be wrong but ...
> >
>
> My understanding of PAPR+ is that H_HOME_NODE_ASSOCIATIVITY returns a sequence of
> numbers in registers R4 to R9 (that is 64 * 6 = 384 bits). The numbers are either
> 16-bit long (if high order bit is 1) or 32-bit long. The remaining unused bits are
> set to 1.
Ok, that's the bit I was missing. What we get is thus not a memory array
but a register one, which we "incorrectly" swap when writing to memory
inside plpar_hcall9().
Now, I'm not sure that replacing:
- for (i = 0; i < VPHN_REGISTER_COUNT; i++)
- retbuf[i] = cpu_to_be64(retbuf[i]);
With:
+ if (j % 4 == 0) {
+ fixed.packed[k] = cpu_to_be64(packed[k]);
+ k++;
+ }
Brings any benefit in term of readability. It makes sense to have a
"first pass" that undoes the helper swapping to re-create the original
"byte stream".
In a second pass, we parse that stream, one 16-bytes at a time, and
we could do so with a simple loop of be16_to_cpup(foo++). I wouldn't
bother with the cast to 32-bit etc... if you encounter a 32-bit case,
you just fetch another 16-bit and do value = (old << 16) | new
I think that should lead to something more readable, no ?
> Of course, in a LE guest, plpar_hcall9() stores flipped values to memory.
>
> > >
> > > #define VPHN_FIELD_UNUSED (0xffff)
> > > #define VPHN_FIELD_MSB (0x8000)
> > > #define VPHN_FIELD_MASK (~VPHN_FIELD_MSB)
> > >
> > > - for (i = 1; i < VPHN_ASSOC_BUFSIZE; i++) {
> > > - if (be16_to_cpup(field) == VPHN_FIELD_UNUSED)
> > > + for (i = 1, j = 0, k = 0; i < VPHN_ASSOC_BUFSIZE;) {
> > > + u16 field;
> > > +
> > > + if (j % 4 == 0) {
> > > + fixed.packed[k] = cpu_to_be64(packed[k]);
> > > + k++;
> > > + }
> >
> > So we have essentially a bunch of 16-bit fields ... the above loads and
> > swap a whole 4 of them at once. However that means not only we byteswap
> > them individually, but we also flip the order of the fields. This is
> > ok ?
> >
>
> Yes. FWIW, it is exactly what the current code does.
>
> > > + field = be16_to_cpu(fixed.field[j]);
> > > +
> > > + if (field == VPHN_FIELD_UNUSED)
> > > /* All significant fields processed.
> > > */
> > > break;
> >
> > For example, we might have USED,USED,USED,UNUSED ... after the swap, we
> > now have UNUSED,USED,USED,USED ... and we stop parsing in the above
> > line on the first one. Or am I missing something ?
> >
>
> If we get USED,USED,USED,UNUSED from memory, that means the hypervisor
> has returned UNUSED,USED,USED,USED. My point is that it cannot happen:
> why would the hypervisor care to pack a sequence of useful numbers with
> holes in it ?
> FWIW, I could never observe such a thing in a PowerVM guest... All ones always
> come after the payload.
>
> > > - if (be16_to_cpup(field) & VPHN_FIELD_MSB) {
> > > + if (field & VPHN_FIELD_MSB) {
> > > /* Data is in the lower 15 bits of this field */
> > > - unpacked[i] = cpu_to_be32(
> > > - be16_to_cpup(field) & VPHN_FIELD_MASK);
> > > - field++;
> > > + unpacked[i++] = cpu_to_be32(field & VPHN_FIELD_MASK);
> > > + j++;
> > > } else {
> > > /* Data is in the lower 15 bits of this field
> > > * concatenated with the next 16 bit field
> > > */
> > > - unpacked[i] = *((__be32 *)field);
> > > - field += 2;
> > > + if (unlikely(j % 4 == 3)) {
> > > + /* The next field is to be copied from the next
> > > + * 64-bit input value. We must fix it now.
> > > + */
> > > + fixed.packed[k] = cpu_to_be64(packed[k]);
> > > + k++;
> > > + }
> > > +
> > > + unpacked[i++] = *((__be32 *)&fixed.field[j]);
> > > + j += 2;
> > > }
> > > }
> > >
> > > @@ -1460,11 +1479,8 @@ static long hcall_vphn(unsigned long cpu, __be32 *associativity)
> > > long retbuf[PLPAR_HCALL9_BUFSIZE] = {0};
> > > u64 flags = 1;
> > > int hwcpu = get_hard_smp_processor_id(cpu);
> > > - int i;
> > >
> > > rc = plpar_hcall9(H_HOME_NODE_ASSOCIATIVITY, retbuf, flags, hwcpu);
> > > - for (i = 0; i < VPHN_REGISTER_COUNT; i++)
> > > - retbuf[i] = cpu_to_be64(retbuf[i]);
> > > vphn_unpack_associativity(retbuf, associativity);
> > >
> > > return rc;
> >
> >
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Benjamin Herrenschmidt @ 2014-11-28 1:58 UTC (permalink / raw)
To: Segher Boessenkool
Cc: linuxppc-dev, rusty, Madhavan Srinivasan, paulus, anton
In-Reply-To: <20141127165650.GA28278@gate.crashing.org>
On Thu, 2014-11-27 at 10:56 -0600, Segher Boessenkool wrote:
> On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
> > Here is the design of this patch. Since local_* operations
> > are only need to be atomic to interrupts (IIUC), patch uses
> > one of the Condition Register (CR) fields as a flag variable. When
> > entering the local_*, specific bit in the CR5 field is set
> > and on exit, bit is cleared. CR bit checking is done in the
> > interrupt return path. If CR5[EQ] bit set and if we return
> > to kernel, we reset to start of local_* operation.
>
> Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
> and it likes to use it very much, it might be more convenient to
> use e.g. CR1 (which is allocated almost last, only before CR0).
We use CR1 all over the place in your asm code. Any other suggestion ?
What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
instead ?
> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -306,7 +306,26 @@ do_kvm_##n: \
> > std r10,0(r1); /* make stack chain pointer */ \
> > std r0,GPR0(r1); /* save r0 in stackframe */ \
> > std r10,GPR1(r1); /* save r1 in stackframe */ \
> > - beq 4f; /* if from kernel mode */ \
> > +BEGIN_FTR_SECTION; \
> > + lis r9,4096; /* Create a mask with HV and PR */ \
> > + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
> > + mr r10,r9; /* to check for Hyp state */ \
> > + ori r9,r9,16384; \
> > + and r9,r12,r9; \
> > + cmpd cr3,r10,r9; \
> > + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
> > + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
>
> Wow, such nastiness, only to avoid using dot insns (since you need to keep
> the current CR0 value for the following beq 4f). And CR0 already holds the
> PR bit, so you need only to check the HV bit anyway? Some restructuring
> would make this a lot simpler and clearer.
>
> > + /*
> > + * Now that we are about to exit from interrupt, lets check for
> > + * cr5 eq bit. If it is set, then we may be in the middle of
> > + * local_t update. In this case, we should rewind the NIP
> > + * accordingly.
> > + */
> > + mfcr r3
> > + andi. r4,r3,0x200
> > + beq 63f
>
> This is just bne cr5,63f isn't it?
>
> > index 72e783e..edb75a9 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
> > rldicl r10,r10,48,1; /* clear MSR_EE */ \
> > rotldi r10,r10,16; \
> > mtspr SPRN_##_H##SRR1,r10; \
> > -2: mtcrf 0x80,r9; \
> > +2: mtcrf 0x90,r9; \
> > ld r9,PACA_EXGEN+EX_R9(r13); \
> > ld r10,PACA_EXGEN+EX_R10(r13); \
> > ld r11,PACA_EXGEN+EX_R11(r13); \
>
> What does this do?
>
>
> Segher
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Benjamin Herrenschmidt @ 2014-11-28 2:00 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Anton Blanchard, paulus
In-Reply-To: <20141127205015.GA10073@gate.crashing.org>
On Thu, 2014-11-27 at 14:50 -0600, Segher Boessenkool wrote:
> On Thu, Nov 27, 2014 at 11:41:40AM -0600, Peter Bergner wrote:
> > On Thu, 2014-11-27 at 10:08 -0600, Segher Boessenkool wrote:
> > > On Wed, Nov 26, 2014 at 05:50:27PM -0600, Peter Bergner wrote:
> > > > Nope, you don't get a SIGILL when executing 64-bit instructions in
> > > > 32-bit mode, so it'll happily just execute the instruction, doing
> > > > a full 64-bit compare. I'm guessing that the upper 32-bits of both
> > > > r3 and r4 contain zeros, so we're probably just getting lucky.
> > >
> > > You will get a SIGILL if you run on 32-bit hardware.
> >
> > Ha, I completely forgot about 32-bit hardware. Anyway, I looked
> > at the ISA, and cmpdi and cmpwi are just extended mnemonics for
> > cmpi, with cmpdi setting the L field to 1. Probably on 32-bit
> > hardware, the hardware is just ignoring the L bit being set and
> > doing a cmpwi for us???
>
> Huh. Yes, maybe some implementations do that.
>
> The good news is that those then compute the correct thing ;-)
>
> Can QEMU help catch such bugs more reliably?
That's all moot, that piece of code only exist on 64-bit kernels :-)
So the only risk here is the very remote and unlikely case where the
register might contain 0 in the low 32-bits and some garbage in the top.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Madhavan Srinivasan @ 2014-11-28 2:57 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <20141127165650.GA28278@gate.crashing.org>
On Thursday 27 November 2014 10:26 PM, Segher Boessenkool wrote:
> On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
>> Here is the design of this patch. Since local_* operations
>> are only need to be atomic to interrupts (IIUC), patch uses
>> one of the Condition Register (CR) fields as a flag variable. When
>> entering the local_*, specific bit in the CR5 field is set
>> and on exit, bit is cleared. CR bit checking is done in the
>> interrupt return path. If CR5[EQ] bit set and if we return
>> to kernel, we reset to start of local_* operation.
>
> Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
> and it likes to use it very much, it might be more convenient to
> use e.g. CR1 (which is allocated almost last, only before CR0).
>
No. I did not try it with GCC5.0 But I did force kernel compilation with
fixed-cr5 which should make GCC avoid using CR5. But i will try that today.
>> --- a/arch/powerpc/include/asm/exception-64s.h
>> +++ b/arch/powerpc/include/asm/exception-64s.h
>> @@ -306,7 +306,26 @@ do_kvm_##n: \
>> std r10,0(r1); /* make stack chain pointer */ \
>> std r0,GPR0(r1); /* save r0 in stackframe */ \
>> std r10,GPR1(r1); /* save r1 in stackframe */ \
>> - beq 4f; /* if from kernel mode */ \
>> +BEGIN_FTR_SECTION; \
>> + lis r9,4096; /* Create a mask with HV and PR */ \
>> + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
>> + mr r10,r9; /* to check for Hyp state */ \
>> + ori r9,r9,16384; \
>> + and r9,r12,r9; \
>> + cmpd cr3,r10,r9; \
>> + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
>> + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
>
> Wow, such nastiness, only to avoid using dot insns (since you need to keep
> the current CR0 value for the following beq 4f). And CR0 already holds the
> PR bit, so you need only to check the HV bit anyway? Some restructuring
> would make this a lot simpler and clearer.
Ok I can try that.
>
>> + /*
>> + * Now that we are about to exit from interrupt, lets check for
>> + * cr5 eq bit. If it is set, then we may be in the middle of
>> + * local_t update. In this case, we should rewind the NIP
>> + * accordingly.
>> + */
>> + mfcr r3
>> + andi. r4,r3,0x200
>> + beq 63f
>
> This is just bne cr5,63f isn't it?
>
>> index 72e783e..edb75a9 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
>> rldicl r10,r10,48,1; /* clear MSR_EE */ \
>> rotldi r10,r10,16; \
>> mtspr SPRN_##_H##SRR1,r10; \
>> -2: mtcrf 0x80,r9; \
>> +2: mtcrf 0x90,r9; \
>> ld r9,PACA_EXGEN+EX_R9(r13); \
>> ld r10,PACA_EXGEN+EX_R10(r13); \
>> ld r11,PACA_EXGEN+EX_R11(r13); \
>
> What does this do?
>
Since I use the CR3, I restore it here.
>
> Segher
>
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Madhavan Srinivasan @ 2014-11-28 3:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Segher Boessenkool
Cc: linuxppc-dev, rusty, paulus, anton
In-Reply-To: <1417139935.2852.19.camel@kernel.crashing.org>
On Friday 28 November 2014 07:28 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2014-11-27 at 10:56 -0600, Segher Boessenkool wrote:
>> On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
>>> Here is the design of this patch. Since local_* operations
>>> are only need to be atomic to interrupts (IIUC), patch uses
>>> one of the Condition Register (CR) fields as a flag variable. When
>>> entering the local_*, specific bit in the CR5 field is set
>>> and on exit, bit is cleared. CR bit checking is done in the
>>> interrupt return path. If CR5[EQ] bit set and if we return
>>> to kernel, we reset to start of local_* operation.
>>
>> Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
>> and it likes to use it very much, it might be more convenient to
>> use e.g. CR1 (which is allocated almost last, only before CR0).
>
> We use CR1 all over the place in your asm code. Any other suggestion ?
>
Yes. CR1 is used in many places and so do CR7. And CR0 are alway used
for dot instn. And I guess Vector instructions use CR6.
> What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
> instead ?
>
Will try this today with GCC 5.0.
>>> --- a/arch/powerpc/include/asm/exception-64s.h
>>> +++ b/arch/powerpc/include/asm/exception-64s.h
>>> @@ -306,7 +306,26 @@ do_kvm_##n: \
>>> std r10,0(r1); /* make stack chain pointer */ \
>>> std r0,GPR0(r1); /* save r0 in stackframe */ \
>>> std r10,GPR1(r1); /* save r1 in stackframe */ \
>>> - beq 4f; /* if from kernel mode */ \
>>> +BEGIN_FTR_SECTION; \
>>> + lis r9,4096; /* Create a mask with HV and PR */ \
>>> + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
>>> + mr r10,r9; /* to check for Hyp state */ \
>>> + ori r9,r9,16384; \
>>> + and r9,r12,r9; \
>>> + cmpd cr3,r10,r9; \
>>> + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
>>> + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
>>
>> Wow, such nastiness, only to avoid using dot insns (since you need to keep
>> the current CR0 value for the following beq 4f). And CR0 already holds the
>> PR bit, so you need only to check the HV bit anyway? Some restructuring
>> would make this a lot simpler and clearer.
>>
>>> + /*
>>> + * Now that we are about to exit from interrupt, lets check for
>>> + * cr5 eq bit. If it is set, then we may be in the middle of
>>> + * local_t update. In this case, we should rewind the NIP
>>> + * accordingly.
>>> + */
>>> + mfcr r3
>>> + andi. r4,r3,0x200
>>> + beq 63f
>>
>> This is just bne cr5,63f isn't it?
>>
>>> index 72e783e..edb75a9 100644
>>> --- a/arch/powerpc/kernel/exceptions-64s.S
>>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>>> @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
>>> rldicl r10,r10,48,1; /* clear MSR_EE */ \
>>> rotldi r10,r10,16; \
>>> mtspr SPRN_##_H##SRR1,r10; \
>>> -2: mtcrf 0x80,r9; \
>>> +2: mtcrf 0x90,r9; \
>>> ld r9,PACA_EXGEN+EX_R9(r13); \
>>> ld r10,PACA_EXGEN+EX_R10(r13); \
>>> ld r11,PACA_EXGEN+EX_R11(r13); \
>>
>> What does this do?
>>
>>
>> Segher
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Madhavan Srinivasan @ 2014-11-28 3:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417136200.2852.14.camel@kernel.crashing.org>
On Friday 28 November 2014 06:26 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
>> This patch create the infrastructure to handle the CR based
>> local_* atomic operations. Local atomic operations are fast
>> and highly reentrant per CPU counters. Used for percpu
>> variable updates. Local atomic operations only guarantee
>> variable modification atomicity wrt the CPU which owns the
>> data and these needs to be executed in a preemption safe way.
>>
>> Here is the design of this patch. Since local_* operations
>> are only need to be atomic to interrupts (IIUC), patch uses
>> one of the Condition Register (CR) fields as a flag variable. When
>> entering the local_*, specific bit in the CR5 field is set
>> and on exit, bit is cleared. CR bit checking is done in the
>> interrupt return path. If CR5[EQ] bit set and if we return
>> to kernel, we reset to start of local_* operation.
>>
>> Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
>> instruction pair is used for local_* operations, which are heavy
>> on cycle count and they dont support a local variant. So to
>> see whether the new implementation helps, used a modified
>> version of Rusty's benchmark code on local_t.
>>
>> https://lkml.org/lkml/2008/12/16/450
>>
>> Modifications:
>> - increated the working set size from 1MB to 8MB,
>> - removed cpu_local_inc test.
>>
>> Test ran
>> - on POWER8 1S Scale out System 2.0GHz
>> - on OPAL v3 with v3.18-rc4 patch kernel as Host
>>
>> Here are the values with the patch.
>>
>> Time in ns per iteration
>>
>> inc add read add_return
>> atomic_long 67 67 18 69
>> irqsave/rest 39 39 23 39
>> trivalue 39 39 29 49
>> local_t 26 26 24 26
>>
>> Since CR5 is used as a flag, have added CFLAGS to avoid CR5
>> for the kernel compilation and CR5 is zeroed at the kernel
>> entry.
>>
>> Tested the patch in a
>> - pSeries LPAR,
>> - Host with patched/unmodified guest kernel
>>
>> To check whether userspace see any CR5 corruption, ran a simple
>> test which does,
>> - set CR5 field,
>> - while(1)
>> - sleep or gettimeofday
>> - chk bit set
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> - I really appreciate feedback on the patchset.
>> - Kindly comment if I should try with any other benchmark or
>> workload to check the numbers.
>> - Also, kindly recommand any know stress test for CR
>>
>> Makefile | 6 ++
>> arch/powerpc/include/asm/exception-64s.h | 21 +++++-
>> arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
>> arch/powerpc/kernel/exceptions-64s.S | 2 +-
>> arch/powerpc/kernel/head_64.S | 8 +++
>> 5 files changed, 138 insertions(+), 5 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 00d618b..2e271ad 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -706,6 +706,12 @@ endif
>>
>> KBUILD_CFLAGS += $(call cc-option, -fno-var-tracking-assignments)
>>
>> +ifdef CONFIG_PPC64
>> +# We need this flag to force compiler not to use CR5, since
>> +# local_t type code is based on this.
>> +KBUILD_CFLAGS += -ffixed-cr5
>> +endif
>> +
>> ifdef CONFIG_DEBUG_INFO
>> ifdef CONFIG_DEBUG_INFO_SPLIT
>> KBUILD_CFLAGS += $(call cc-option, -gsplit-dwarf, -g)
>> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
>> index 77f52b2..c42919a 100644
>> --- a/arch/powerpc/include/asm/exception-64s.h
>> +++ b/arch/powerpc/include/asm/exception-64s.h
>> @@ -306,7 +306,26 @@ do_kvm_##n: \
>> std r10,0(r1); /* make stack chain pointer */ \
>> std r0,GPR0(r1); /* save r0 in stackframe */ \
>> std r10,GPR1(r1); /* save r1 in stackframe */ \
>> - beq 4f; /* if from kernel mode */ \
>> +BEGIN_FTR_SECTION; \
>> + lis r9,4096; /* Create a mask with HV and PR */ \
>> + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
>> + mr r10,r9; /* to check for Hyp state */ \
>> + ori r9,r9,16384; \
>> + and r9,r12,r9; \
>> + cmpd cr3,r10,r9; \
>> + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
>> + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
>> +FTR_SECTION_ELSE; \
>
> Can't we just unconditionally clear at as long as we do that after we've
> saved it ? In that case, it's just a matter for the fixup code to check
> the saved version rather than the actual CR..
>
I use CR bit setting in the interrupt return path to enter the fixup
section search. If we unconditionally clear it, we will have to enter
the fixup section for every kernel return nip right?
Regards
Maddy
>> + beq 4f; /* if kernel mode branch */ \
>> + li r10,0; /* Clear CR5 incase of coming */ \
>> + mtcrf 0x04,r10; /* from user. */ \
>> + nop; /* This part of code is for */ \
>> + nop; /* kernel with MSR[HV]=0, */ \
>> + nop; /* MSR[PR]=0, so just chk for */ \
>> + nop; /* MSR[PR] */ \
>> + nop; \
>> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \
>> +66: beq 4f; /* if from kernel mode */ \
>> ACCOUNT_CPU_USER_ENTRY(r9, r10); \
>> SAVE_PPR(area, r9, r10); \
>> 4: EXCEPTION_PROLOG_COMMON_2(area) \
>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 0905c8d..e42bb99 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -68,7 +68,26 @@ system_call_common:
>> 2: std r2,GPR2(r1)
>> std r3,GPR3(r1)
>> mfcr r2
>> - std r4,GPR4(r1)
>> +BEGIN_FTR_SECTION
>> + lis r10,4096
>> + rldicr r10,r10,32,31
>> + mr r11,r10
>> + ori r10,r10,16384
>> + and r10,r12,r10
>> + cmpd r11,r10
>> + beq 67f
>> + mtcrf 0x04,r11
>> +FTR_SECTION_ELSE
>> + beq 67f
>> + li r11,0
>> + mtcrf 0x04,r11
>> + nop
>> + nop
>> + nop
>> + nop
>> + nop
>> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
>> +67: std r4,GPR4(r1)
>> std r5,GPR5(r1)
>> std r6,GPR6(r1)
>> std r7,GPR7(r1)
>> @@ -224,8 +243,26 @@ syscall_exit:
>> BEGIN_FTR_SECTION
>> stdcx. r0,0,r1 /* to clear the reservation */
>> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> +BEGIN_FTR_SECTION
>> + lis r4,4096
>> + rldicr r4,r4,32,31
>> + mr r6,r4
>> + ori r4,r4,16384
>> + and r4,r8,r4
>> + cmpd cr3,r6,r4
>> + beq cr3,65f
>> + mtcr r5
>> +FTR_SECTION_ELSE
>> andi. r6,r8,MSR_PR
>> - ld r4,_LINK(r1)
>> + beq 65f
>> + mtcr r5
>> + nop
>> + nop
>> + nop
>> + nop
>> + nop
>> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
>> +65: ld r4,_LINK(r1)
>>
>> beq- 1f
>> ACCOUNT_CPU_USER_EXIT(r11, r12)
>> @@ -234,7 +271,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> 1: ld r2,GPR2(r1)
>> ld r1,GPR1(r1)
>> mtlr r4
>> +#ifdef CONFIG_PPC64
>> + mtcrf 0xFB,r5
>> +#else
>> mtcr r5
>> +#endif
>> mtspr SPRN_SRR0,r7
>> mtspr SPRN_SRR1,r8
>> RFI
>> @@ -804,7 +845,66 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> */
>> .globl fast_exception_return
>> fast_exception_return:
>> - ld r3,_MSR(r1)
>> +
>> + /*
>> + * Now that we are about to exit from interrupt, lets check for
>> + * cr5 eq bit. If it is set, then we may be in the middle of
>> + * local_t update. In this case, we should rewind the NIP
>> + * accordingly.
>> + */
>> + mfcr r3
>> + andi. r4,r3,0x200
>> + beq 63f
>> +
>> + /*
>> + * Now that the bit is set, lets check for return to User
>> + */
>> + ld r4,_MSR(r1)
>> +BEGIN_FTR_SECTION
>> + li r3,4096
>> + rldicr r3,r3,32,31
>> + mr r5,r3
>> + ori r3,r3,16384
>> + and r3,r4,r3
>> + cmpd r5,r3
>> + bne 63f
>> +FTR_SECTION_ELSE
>> + andi. r3,r4,MSR_PR
>> + bne 63f
>> + nop
>> + nop
>> + nop
>> + nop
>> + nop
>> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
>> +
>> + /*
>> + * Looks like we are returning to Kernel, so
>> + * lets get the NIP and search the ex_table.
>> + * Change the NIP based on the return value
>> + */
>> +lookup_ex_table:
>> + ld r3,_NIP(r1)
>> + bl search_exception_tables
>> + cmpli 0,1,r3,0
>> + bne 62f
>> +
>> + /*
>> + * This is a panic case. Reason is that, we
>> + * have the CR5 bit set, but we are not in
>> + * local_* code and we are returning to Kernel.
>> + */
>> + ld r3,_NIP(r1)
>> + mfcr r4
>> + EMIT_BUG_ENTRY lookup_ex_table, __FILE__,__LINE__,BUGFLAG_WARNING
>> +
>> + /*
>> + * Now save the return fixup address as NIP
>> + */
>> +62: ld r4,8(r3)
>> + std r4,_NIP(r1)
>> + crclr 22
>> +63: ld r3,_MSR(r1)
>> ld r4,_CTR(r1)
>> ld r0,_LINK(r1)
>> mtctr r4
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 72e783e..edb75a9 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
>> rldicl r10,r10,48,1; /* clear MSR_EE */ \
>> rotldi r10,r10,16; \
>> mtspr SPRN_##_H##SRR1,r10; \
>> -2: mtcrf 0x80,r9; \
>> +2: mtcrf 0x90,r9; \
>> ld r9,PACA_EXGEN+EX_R9(r13); \
>> ld r10,PACA_EXGEN+EX_R10(r13); \
>> ld r11,PACA_EXGEN+EX_R11(r13); \
>> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
>> index d48125d..02e49b3 100644
>> --- a/arch/powerpc/kernel/head_64.S
>> +++ b/arch/powerpc/kernel/head_64.S
>> @@ -347,6 +347,14 @@ __mmu_off:
>> *
>> */
>> __start_initialization_multiplatform:
>> +
>> + /*
>> + * Before we do anything, lets clear CR5 field,
>> + * so that we will have a clean start at entry
>> + */
>> + li r11,0
>> + mtcrf 0x04,r11
>> +
>> /* Make sure we are running in 64 bits mode */
>> bl enable_64b_mode
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Benjamin Herrenschmidt @ 2014-11-28 3:21 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <5477E8C1.9030600@linux.vnet.ibm.com>
On Fri, 2014-11-28 at 08:45 +0530, Madhavan Srinivasan wrote:
> > Can't we just unconditionally clear at as long as we do that after we've
> > saved it ? In that case, it's just a matter for the fixup code to check
> > the saved version rather than the actual CR..
> >
> I use CR bit setting in the interrupt return path to enter the fixup
> section search. If we unconditionally clear it, we will have to enter
> the fixup section for every kernel return nip right?
As I said above. Can't we look at the saved version ?
IE.
- On interrupt entry:
* Save CR to CCR(r1)
* clear CR5
- On exit
* Check CCR(r1)'s CR5 field
* restore CR
Cheers,
Ben.
^ permalink raw reply
* [PATCH V4] powerpc, xmon: Enable HW instruction breakpoint on POWER8
From: Anshuman Khandual @ 2014-11-28 4:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey
This patch enables support for hardware instruction breakpoint in
xmon on POWER8 platform with the help of a new register called the
CIABR (Completed Instruction Address Breakpoint Register). With this
patch, a single hardware instruction breakpoint can be added and
cleared during any active xmon debug session. The hardware based
instruction breakpoint mechanism works correctly with the existing
TRAP based instruction breakpoint available on xmon.
There are no powerpc CPU with CPU_FTR_IABR feature any more. This
patch has re-purposed all the existing IABR related code to work
with CIABR register based HW instruction breakpoint.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Changes in V4:
- Moved the external function code block from xmon.h to xmon.c
- Reformatted the in-code documentation as kernel-doc format
- Re-purposed all the IABR related code for CIABR
- Removed all CIABR specific code which existed along with IABR
- Changed the patch commit message
Changes in V3: [Posted at https://patchwork.ozlabs.org/patch/398006/]
- Moved the 'ciabr_used' early init inside 'cmds' function
- Some minor code cleanup
- Added more in-code documentation
- Changed the commit message
Changes in V2: [Posted at http://patchwork.ozlabs.org/patch/373114/]
- Fixed the compilation problem in 32 bit archs
- Selective inclusion of plapr_set_ciabr for required platforms
- Cleaned up the white space issues
arch/powerpc/xmon/xmon.c | 58 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index b988b5a..0ea66e0 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -51,6 +51,12 @@
#include <asm/paca.h>
#endif
+#if defined(CONFIG_PPC_SPLPAR)
+#include <asm/plpar_wrappers.h>
+#else
+static inline long plapr_set_ciabr(unsigned long ciabr) {return 0; };
+#endif
+
#include "nonstdio.h"
#include "dis-asm.h"
@@ -270,6 +276,45 @@ static inline void cinval(void *p)
asm volatile ("dcbi 0,%0; icbi 0,%0" : : "r" (p));
}
+/**
+ * write_ciabr() - write the CIABR SPR
+ * @ciabr: The value to write.
+ *
+ * This function writes a value to the CIARB register either directly
+ * through mtspr instruction if the kernel is in HV privilege mode or
+ * call a hypervisor function to achieve the same in case the kernel
+ * is in supervisor privilege mode.
+ */
+static void write_ciabr(unsigned long ciabr)
+{
+ if (!cpu_has_feature(CPU_FTR_ARCH_207S))
+ return;
+
+ if (cpu_has_feature(CPU_FTR_HVMODE)) {
+ mtspr(SPRN_CIABR, ciabr);
+ return;
+ }
+ plapr_set_ciabr(ciabr);
+}
+
+/**
+ * set_ciabr() - set the CIABR
+ * @addr: The value to set.
+ *
+ * This function sets the correct privilege value into the the HW
+ * breakpoint address before writing it up in the CIABR register.
+ */
+static void set_ciabr(unsigned long addr)
+{
+ addr &= ~CIABR_PRIV;
+
+ if (cpu_has_feature(CPU_FTR_HVMODE))
+ addr |= CIABR_PRIV_HYPER;
+ else
+ addr |= CIABR_PRIV_SUPER;
+ write_ciabr(addr);
+}
+
/*
* Disable surveillance (the service processor watchdog function)
* while we are in xmon.
@@ -764,9 +809,9 @@ static void insert_cpu_bpts(void)
brk.len = 8;
__set_breakpoint(&brk);
}
- if (iabr && cpu_has_feature(CPU_FTR_IABR))
- mtspr(SPRN_IABR, iabr->address
- | (iabr->enabled & (BP_IABR|BP_IABR_TE)));
+
+ if (iabr)
+ set_ciabr(iabr->address);
}
static void remove_bpts(void)
@@ -792,8 +837,7 @@ static void remove_bpts(void)
static void remove_cpu_bpts(void)
{
hw_breakpoint_disable();
- if (cpu_has_feature(CPU_FTR_IABR))
- mtspr(SPRN_IABR, 0);
+ write_ciabr(0);
}
/* Command interpreting routine */
@@ -1127,7 +1171,7 @@ static char *breakpoint_help_string =
"b <addr> [cnt] set breakpoint at given instr addr\n"
"bc clear all breakpoints\n"
"bc <n/addr> clear breakpoint number n or at addr\n"
- "bi <addr> [cnt] set hardware instr breakpoint (POWER3/RS64 only)\n"
+ "bi <addr> [cnt] set hardware instr breakpoint (POWER8 only)\n"
"bd <addr> [cnt] set hardware data breakpoint\n"
"";
@@ -1166,7 +1210,7 @@ bpt_cmds(void)
break;
case 'i': /* bi - hardware instr breakpoint */
- if (!cpu_has_feature(CPU_FTR_IABR)) {
+ if (!cpu_has_feature(CPU_FTR_ARCH_207S)) {
printf("Hardware instruction breakpoint "
"not supported on this cpu\n");
break;
--
1.9.3
^ permalink raw reply related
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-28 7:34 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-arch, Michael S. Tsirkin, Heiko Carstens, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <alpine.DEB.2.11.1411272246110.3961@nanos>
> On Thu, 27 Nov 2014, David Hildenbrand wrote:
> > > OTOH, there is no reason why we need to disable preemption over that
> > > page_fault_disabled() region. There are code pathes which really do
> > > not require to disable preemption for that.
> > >
> > > We have that seperated in preempt-rt for obvious reasons and IIRC
> > > Peter Zijlstra tried to distangle it in mainline some time ago. I
> > > forgot why that never got merged.
> > >
> >
> > Of course, we can completely separate that in our page fault code by doing
> > pagefault_disabled() checks instead of in_atomic() checks (even in add on
> > patches later).
> >
> > > We tie way too much stuff on the preemption count already, which is a
> > > mightmare because we have no clear distinction of protection
> > > scopes.
> >
> > Although it might not be optimal, but keeping a separate counter for
> > pagefault_disable() as part of the preemption counter seems to be the only
> > doable thing right now.
>
> It needs to be seperate, if it should be useful. Otherwise we just
> have a extra accounting in preempt_count() which does exactly the same
> thing as we have now: disabling preemption.
>
> Now you might say, that we could mask out that part when checking
> preempt_count, but that wont work on x86 as x86 has the preempt
> counter as a per cpu variable and not as a per thread one.
Ah right, it's per cpu on x86. So it really belongs to a thread if we want to
demangle preemption and pagefault_disable.
Would work for now, but for x86 not on the long run.
>
> But if you want to distangle pagefault disable from preempt disable
> then you must move it to the thread, because it is a property of the
> thread. preempt count is very much a per cpu counter as you can only
> go through schedule when it becomes 0.
Thinking about it, this makes perfect sense!
>
> Btw, I find the x86 representation way more clear, because it
> documents that preempt count is a per cpu BKL and not a magic thread
> property. And sadly that is how preempt count is used ...
>
> > I am not sure if a completely separated counter is even possible,
> > increasing the size of thread_info.
>
> And adding a ulong to thread_info is going to create exactly which
> problem?
If we're allowed to increase the size of thread_info - absolutely fine with me!
(I am not sure if some archs have special constraints on the size)
Will see what I can come up with.
Thanks!
>
> Thanks,
>
> tglx
>
^ permalink raw reply
* Re: [RFC PATCH 0/2] powerpc: CR based local atomic operation implementation
From: Madhavan Srinivasan @ 2014-11-28 8:27 UTC (permalink / raw)
To: David Laight, mpe@ellerman.id.au
Cc: linuxppc-dev@lists.ozlabs.org, rusty@rustcorp.com.au,
paulus@samba.org, anton@samba.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9FDC8B@AcuExch.aculab.com>
On Thursday 27 November 2014 07:35 PM, David Laight wrote:
> From: Madhavan Srinivasan
>> This patchset create the infrastructure to handle the CR based
>> local_* atomic operations. Local atomic operations are fast
>> and highly reentrant per CPU counters. Used for percpu
>> variable updates. Local atomic operations only guarantee
>> variable modification atomicity wrt the CPU which owns the
>> data and these needs to be executed in a preemption safe way.
>
> These are usually called 'restartable atomic sequences (RAS)'.
>
>> Here is the design of the first patch. Since local_* operations
>> are only need to be atomic to interrupts (IIUC), patch uses
>> one of the Condition Register (CR) fields as a flag variable. When
>> entering the local_*, specific bit in the CR5 field is set
>> and on exit, bit is cleared. CR bit checking is done in the
>> interrupt return path. If CR5[EQ] bit set and if we return
>> to kernel, we reset to start of local_* operation.
>
> I don't claim to be able to read ppc assembler.
> But I can't see the code that clears CR5[EQ] for the duration
> of the ISR.
I use crclr instruction at the end of the code block to clear the bit.
> Without it a nested interrupt will go through unwanted paths.
>
> There are also a lot of 'magic' constants in that assembly code.
>
All these constants are define in asm/ppc-opcode.h
> I also wonder if it is possible to inspect the interrupted
> code to determine the start/end of the RAS block.
> (Easiest if you assume that there is a single 'write' instruction
> as the last entry in the block.)
>
So each local_* function also have code in the __ex_table section. IIUC,
__ex_table contains two address. So if the return address found in the
first column of the _ex_table, use the corresponding address in the
second column to continue from.
> Also, how expensive is it to disable all interrupts?
>
In the patch 1/2, posted the numbers for that too.
> David
>
Regards
Maddy
^ permalink raw reply
* Re: [PATCH REPOST 3/3] powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
From: Greg Kurz @ 2014-11-28 8:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1417139348.2852.17.camel@kernel.crashing.org>
On Fri, 28 Nov 2014 12:49:08 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Thu, 2014-11-27 at 10:28 +0100, Greg Kurz wrote:
> > On Thu, 27 Nov 2014 10:39:23 +1100
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> > > On Mon, 2014-11-17 at 18:42 +0100, Greg Kurz wrote:
> > > > The first argument to vphn_unpack_associativity() is a const long *, but the
> > > > parsing code expects __be64 values actually. This is inconsistent. We should
> > > > either pass a const __be64 * or change vphn_unpack_associativity() so that
> > > > it fixes endianness by itself.
> > > >
> > > > This patch does the latter, since the caller doesn't need to know about
> > > > endianness and this allows to fix significant 64-bit values only. Please
> > > > note that the previous code was able to cope with 32-bit fields being split
> > > > accross two consecutives 64-bit values. Since PAPR+ doesn't say this cannot
> > > > happen, the behaviour was kept. It requires extra checking to know when fixing
> > > > is needed though.
> > >
> > > While I agree with moving the endian fixing down, the patch makes me
> > > nervous. Note that I don't fully understand the format of what we are
> > > parsing here so I might be wrong but ...
> > >
> >
> > My understanding of PAPR+ is that H_HOME_NODE_ASSOCIATIVITY returns a sequence of
> > numbers in registers R4 to R9 (that is 64 * 6 = 384 bits). The numbers are either
> > 16-bit long (if high order bit is 1) or 32-bit long. The remaining unused bits are
> > set to 1.
>
> Ok, that's the bit I was missing. What we get is thus not a memory array
> but a register one, which we "incorrectly" swap when writing to memory
> inside plpar_hcall9().
>
Yes.
> Now, I'm not sure that replacing:
>
> - for (i = 0; i < VPHN_REGISTER_COUNT; i++)
> - retbuf[i] = cpu_to_be64(retbuf[i]);
>
> With:
>
> + if (j % 4 == 0) {
> + fixed.packed[k] = cpu_to_be64(packed[k]);
> + k++;
> + }
>
> Brings any benefit in term of readability. It makes sense to have a
> "first pass" that undoes the helper swapping to re-create the original
> "byte stream".
>
I was myself no quite satisfied by this change and looking for some tips :)
> In a second pass, we parse that stream, one 16-bytes at a time, and
> we could do so with a simple loop of be16_to_cpup(foo++). I wouldn't
> bother with the cast to 32-bit etc... if you encounter a 32-bit case,
> you just fetch another 16-bit and do value = (old << 16) | new
>
> I think that should lead to something more readable, no ?
>
Of course ! This is THE way to go. Thanks Ben ! :)
An while we're here, I have a question about VPHN_ASSOC_BUFSIZE. The
H_HOME_NODE_ASSOCIATIVITY spec says that the stream:
- is at most 64 * 6 = 384 bits long
- may contain 16-bit numbers
- is padded with "all ones"
The stream could theoretically contain up to 384 / 16 = 24 domain numbers.
The current code expects no more than 12 domain numbers... and strangely
seems to correlate the size of the output array to the size of the input
one as noted in the comment:
"6 64-bit registers unpacked into 12 32-bit associativity values"
My understanding is that the resulting array is be32 only because it is
supposed to look like the ibm,associativity property from the DT... and
I could find no clue that this property is limited to 12 values. Have I
missed something ?
> > Of course, in a LE guest, plpar_hcall9() stores flipped values to memory.
> >
> > > >
> > > > #define VPHN_FIELD_UNUSED (0xffff)
> > > > #define VPHN_FIELD_MSB (0x8000)
> > > > #define VPHN_FIELD_MASK (~VPHN_FIELD_MSB)
> > > >
> > > > - for (i = 1; i < VPHN_ASSOC_BUFSIZE; i++) {
> > > > - if (be16_to_cpup(field) == VPHN_FIELD_UNUSED)
> > > > + for (i = 1, j = 0, k = 0; i < VPHN_ASSOC_BUFSIZE;) {
> > > > + u16 field;
> > > > +
> > > > + if (j % 4 == 0) {
> > > > + fixed.packed[k] = cpu_to_be64(packed[k]);
> > > > + k++;
> > > > + }
> > >
> > > So we have essentially a bunch of 16-bit fields ... the above loads and
> > > swap a whole 4 of them at once. However that means not only we byteswap
> > > them individually, but we also flip the order of the fields. This is
> > > ok ?
> > >
> >
> > Yes. FWIW, it is exactly what the current code does.
> >
> > > > + field = be16_to_cpu(fixed.field[j]);
> > > > +
> > > > + if (field == VPHN_FIELD_UNUSED)
> > > > /* All significant fields processed.
> > > > */
> > > > break;
> > >
> > > For example, we might have USED,USED,USED,UNUSED ... after the swap, we
> > > now have UNUSED,USED,USED,USED ... and we stop parsing in the above
> > > line on the first one. Or am I missing something ?
> > >
> >
> > If we get USED,USED,USED,UNUSED from memory, that means the hypervisor
> > has returned UNUSED,USED,USED,USED. My point is that it cannot happen:
> > why would the hypervisor care to pack a sequence of useful numbers with
> > holes in it ?
> > FWIW, I could never observe such a thing in a PowerVM guest... All ones always
> > come after the payload.
> >
> > > > - if (be16_to_cpup(field) & VPHN_FIELD_MSB) {
> > > > + if (field & VPHN_FIELD_MSB) {
> > > > /* Data is in the lower 15 bits of this field */
> > > > - unpacked[i] = cpu_to_be32(
> > > > - be16_to_cpup(field) & VPHN_FIELD_MASK);
> > > > - field++;
> > > > + unpacked[i++] = cpu_to_be32(field & VPHN_FIELD_MASK);
> > > > + j++;
> > > > } else {
> > > > /* Data is in the lower 15 bits of this field
> > > > * concatenated with the next 16 bit field
> > > > */
> > > > - unpacked[i] = *((__be32 *)field);
> > > > - field += 2;
> > > > + if (unlikely(j % 4 == 3)) {
> > > > + /* The next field is to be copied from the next
> > > > + * 64-bit input value. We must fix it now.
> > > > + */
> > > > + fixed.packed[k] = cpu_to_be64(packed[k]);
> > > > + k++;
> > > > + }
> > > > +
> > > > + unpacked[i++] = *((__be32 *)&fixed.field[j]);
> > > > + j += 2;
> > > > }
> > > > }
> > > >
> > > > @@ -1460,11 +1479,8 @@ static long hcall_vphn(unsigned long cpu, __be32 *associativity)
> > > > long retbuf[PLPAR_HCALL9_BUFSIZE] = {0};
> > > > u64 flags = 1;
> > > > int hwcpu = get_hard_smp_processor_id(cpu);
> > > > - int i;
> > > >
> > > > rc = plpar_hcall9(H_HOME_NODE_ASSOCIATIVITY, retbuf, flags, hwcpu);
> > > > - for (i = 0; i < VPHN_REGISTER_COUNT; i++)
> > > > - retbuf[i] = cpu_to_be64(retbuf[i]);
> > > > vphn_unpack_associativity(retbuf, associativity);
> > > >
> > > > return rc;
> > >
> > >
>
>
^ permalink raw reply
* RE: [RFC PATCH 0/2] powerpc: CR based local atomic operation implementation
From: David Laight @ 2014-11-28 10:09 UTC (permalink / raw)
To: 'Madhavan Srinivasan', mpe@ellerman.id.au
Cc: linuxppc-dev@lists.ozlabs.org, rusty@rustcorp.com.au,
paulus@samba.org, anton@samba.org
In-Reply-To: <547831DC.6000703@linux.vnet.ibm.com>
RnJvbTogTWFkaGF2YW4gU3Jpbml2YXNhbg0KPiBPbiBUaHVyc2RheSAyNyBOb3ZlbWJlciAyMDE0
IDA3OjM1IFBNLCBEYXZpZCBMYWlnaHQgd3JvdGU6DQo+ID4gRnJvbTogTWFkaGF2YW4gU3Jpbml2
YXNhbg0KPiA+PiBUaGlzIHBhdGNoc2V0IGNyZWF0ZSB0aGUgaW5mcmFzdHJ1Y3R1cmUgdG8gaGFu
ZGxlIHRoZSBDUiBiYXNlZA0KPiA+PiBsb2NhbF8qIGF0b21pYyBvcGVyYXRpb25zLiBMb2NhbCBh
dG9taWMgb3BlcmF0aW9ucyBhcmUgZmFzdA0KPiA+PiBhbmQgaGlnaGx5IHJlZW50cmFudCBwZXIg
Q1BVIGNvdW50ZXJzLiAgVXNlZCBmb3IgcGVyY3B1DQo+ID4+IHZhcmlhYmxlIHVwZGF0ZXMuIExv
Y2FsIGF0b21pYyBvcGVyYXRpb25zIG9ubHkgZ3VhcmFudGVlDQo+ID4+IHZhcmlhYmxlIG1vZGlm
aWNhdGlvbiBhdG9taWNpdHkgd3J0IHRoZSBDUFUgd2hpY2ggb3ducyB0aGUNCj4gPj4gZGF0YSBh
bmQgdGhlc2UgbmVlZHMgdG8gYmUgZXhlY3V0ZWQgaW4gYSBwcmVlbXB0aW9uIHNhZmUgd2F5Lg0K
PiA+DQo+ID4gVGhlc2UgYXJlIHVzdWFsbHkgY2FsbGVkICdyZXN0YXJ0YWJsZSBhdG9taWMgc2Vx
dWVuY2VzIChSQVMpJy4NCj4gPg0KPiA+PiBIZXJlIGlzIHRoZSBkZXNpZ24gb2YgdGhlIGZpcnN0
IHBhdGNoLiBTaW5jZSBsb2NhbF8qIG9wZXJhdGlvbnMNCj4gPj4gYXJlIG9ubHkgbmVlZCB0byBi
ZSBhdG9taWMgdG8gaW50ZXJydXB0cyAoSUlVQyksIHBhdGNoIHVzZXMNCj4gPj4gb25lIG9mIHRo
ZSBDb25kaXRpb24gUmVnaXN0ZXIgKENSKSBmaWVsZHMgYXMgYSBmbGFnIHZhcmlhYmxlLiBXaGVu
DQo+ID4+IGVudGVyaW5nIHRoZSBsb2NhbF8qLCBzcGVjaWZpYyBiaXQgaW4gdGhlIENSNSBmaWVs
ZCBpcyBzZXQNCj4gPj4gYW5kIG9uIGV4aXQsIGJpdCBpcyBjbGVhcmVkLiBDUiBiaXQgY2hlY2tp
bmcgaXMgZG9uZSBpbiB0aGUNCj4gPj4gaW50ZXJydXB0IHJldHVybiBwYXRoLiBJZiBDUjVbRVFd
IGJpdCBzZXQgYW5kIGlmIHdlIHJldHVybg0KPiA+PiB0byBrZXJuZWwsIHdlIHJlc2V0IHRvIHN0
YXJ0IG9mIGxvY2FsXyogb3BlcmF0aW9uLg0KPiA+DQo+ID4gSSBkb24ndCBjbGFpbSB0byBiZSBh
YmxlIHRvIHJlYWQgcHBjIGFzc2VtYmxlci4NCj4gPiBCdXQgSSBjYW4ndCBzZWUgdGhlIGNvZGUg
dGhhdCBjbGVhcnMgQ1I1W0VRXSBmb3IgdGhlIGR1cmF0aW9uDQo+ID4gb2YgdGhlIElTUi4NCj4g
SSB1c2UgY3JjbHIgaW5zdHJ1Y3Rpb24gYXQgdGhlIGVuZCBvZiB0aGUgY29kZSBibG9jayB0byBj
bGVhciB0aGUgYml0Lg0KPiANCj4gPiBXaXRob3V0IGl0IGEgbmVzdGVkIGludGVycnVwdCB3aWxs
IGdvIHRocm91Z2ggdW53YW50ZWQgcGF0aHMuDQoNClRoYXQgY3JjbHIgbG9va3MgdG8gYmUgaW4g
dGhlIElTUiBleGl0IHBhdGgsIHlvdSBuZWVkIG9uZSBpbiB0aGUNCmlzciBlbnRyeSBwYXRoLg0K
DQo+ID4NCj4gPiBUaGVyZSBhcmUgYWxzbyBhIGxvdCBvZiAnbWFnaWMnIGNvbnN0YW50cyBpbiB0
aGF0IGFzc2VtYmx5IGNvZGUuDQo+ID4NCj4gQWxsIHRoZXNlIGNvbnN0YW50cyBhcmUgZGVmaW5l
IGluIGFzbS9wcGMtb3Bjb2RlLmgNCg0KSSB3YXMgdGhpbmtpbmcgb2YgdGhlIGxpbmVzIGxpa2U6
DQorCW9yaQlyMyxyMywxNjM4NA0KVGhpcyBvbmUgcHJvYmFibHkgZGVzZXJ2ZXMgYSBjb21tZW50
IC0gb3Igc29tZXRoaW5nDQorIjM6IglQUEM0MDVfRVJSNzcoMCwlMikNCg0KPiA+IEkgYWxzbyB3
b25kZXIgaWYgaXQgaXMgcG9zc2libGUgdG8gaW5zcGVjdCB0aGUgaW50ZXJydXB0ZWQNCj4gPiBj
b2RlIHRvIGRldGVybWluZSB0aGUgc3RhcnQvZW5kIG9mIHRoZSBSQVMgYmxvY2suDQo+ID4gKEVh
c2llc3QgaWYgeW91IGFzc3VtZSB0aGF0IHRoZXJlIGlzIGEgc2luZ2xlICd3cml0ZScgaW5zdHJ1
Y3Rpb24NCj4gPiBhcyB0aGUgbGFzdCBlbnRyeSBpbiB0aGUgYmxvY2suKQ0KPiA+DQo+IFNvIGVh
Y2ggbG9jYWxfKiBmdW5jdGlvbiBhbHNvIGhhdmUgY29kZSBpbiB0aGUgX19leF90YWJsZSBzZWN0
aW9uLiBJSVVDLA0KPiBfX2V4X3RhYmxlIGNvbnRhaW5zIHR3byBhZGRyZXNzLiBTbyBpZiB0aGUg
cmV0dXJuIGFkZHJlc3MgZm91bmQgaW4gdGhlDQo+IGZpcnN0IGNvbHVtbiBvZiB0aGUgX2V4X3Rh
YmxlLCB1c2UgdGhlIGNvcnJlc3BvbmRpbmcgYWRkcmVzcyBpbiB0aGUNCj4gc2Vjb25kIGNvbHVt
biB0byBjb250aW51ZSBmcm9tLg0KDQpUaGF0IHJlYWxseSBkb2Vzbid0IHNjYWxlLg0KSSBkb24n
dCBrbm93IGhvdyBtYW55IDEwMDAgYWRkcmVzcyBwYWlycyB5b3UgdGFibGUgd2lsbCBoYXZlIChh
bmQgdGhlDQpvbmVzIGluIGVhY2ggbG9hZGFibGUgbW9kdWxlKSwgYnV0IHRoZSBzZWFyY2ggaXNu
J3QgZ29pbmcgdG8gYmUgY2hlYXAuDQoNCklmIHRoZXNlIHNlcXVlbmNlcyBhcmUgcmVzdGFydGFi
bGUgdGhlbiB0aGV5IGNhbiBvbmx5IGhhdmUgb25lIHdyaXRlDQp0byBtZW1vcnkuDQoNCkdpdmVu
IHlvdXI6DQo+IFRoaXMgcGF0Y2ggcmUtd3JpdGUgdGhlIGN1cnJlbnQgbG9jYWxfKiBmdW5jdGlv
bnMgdG8gQ1I1IGJhc2VkIG9uZS4NCj4gQmFzZSBmbG93IGZvciBlYWNoIGZ1bmN0aW9uIGlzIA0K
PiANCj4gew0KPiAJc2V0IGNyNShlcSkNCj4gCWxvYWQNCj4gCS4uDQo+IAlzdG9yZQ0KPiAJY2xl
YXIgY3I1KGVxKQ0KPiB9DQoNCk9uIElTUiBlbnRyeToNCklmIGFuIElTUiBkZXRlY3RzIGNyNShl
cSkgc2V0IHRoZW4gbG9vayBhdCB0aGUgcmV0dXJuZWQgdG8gaW5zdHJ1Y3Rpb24uDQpJZiBpdCBp
cyAnY2xlYXIgY3I1KGVxKScgZG8gbm90aGluZy4NCk90aGVyd2lzZSByZWFkIGJhY2t3YXJkcyB0
aHJvdWdoIHRoZSBjb2RlIChmb3IgYSBtYXggb2YgKHNheSkgMTYgaW5zdHJ1Y3Rpb25zKQ0Kc2Vh
cmNoaW5nIGZvciB0aGUgJ3NldCBjcjUoZXEpJyBhbmQgY2hhbmdlIHRoZSByZXR1cm4gYWRkcmVz
cyB0byBiZSB0aGF0DQpvZiB0aGUgaW5zdHJ1Y3Rpb24gZm9sbG93aW5nIHRoZSAnc2V0IGNyNShl
cSknLg0KSW4gYWxsIGNhc2VzIGNsZWFyIGNyNShlcSkgZm9yIHRoZSBJU1IgaXRzZWxmIChsZWF2
ZSB0aGUgc2F2ZWQgdmFsdWUgdW5jaGFuZ2VkKS4NCg0KVGhlIHlvdSBkb24ndCBuZWVkIGEgdGFi
bGUgb2YgZmF1bHQgbG9jYXRpb25zLg0KDQoJRGF2aWQNCg0K
^ 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