* 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
* [PATCH RFC 2/2] mm, sched: trigger might_sleep() in might_fault() when pagefaults are disabled
From: David Hildenbrand @ 2014-11-27 17:10 UTC (permalink / raw)
To: linuxppc-dev, linux-arch, linux-kernel
Cc: borntraeger, mst, heiko.carstens, dahi, David.Laight, paulus,
schwidefsky, akpm, tglx
In-Reply-To: <1417108217-42687-1-git-send-email-dahi@linux.vnet.ibm.com>
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);
+}
#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 related
* [PATCH RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 17:10 UTC (permalink / raw)
To: linuxppc-dev, linux-arch, linux-kernel
Cc: borntraeger, mst, heiko.carstens, dahi, David.Laight, paulus,
schwidefsky, akpm, tglx
In-Reply-To: <1416915806-24757-1-git-send-email-dahi@linux.vnet.ibm.com>
Simple prototype to enable might_sleep() checks in might_fault(), avoiding false
positives for scenarios involving explicit pagefault_disable().
So this should work:
spin_lock(&lock); /* also if left away */
pagefault_disable()
rc = copy_to_user(...)
pagefault_enable();
spin_unlock(&lock); /*
And this should report a warning again:
spin_lock(&lock);
rc = copy_to_user(...);
spin_unlock(&lock);
Still missing:
- Split of preempt documentation update + preempt_active define reshuffle
- Debug version to test for over/underflows
- Change documentation of user access methods to reflect the real behavior
- Don't touch the preempt counter, only the pagefault disable counter (future
work)
David Hildenbrand (2):
preempt: track pagefault_disable() calls in the preempt counter
mm, sched: trigger might_sleep() in might_fault() when pagefaults are
disabled
include/linux/kernel.h | 9 +++++++--
include/linux/preempt_mask.h | 24 +++++++++++++++++++-----
include/linux/uaccess.h | 21 ++++++++++++++-------
mm/memory.c | 15 ++++-----------
4 files changed, 44 insertions(+), 25 deletions(-)
--
1.8.5.5
^ permalink raw reply
* [PATCH RFC 1/2] preempt: track pagefault_disable() calls in the preempt counter
From: David Hildenbrand @ 2014-11-27 17:10 UTC (permalink / raw)
To: linuxppc-dev, linux-arch, linux-kernel
Cc: borntraeger, mst, heiko.carstens, dahi, David.Laight, paulus,
schwidefsky, akpm, tglx
In-Reply-To: <1417108217-42687-1-git-send-email-dahi@linux.vnet.ibm.com>
Let's track the levels of pagefault_disable() calls in a separate part of the
preempt counter. Also update the regular preempt counter to keep the existing
pagefault infrastructure working (can be demangeled and cleaned up later).
This change is needed to detect whether we are running in a simple atomic
context or in pagefault_disable() context.
Cleanup the PREEMPT_ACTIVE defines and fix the preempt count documentation on
the way.
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
include/linux/preempt_mask.h | 24 +++++++++++++++++++-----
include/linux/uaccess.h | 21 ++++++++++++++-------
2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/include/linux/preempt_mask.h b/include/linux/preempt_mask.h
index dbeec4d..9d6e7f7 100644
--- a/include/linux/preempt_mask.h
+++ b/include/linux/preempt_mask.h
@@ -4,11 +4,15 @@
#include <linux/preempt.h>
/*
- * We put the hardirq and softirq counter into the preemption
+ * We put the hardirq, softirq and pagefault_disable counter into the preemption
* counter. The bitmask has the following meaning:
*
* - bits 0-7 are the preemption count (max preemption depth: 256)
* - bits 8-15 are the softirq count (max # of softirqs: 256)
+ * - bits 16-19 are the hardirq count (max # of hardirqs: 16)
+ * - bit 20 is the nmi flag
+ * - bit 21 is the preempt_active flag
+ * - bits 22-25 are the pagefault count (max pagefault disable depth: 16)
*
* The hardirq count could in theory be the same as the number of
* interrupts in the system, but we run all interrupt handlers with
@@ -21,16 +25,21 @@
* HARDIRQ_MASK: 0x000f0000
* NMI_MASK: 0x00100000
* PREEMPT_ACTIVE: 0x00200000
+ * PAGEFAULT_MASK: 0x03C00000
*/
#define PREEMPT_BITS 8
#define SOFTIRQ_BITS 8
#define HARDIRQ_BITS 4
#define NMI_BITS 1
+#define PREEMPT_ACTIVE_BITS 1
+#define PAGEFAULT_BITS 4
#define PREEMPT_SHIFT 0
#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define PREEMPT_ACTIVE_SHIFT (NMI_SHIFT + NMI_BITS)
+#define PAGEFAULT_SHIFT (PREEMPT_ACTIVE_SHIFT + PREEMPT_ACTIVE_BITS)
#define __IRQ_MASK(x) ((1UL << (x))-1)
@@ -38,18 +47,17 @@
#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_ACTIVE_SHIFT)
+#define PAGEFAULT_MASK (__IRQ_MASK(PAGEFAULT_BITS) << PAGEFAULT_SHIFT)
#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define PAGEFAULT_OFFSET (1UL << PAGEFAULT_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
-#define PREEMPT_ACTIVE_BITS 1
-#define PREEMPT_ACTIVE_SHIFT (NMI_SHIFT + NMI_BITS)
-#define PREEMPT_ACTIVE (__IRQ_MASK(PREEMPT_ACTIVE_BITS) << PREEMPT_ACTIVE_SHIFT)
-
#define hardirq_count() (preempt_count() & HARDIRQ_MASK)
#define softirq_count() (preempt_count() & SOFTIRQ_MASK)
#define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \
@@ -71,6 +79,12 @@
*/
#define in_nmi() (preempt_count() & NMI_MASK)
+/*
+ * Are we in pagefault_disable context?
+ */
+#define pagefault_disabled() (preempt_count() & PAGEFAULT_MASK)
+
+
#if defined(CONFIG_PREEMPT_COUNT)
# define PREEMPT_CHECK_OFFSET 1
#else
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index ecd3319..a2ba6e6 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -4,18 +4,24 @@
#include <linux/preempt.h>
#include <asm/uaccess.h>
+#define __pagefault_count_inc() preempt_count_add(PAGEFAULT_OFFSET)
+#define __pagefault_count_dec() preempt_count_sub(PAGEFAULT_OFFSET)
+
/*
- * These routines enable/disable the pagefault handler in that
- * it will not take any locks and go straight to the fixup table.
+ * These routines enable/disable the pagefault handler. If disabled, it will
+ * not take any locks and go straight to the fixup table.
+ *
+ * We increase the preempt and the pagefault count, to be able to distinguish
+ * whether we run in simple atomic context or in a real pagefault_disable context.
+ *
+ * For now, after pagefault_disabled() has been called, we run in atomic
+ * context. User access methods will not sleep.
*
- * They have great resemblance to the preempt_disable/enable calls
- * and in fact they are identical; this is because currently there is
- * no other way to make the pagefault handlers do this. So we do
- * disable preemption but we don't necessarily care about that.
*/
static inline void pagefault_disable(void)
{
preempt_count_inc();
+ __pagefault_count_inc();
/*
* make sure to have issued the store before a pagefault
* can hit.
@@ -25,12 +31,13 @@ static inline void pagefault_disable(void)
static inline void pagefault_enable(void)
{
-#ifndef CONFIG_PREEMPT
/*
* make sure to issue those last loads/stores before enabling
* the pagefault handler again.
*/
barrier();
+ __pagefault_count_dec();
+#ifndef CONFIG_PREEMPT
preempt_count_dec();
#else
preempt_enable();
--
1.8.5.5
^ permalink raw reply related
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Segher Boessenkool @ 2014-11-27 16: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, 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).
> --- 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
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 16:49 UTC (permalink / raw)
To: David Laight
Cc: linux-arch@vger.kernel.org, Michael S. Tsirkin, Heiko Carstens,
linux-kernel@vger.kernel.org, mingo@kernel.org,
Christian Borntraeger, paulus@samba.org, schwidefsky@de.ibm.com,
Thomas Gleixner, linuxppc-dev@lists.ozlabs.org,
akpm@linux-foundation.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9FDDD6@AcuExch.aculab.com>
> From: David Hildenbrand [mailto:dahi@linux.vnet.ibm.com]
> > > From: David Hildenbrand
> > > ...
> > > > 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. I am not sure if a completely separated counter is even
> > > > possible, increasing the size of thread_info.
> > >
> > > What about adding (say) 0x10000 for the more restrictive test?
> > >
> > > David
> > >
> >
> > You mean as part of the preempt counter?
> >
> > The current layout (on my branch) is
> >
> > * PREEMPT_MASK: 0x000000ff
> > * SOFTIRQ_MASK: 0x0000ff00
> > * HARDIRQ_MASK: 0x000f0000
> > * NMI_MASK: 0x00100000
> > * PREEMPT_ACTIVE: 0x00200000
> >
> > I would have added
> > * PAGEFAULT_MASK: 0x03C00000
>
> I'm not sure where you'd need to add the bits.
>
> I think the above works because disabling 'HARDIRQ' implicitly
> disables 'SOFTIRQ' and 'PREEMPT' (etc), so if 256+ threads
> disable PREEMPT everything still works.
AFAIK 256+ levels of preempt will break the system :)
Therefore with CONFIG_DEBUG_PREEMPT we verify that we don't have any
over/underflows.
But such bugs can only be found with CONFIG_DEBUG_PREEMPT enabled.
>
> So if disabling pagefaults implies that pre-emption is disabled
> (but SOFTIRQ is still allowed) then you need to insert your bit(s)
> between 0xff00 and 0x00ff.
> OTOH if disabling pre-emption implies that pagefaults are disabled
> then you'd need to use the lsb and change all the above values.
>
> Which makes me think that 'PREEMPT_ACTIVE' isn't right at all.
> Two threads disabling NMIs (or 32 disabling HARDIRQ) won't DTRT.
With threads you mean levels? This is a per thread information.
>
> OTOH I'm only guessing at how this is used.
>
> David
>
>
>
^ permalink raw reply
* RE: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Laight @ 2014-11-27 16:27 UTC (permalink / raw)
To: 'David Hildenbrand'
Cc: linux-arch@vger.kernel.org, Michael S. Tsirkin, Heiko Carstens,
linux-kernel@vger.kernel.org, mingo@kernel.org,
Christian Borntraeger, paulus@samba.org, schwidefsky@de.ibm.com,
Thomas Gleixner, linuxppc-dev@lists.ozlabs.org,
akpm@linux-foundation.org
In-Reply-To: <20141127164555.4bcebfe8@thinkpad-w530>
From: David Hildenbrand [mailto:dahi@linux.vnet.ibm.com]
> > From: David Hildenbrand
> > ...
> > > 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. I am not sure if a completely separated count=
er is even
> > > possible, increasing the size of thread_info.
> >
> > What about adding (say) 0x10000 for the more restrictive test?
> >
> > David
> >
>=20
> You mean as part of the preempt counter?
>=20
> The current layout (on my branch) is
>=20
> * PREEMPT_MASK: 0x000000ff
> * SOFTIRQ_MASK: 0x0000ff00
> * HARDIRQ_MASK: 0x000f0000
> * NMI_MASK: 0x00100000
> * PREEMPT_ACTIVE: 0x00200000
>=20
> I would have added
> * PAGEFAULT_MASK: 0x03C00000
I'm not sure where you'd need to add the bits.
I think the above works because disabling 'HARDIRQ' implicitly
disables 'SOFTIRQ' and 'PREEMPT' (etc), so if 256+ threads
disable PREEMPT everything still works.
So if disabling pagefaults implies that pre-emption is disabled
(but SOFTIRQ is still allowed) then you need to insert your bit(s)
between 0xff00 and 0x00ff.
OTOH if disabling pre-emption implies that pagefaults are disabled
then you'd need to use the lsb and change all the above values.
Which makes me think that 'PREEMPT_ACTIVE' isn't right at all.
Two threads disabling NMIs (or 32 disabling HARDIRQ) won't DTRT.
OTOH I'm only guessing at how this is used.
David
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Segher Boessenkool @ 2014-11-27 16:08 UTC (permalink / raw)
To: Peter Bergner; +Cc: linuxppc-dev, Anton Blanchard, paulus
In-Reply-To: <1417045827.16862.32.camel@otta>
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.
> > Could we catch this by forcing -m32 in the CFLAGS for vdso32 ?
>
> As Segher mentioned, GCC passing -many down to the assembler means
> -m32 won't help. It was due to Anton disabling that gcc "feature",
> that this was caught.
There are extremely many complex failure cases without the -many.
Feel free to work on removing it, I won't :-)
Segher
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 15:45 UTC (permalink / raw)
To: David Laight
Cc: linux-arch@vger.kernel.org, Michael S. Tsirkin, Heiko Carstens,
linux-kernel@vger.kernel.org, mingo@kernel.org,
Christian Borntraeger, paulus@samba.org, schwidefsky@de.ibm.com,
Thomas Gleixner, linuxppc-dev@lists.ozlabs.org,
akpm@linux-foundation.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9FDD6A@AcuExch.aculab.com>
> From: David Hildenbrand
> ...
> > 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. I am not sure if a completely separated counter is even
> > possible, increasing the size of thread_info.
>
> What about adding (say) 0x10000 for the more restrictive test?
>
> David
>
You mean as part of the preempt counter?
The current layout (on my branch) is
* PREEMPT_MASK: 0x000000ff
* SOFTIRQ_MASK: 0x0000ff00
* HARDIRQ_MASK: 0x000f0000
* NMI_MASK: 0x00100000
* PREEMPT_ACTIVE: 0x00200000
I would have added
* PAGEFAULT_MASK: 0x03C00000
So 4 bit == 16 levels (tbd)
By implementing scope checks in the debug case like done for the regular
preempt_count_inc() preempt_count_dec(), we could catch over/underflows.
Thanks,
David
^ permalink raw reply
* RE: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Laight @ 2014-11-27 15:37 UTC (permalink / raw)
To: 'David Hildenbrand', Thomas Gleixner
Cc: linux-arch@vger.kernel.org, Michael S. Tsirkin, Heiko Carstens,
linux-kernel@vger.kernel.org, Christian Borntraeger,
paulus@samba.org, schwidefsky@de.ibm.com,
akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org,
mingo@kernel.org
In-Reply-To: <20141127161905.7c6220ee@thinkpad-w530>
RnJvbTogRGF2aWQgSGlsZGVuYnJhbmQNCi4uLg0KPiBBbHRob3VnaCBpdCBtaWdodCBub3QgYmUg
b3B0aW1hbCwgYnV0IGtlZXBpbmcgYSBzZXBhcmF0ZSBjb3VudGVyIGZvcg0KPiBwYWdlZmF1bHRf
ZGlzYWJsZSgpIGFzIHBhcnQgb2YgdGhlIHByZWVtcHRpb24gY291bnRlciBzZWVtcyB0byBiZSB0
aGUgb25seQ0KPiBkb2FibGUgdGhpbmcgcmlnaHQgbm93LiBJIGFtIG5vdCBzdXJlIGlmIGEgY29t
cGxldGVseSBzZXBhcmF0ZWQgY291bnRlciBpcyBldmVuDQo+IHBvc3NpYmxlLCBpbmNyZWFzaW5n
IHRoZSBzaXplIG9mIHRocmVhZF9pbmZvLg0KDQpXaGF0IGFib3V0IGFkZGluZyAoc2F5KSAweDEw
MDAwIGZvciB0aGUgbW9yZSByZXN0cmljdGl2ZSB0ZXN0Pw0KDQoJRGF2aWQNCg0K
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 15:19 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.1411271602320.3961@nanos>
> 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. I am not sure if a completely separated counter is even
possible, increasing the size of thread_info.
I am working on a prototype right now.
Thanks!
>
> Thanks,
>
> tglx
>
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Thomas Gleixner @ 2014-11-27 15:07 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-arch, Christian Borntraeger, Michael S. Tsirkin,
linux-kernel, David Hildenbrand, paulus, schwidefsky, akpm,
linuxppc-dev, mingo
In-Reply-To: <20141127120441.GB4390@osiris>
On Thu, 27 Nov 2014, Heiko Carstens wrote:
> On Thu, Nov 27, 2014 at 09:03:01AM +0100, David Hildenbrand wrote:
> > > Code like
> > > spin_lock(&lock);
> > > if (copy_to_user(...))
> > > rc = ...
> > > spin_unlock(&lock);
> > > really *should* generate warnings like it did before.
> > >
> > > And *only* code like
> > > spin_lock(&lock);
> >
> > Is only code like this valid or also with the spin_lock() dropped?
> > (e.g. the access in patch1 if I remember correctly)
> >
> > So should page_fault_disable() increment the pagefault counter and the preempt
> > counter or only the first one?
>
> Given that a sequence like
>
> page_fault_disable();
> if (copy_to_user(...))
> rc = ...
> page_fault_enable();
>
> is correct code right now I think page_fault_disable() should increase both.
> No need for surprising semantic changes.
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.
We tie way too much stuff on the preemption count already, which is a
mightmare because we have no clear distinction of protection
scopes.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH v1 1/1] powerpc/85xx: Add support for Emerson/Artesyn MVME2500.
From: Alessio Igor Bogani @ 2014-11-27 14:28 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <1417040495.15957.176.camel@freescale.com>
Scott,
On 26 November 2014 at 23:21, Scott Wood <scottwood@freescale.com> wrote:
> On Wed, 2014-11-26 at 15:17 +0100, Alessio Igor Bogani wrote:
>> + board_soc: soc: soc@ffe00000 {
>
> There's no need for two labels on the same node.
I'll remove board_soc label.
[...]
>> + eeprom-vpd@54 {
>> + compatible = "atmel,24c64";
>> + reg = <0x54>;
>> + };
>
> eeprom-vpd?
>
> Node name isn't the right place to put the intended usage of the
> contents of the EEPROM.
I'll rename eeprom-vpd to eeprom.
[...]
>> + spd@50 {
>> + compatible = "atmel,24c02";
>> + reg = <0x50>;
>> + };
>
> Likewise, I suspect this is also an eeprom.
I'll rename spd to eeprom.
[...]
>> + partition@u-boot {
>> + label = "u-boot";
>> + reg = <0x00000000 0x000A0000>;
>> + read-only;
>> + };
>> + partition@dtb {
>> + label = "dtb";
>> + reg = <0x000A0000 0x00020000>;
>> + };
>
> Unfortunately you seem to have copied a bad example here... After the @
> should be a number that matches reg.
>
> Better yet, don't put partition information in the dts at all -- it's
> not hardware description. Use the mtdparts command line.
I'll remove partition scheme.
>> + lbc: localbus@ffe05000 {
>> + reg = <0 0xffe05000 0 0x1000>;
>> +
>
> It's not possible to program the LBC with a window of only 0x1000 bytes.
All similar boards seem to have the same value there. AFAIK 0x1000 is
a offset so it stands for 4KB.
>> +
>> + serial2: serial@1,0 {
>> + #cell-index = <2>;
>> + device_type = "serial";
>> + compatible = "ns16550";
>> + reg = <0x1 0x0 0x100>;
>> + clock-frequency = <1843200>;
>> + interrupts = <11 2 0 0>;
>> + };
>
> Why do you need cell-index, what connection do these values have to
> actual hardware (e.g. values written to a register, rather than numbers
> in a manual), and why did the name change to #cell-index?
I have used fsl/pq3-duart-0.dtsi as template and #cell-index are used there.
I'll remove #cell-index. The name should be already correct.
>> + interrupts = <9 1 0 0 >;
>
> Whitespace
I'll remove it.
>> +/include/ "mvme2500.dtsi"
>
> Are you going to have more than one .dts using this .dtsi? If not, why
> separate this part?
The pq3-gpio-0.dtsi defines an gpio controller in this way:
gpio-controller@f000 {
reg = <0xf000 0x100>;
[...]
But MVME2500 board requires a slightly different definition:
reg = <0xfc00 0x100>;
Override gpio-controller reg definition included by
fsl/p2020si-post.dtsi (which includes the above mentioned
fsl/pq3-gpio-0.dtsi) using mvme2500.dtsi is the only solution I have
found so far.
Can you suggest me a better approach, please?
>> diff --git a/arch/powerpc/boot/dts/mvme2500.dtsi b/arch/powerpc/boot/dts/mvme2500.dtsi
>> new file mode 100644
>> index 0000000..6966f13
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/mvme2500.dtsi
> [snip]
>> +/include/ "fsl/pq3-mpic-message-B.dtsi"
>> +};
>
> Why is this being included from a board file rather than from the SoC
> file?
My fault. I'll move that include into mvme2500.dts.
>> diff --git a/arch/powerpc/configs/85xx/mvme2500_defconfig b/arch/powerpc/configs/85xx/mvme2500_defconfig
>> new file mode 100644
>> index 0000000..06fe629
>> --- /dev/null
>> +++ b/arch/powerpc/configs/85xx/mvme2500_defconfig
>
> Why does this board need its own defconfig?
>
> If it's just for the address space stuff, maybe it could be a more
> general mpc85xx_2g_1g_1g_defconfig. xes_mpc85xx_defconfig uses the same
> layout (though it's SMP). Maybe other boards could share it in the
> future, or users of existing boards might prefer it...
Sorry for ignorance but what are *_defconfigs supposed to provide?
A barely bootable system (in that case I can pick the config of a
similar board) or a system with all drivers for devices exposed by its
device tree?
> Better still would be if we could have address map tweaks be kconfig
> fragments that get mixed in by the user, with merge_config.sh.
Personally I would prefer see something more simple like this:
%_defconfig: scripts/kconfig/conf
# Grab the platform generic config file (for a SoC family)
$(Q)$< --defconfig=arch/$(SRCARCH)/configs/mpc$(shell dirname
$@)_defconfig Kconfig
# So merge board specific configuration options
$(Q)$(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m
-O $(objtree) $(objtree)/.config arch/$(SRCARCH)/configs/$@
# Expand config
$(Q)yes "" | $(MAKE) -f $(srctree)/Makefile oldconfig
>> +CONFIG_MATH_EMULATION=y
>> +CONFIG_MATH_EMULATION_HW_UNIMPLEMENTED=y
>
> CONFIG_MATH_EMULATION_HW_UNIMPLEMENTED is not appropriate for e500v2
> which does not implement any part of the classic PPC FPU. You want
> either full emulation or no emulation at all.
I'll change configuration for use full emulation.
>> +CONFIG_ADVANCED_OPTIONS=y
>> +CONFIG_LOWMEM_SIZE_BOOL=y
>> +CONFIG_LOWMEM_SIZE=0x40000000
>> +CONFIG_PAGE_OFFSET_BOOL=y
>> +CONFIG_PAGE_OFFSET=0x80000000
>> +CONFIG_KERNEL_START_BOOL=y
>> +CONFIG_TASK_SIZE_BOOL=y
>> +CONFIG_TASK_SIZE=0x80000000
>
> I gues the point here is to avoid using highmem just for the last 256
> MiB?
Yes. Can you suggest me a better solution, please?
>> +CONFIG_STAGING=y
>
> What do you need from staging?
CONFIG_VME_USER. It is a staging driver although it isn't appear in
staging menu.
>> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
>> index f22635a..b92674a 100644
>> --- a/arch/powerpc/platforms/85xx/Kconfig
>> +++ b/arch/powerpc/platforms/85xx/Kconfig
>> @@ -241,6 +241,14 @@ config SGY_CTS1000
>> help
>> Enable this to support functionality in Servergy's CTS-1000 systems.
>>
>> +config MVME2500
>> + bool "Artesyn MVME2500"
>> + select DEFAULT_UIMAGE
>> + select SWIOTLB
>
> Why do you need SWIOTLB with only 1 GiB RAM?
I'll remove SWIOTLB usages.
>> +#include <linux/stddef.h>
>> +#include <linux/kernel.h>
>> +#include <linux/pci.h>
>> +#include <linux/kdev_t.h>
>> +#include <linux/delay.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_platform.h>
>> +
>> +#include <asm/time.h>
>> +#include <asm/machdep.h>
>> +#include <asm/pci-bridge.h>
>> +#include <mm/mmu_decl.h>
>> +#include <asm/prom.h>
>> +#include <asm/udbg.h>
>> +#include <asm/mpic.h>
>> +#include <asm/swiotlb.h>
>> +#include <asm/nvram.h>
>> +
>> +#include <sysdev/fsl_soc.h>
>> +#include <sysdev/fsl_pci.h>
>> +
>> +#include "mpc85xx.h"
>
> I don't think you need all of these.
Il'' avoid include useless headers.
>
>> +#if defined(CONFIG_MMIO_NVRAM)
>> + mmio_nvram_init();
>> +#endif
>
> You select it in kconfig, so why do you need the ifdef?
>
>> + printk(KERN_INFO "MVME2500 board from Artesyn\n");
>
> pr_info()
I''l change the code for using pr_info() instead of printk().
>> + return of_flat_dt_is_compatible(root, "Artesyn,MVME2500");
>
> The compatible in the dts uses "artesyn", not "Artesyn". Don't rely on
> the fact that Linux (on some arches) uses case-insensitive comparisons
> to deal with broken old firmware. Nothing in ePAPR says that compatible
> should be case-insensitive.
I'll rename Artesyn to artesyn.
Thank you very much.
Ciao,
Alessio
^ permalink raw reply
* RE: [RFC PATCH 0/2] powerpc: CR based local atomic operation implementation
From: David Laight @ 2014-11-27 14:05 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: <1417090721-25298-1-git-send-email-maddy@linux.vnet.ibm.com>
RnJvbTogTWFkaGF2YW4gU3Jpbml2YXNhbg0KPiBUaGlzIHBhdGNoc2V0IGNyZWF0ZSB0aGUgaW5m
cmFzdHJ1Y3R1cmUgdG8gaGFuZGxlIHRoZSBDUiBiYXNlZA0KPiBsb2NhbF8qIGF0b21pYyBvcGVy
YXRpb25zLiBMb2NhbCBhdG9taWMgb3BlcmF0aW9ucyBhcmUgZmFzdA0KPiBhbmQgaGlnaGx5IHJl
ZW50cmFudCBwZXIgQ1BVIGNvdW50ZXJzLiAgVXNlZCBmb3IgcGVyY3B1DQo+IHZhcmlhYmxlIHVw
ZGF0ZXMuIExvY2FsIGF0b21pYyBvcGVyYXRpb25zIG9ubHkgZ3VhcmFudGVlDQo+IHZhcmlhYmxl
IG1vZGlmaWNhdGlvbiBhdG9taWNpdHkgd3J0IHRoZSBDUFUgd2hpY2ggb3ducyB0aGUNCj4gZGF0
YSBhbmQgdGhlc2UgbmVlZHMgdG8gYmUgZXhlY3V0ZWQgaW4gYSBwcmVlbXB0aW9uIHNhZmUgd2F5
Lg0KDQpUaGVzZSBhcmUgdXN1YWxseSBjYWxsZWQgJ3Jlc3RhcnRhYmxlIGF0b21pYyBzZXF1ZW5j
ZXMgKFJBUyknLg0KDQo+IEhlcmUgaXMgdGhlIGRlc2lnbiBvZiB0aGUgZmlyc3QgcGF0Y2guIFNp
bmNlIGxvY2FsXyogb3BlcmF0aW9ucw0KPiBhcmUgb25seSBuZWVkIHRvIGJlIGF0b21pYyB0byBp
bnRlcnJ1cHRzIChJSVVDKSwgcGF0Y2ggdXNlcw0KPiBvbmUgb2YgdGhlIENvbmRpdGlvbiBSZWdp
c3RlciAoQ1IpIGZpZWxkcyBhcyBhIGZsYWcgdmFyaWFibGUuIFdoZW4NCj4gZW50ZXJpbmcgdGhl
IGxvY2FsXyosIHNwZWNpZmljIGJpdCBpbiB0aGUgQ1I1IGZpZWxkIGlzIHNldA0KPiBhbmQgb24g
ZXhpdCwgYml0IGlzIGNsZWFyZWQuIENSIGJpdCBjaGVja2luZyBpcyBkb25lIGluIHRoZQ0KPiBp
bnRlcnJ1cHQgcmV0dXJuIHBhdGguIElmIENSNVtFUV0gYml0IHNldCBhbmQgaWYgd2UgcmV0dXJu
DQo+IHRvIGtlcm5lbCwgd2UgcmVzZXQgdG8gc3RhcnQgb2YgbG9jYWxfKiBvcGVyYXRpb24uDQoN
CkkgZG9uJ3QgY2xhaW0gdG8gYmUgYWJsZSB0byByZWFkIHBwYyBhc3NlbWJsZXIuDQpCdXQgSSBj
YW4ndCBzZWUgdGhlIGNvZGUgdGhhdCBjbGVhcnMgQ1I1W0VRXSBmb3IgdGhlIGR1cmF0aW9uDQpv
ZiB0aGUgSVNSLg0KV2l0aG91dCBpdCBhIG5lc3RlZCBpbnRlcnJ1cHQgd2lsbCBnbyB0aHJvdWdo
IHVud2FudGVkIHBhdGhzLg0KDQpUaGVyZSBhcmUgYWxzbyBhIGxvdCBvZiAnbWFnaWMnIGNvbnN0
YW50cyBpbiB0aGF0IGFzc2VtYmx5IGNvZGUuDQoNCkkgYWxzbyB3b25kZXIgaWYgaXQgaXMgcG9z
c2libGUgdG8gaW5zcGVjdCB0aGUgaW50ZXJydXB0ZWQNCmNvZGUgdG8gZGV0ZXJtaW5lIHRoZSBz
dGFydC9lbmQgb2YgdGhlIFJBUyBibG9jay4NCihFYXNpZXN0IGlmIHlvdSBhc3N1bWUgdGhhdCB0
aGVyZSBpcyBhIHNpbmdsZSAnd3JpdGUnIGluc3RydWN0aW9uDQphcyB0aGUgbGFzdCBlbnRyeSBp
biB0aGUgYmxvY2suKQ0KDQpBbHNvLCBob3cgZXhwZW5zaXZlIGlzIGl0IHRvIGRpc2FibGUgYWxs
IGludGVycnVwdHM/DQoNCglEYXZpZA0KDQo=
^ permalink raw reply
* [RFC PATCH 2/2]powerpc: rewrite local_* to use CR5 flag
From: Madhavan Srinivasan @ 2014-11-27 12:18 UTC (permalink / raw)
To: mpe; +Cc: Madhavan Srinivasan, rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417090721-25298-1-git-send-email-maddy@linux.vnet.ibm.com>
This patch re-write the current local_* functions to CR5 based one.
Base flow for each function is
{
set cr5(eq)
load
..
store
clear cr5(eq)
}
Above set of instructions are followed by a fixup section which points
to the entry of the function incase of interrupt in the flow. If the
interrupt happens to be after the store, we just continue to last
instruction in that block.
Currently only asm/local.h has been rewrite, and local64 is TODO.
Also the entire change is only for PPC64.
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++++++++++
1 file changed, 306 insertions(+)
diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h
index b8da913..a26e5d3 100644
--- a/arch/powerpc/include/asm/local.h
+++ b/arch/powerpc/include/asm/local.h
@@ -11,6 +11,310 @@ typedef struct
#define LOCAL_INIT(i) { ATOMIC_LONG_INIT(i) }
+#ifdef CONFIG_PPC64
+
+static __inline__ long local_read(local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3: crclr 22\n"
+"4:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,3b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (&(l->a.counter)));
+
+ return t;
+}
+
+static __inline__ void local_set(local_t *l, long i)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3:" PPC405_ERR77(0,%2)
+"4:" PPC_STL" %0,0(%2)\n"
+"5: crclr 22\n"
+"6:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,5b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (&(i)), "r" (&(l->a.counter)));
+}
+
+static __inline__ void local_add(long i, local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%2)\n"
+"3: add %0,%1,%0\n"
+"4:" PPC405_ERR77(0,%2)
+"5:" PPC_STL" %0,0(%2)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (i), "r" (&(l->a.counter)));
+}
+
+static __inline__ void local_sub(long i, local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%2)\n"
+"3: subf %0,%1,%0\n"
+"4:" PPC405_ERR77(0,%2)
+"5:" PPC_STL" %0,0(%2)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (i), "r" (&(l->a.counter)));
+}
+
+static __inline__ long local_add_return(long a, local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%2)\n"
+"3: add %0,%1,%0\n"
+"4:" PPC405_ERR77(0,%2)
+"5:" PPC_STL "%0,0(%2)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (a), "r" (&(l->a.counter))
+ : "cc", "memory");
+
+ return t;
+}
+
+
+#define local_add_negative(a, l) (local_add_return((a), (l)) < 0)
+
+static __inline__ long local_sub_return(long a, local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%2)\n"
+"3: subf %0,%1,%0\n"
+"4:" PPC405_ERR77(0,%2)
+"5:" PPC_STL "%0,0(%2)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (a), "r" (&(l->a.counter))
+ : "cc", "memory");
+
+ return t;
+}
+
+static __inline__ long local_inc_return(local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3: addic %0,%0,1\n"
+"4:" PPC405_ERR77(0,%1)
+"5:" PPC_STL "%0,0(%1)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous"
+ : "=&r" (t)
+ : "r" (&(l->a.counter))
+ : "cc", "xer", "memory");
+
+ return t;
+}
+
+/*
+ * local_inc_and_test - increment and test
+ * @l: pointer of type local_t
+ *
+ * Atomically increments @l by 1
+ * and returns true if the result is zero, or false for all
+ * other cases.
+ */
+#define local_inc_and_test(l) (local_inc_return(l) == 0)
+
+static __inline__ long local_dec_return(local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3: addic %0,%0,-1\n"
+"4:" PPC405_ERR77(0,%1)
+"5:" PPC_STL "%0,0(%1)\n"
+"6: crclr 22\n"
+"7:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,6b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (&(l->a.counter))
+ : "cc", "xer", "memory");
+
+ return t;
+}
+
+#define local_inc(l) local_inc_return(l)
+#define local_dec(l) local_dec_return(l)
+
+#define local_cmpxchg(l, o, n) \
+ (cmpxchg_local(&((l)->a.counter), (o), (n)))
+#define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n)))
+
+/**
+ * local_add_unless - add unless the number is a given value
+ * @l: pointer of type local_t
+ * @a: the amount to add to v...
+ * @u: ...unless v is equal to u.
+ *
+ * Atomically adds @a to @l, so long as it was not @u.
+ * Returns non-zero if @l was not @u, and zero otherwise.
+ */
+static __inline__ int local_add_unless(local_t *l, long a, long u)
+{
+ long t;
+
+ __asm__ __volatile__ (
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3: cmpw 0,%0,%3 \n"
+"4: beq- 9f \n"
+"5: add %0,%2,%0 \n"
+"6:" PPC405_ERR77(0,%1)
+"7:" PPC_STL" %0,0(%1) \n"
+"8: subf %0,%2,%0 \n"
+"9: crclr 22\n"
+"10:\n"
+" .section __ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,1b\n"
+ PPC_LONG "7b,1b\n"
+ PPC_LONG "8b,8b\n"
+ PPC_LONG "9b,9b\n"
+" .previous\n"
+ : "=&r" (t)
+ : "r" (&(l->a.counter)), "r" (a), "r" (u)
+ : "cc", "memory");
+
+ return t != u;
+}
+
+#define local_inc_not_zero(l) local_add_unless((l), 1, 0)
+
+#define local_sub_and_test(a, l) (local_sub_return((a), (l)) == 0)
+#define local_dec_and_test(l) (local_dec_return((l)) == 0)
+
+/*
+ * Atomically test *l and decrement if it is greater than 0.
+ * The function returns the old value of *l minus 1.
+ */
+static __inline__ long local_dec_if_positive(local_t *l)
+{
+ long t;
+
+ __asm__ __volatile__(
+"1: crset 22\n"
+"2:" PPC_LL" %0,0(%1)\n"
+"3: cmpwi %0,1\n"
+"4: addi %0,%0,-1\n"
+"5: blt- 8f\n"
+"6:" PPC405_ERR77(0,%1)
+"7:" PPC_STL "%0,0(%1)\n"
+"8: crclr 22\n"
+"9:\n"
+" .section__ex_table,\"a\"\n"
+ PPC_LONG_ALIGN "\n"
+ PPC_LONG "2b,1b\n"
+ PPC_LONG "3b,1b\n"
+ PPC_LONG "4b,1b\n"
+ PPC_LONG "5b,1b\n"
+ PPC_LONG "6b,1b\n"
+ PPC_LONG "7b,1b\n"
+ PPC_LONG "8b,8b\n"
+" .previous\n"
+ : "=&b" (t)
+ : "r" (&(l->a.counter))
+ : "cc", "memory");
+
+ return t;
+}
+
+#else
+
#define local_read(l) atomic_long_read(&(l)->a)
#define local_set(l,i) atomic_long_set(&(l)->a, (i))
@@ -162,6 +466,8 @@ static __inline__ long local_dec_if_positive(local_t *l)
return t;
}
+#endif
+
/* Use these for per-cpu local_t variables: on some archs they are
* much more efficient than these naive implementations. Note they take
* a variable, not an address.
--
1.9.1
^ permalink raw reply related
* [RFC PATCH 0/2] powerpc: CR based local atomic operation implementation
From: Madhavan Srinivasan @ 2014-11-27 12:18 UTC (permalink / raw)
To: mpe; +Cc: Madhavan Srinivasan, rusty, paulus, anton, linuxppc-dev
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.
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.
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 varient. So to
see whether the new implementation helps, Used a modified
version of Rusty's benchmark code on local_t. Have the
performance numbers in the patch commit message.
Second patch has the rewrite of the local_* functions to use
CR5 based logic. Changes are mostly in asm/local.h and only for
CONFIG_PPC64
Madhavan Srinivasan (2):
powerpc: foundation code to handle CR5 for local_t
powerpc: rewrite local_* to use CR5 flag
Makefile | 6 +
arch/powerpc/include/asm/exception-64s.h | 21 ++-
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/entry_64.S | 106 ++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +
6 files changed, 444 insertions(+), 5 deletions(-)
--
1.9.1
^ permalink raw reply
* [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Madhavan Srinivasan @ 2014-11-27 12:18 UTC (permalink / raw)
To: mpe; +Cc: Madhavan Srinivasan, rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417090721-25298-1-git-send-email-maddy@linux.vnet.ibm.com>
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; \
+ 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
--
1.9.1
^ permalink raw reply related
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 12:08 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-arch, Michael S. Tsirkin, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141127120441.GB4390@osiris>
> On Thu, Nov 27, 2014 at 09:03:01AM +0100, David Hildenbrand wrote:
> > > Code like
> > > spin_lock(&lock);
> > > if (copy_to_user(...))
> > > rc = ...
> > > spin_unlock(&lock);
> > > really *should* generate warnings like it did before.
> > >
> > > And *only* code like
> > > spin_lock(&lock);
> >
> > Is only code like this valid or also with the spin_lock() dropped?
> > (e.g. the access in patch1 if I remember correctly)
> >
> > So should page_fault_disable() increment the pagefault counter and the preempt
> > counter or only the first one?
>
> Given that a sequence like
>
> page_fault_disable();
> if (copy_to_user(...))
> rc = ...
> page_fault_enable();
>
> is correct code right now I think page_fault_disable() should increase both.
> No need for surprising semantic changes.
>
> > So we would have pagefault code rely on:
> >
> > in_disabled_pagefault() ( pagefault_disabled() ... whatever ) instead of
> > in_atomic().
>
> No, let's be more defensive: the page fault handler should do nothing if
> in_atomic() just like now. But it could have a quick check and emit a one
> time warning if page faults aren't disabled in addition.
> That might help debugging but keeps the system more likely alive.
Sounds sane if we increase both counters!
>
> might_fault() however should call might_sleep() if page faults aren't
> disabled, but that's what you proposed anyway I think.
Jap, sounds good to me. Will see if I can come up with something.
Thanks!
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Heiko Carstens @ 2014-11-27 12:04 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, Michael S. Tsirkin, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141127090301.3ddc3077@thinkpad-w530>
On Thu, Nov 27, 2014 at 09:03:01AM +0100, David Hildenbrand wrote:
> > Code like
> > spin_lock(&lock);
> > if (copy_to_user(...))
> > rc = ...
> > spin_unlock(&lock);
> > really *should* generate warnings like it did before.
> >
> > And *only* code like
> > spin_lock(&lock);
>
> Is only code like this valid or also with the spin_lock() dropped?
> (e.g. the access in patch1 if I remember correctly)
>
> So should page_fault_disable() increment the pagefault counter and the preempt
> counter or only the first one?
Given that a sequence like
page_fault_disable();
if (copy_to_user(...))
rc = ...
page_fault_enable();
is correct code right now I think page_fault_disable() should increase both.
No need for surprising semantic changes.
> So we would have pagefault code rely on:
>
> in_disabled_pagefault() ( pagefault_disabled() ... whatever ) instead of
> in_atomic().
No, let's be more defensive: the page fault handler should do nothing if
in_atomic() just like now. But it could have a quick check and emit a one
time warning if page faults aren't disabled in addition.
That might help debugging but keeps the system more likely alive.
might_fault() however should call might_sleep() if page faults aren't
disabled, but that's what you proposed anyway I think.
^ permalink raw reply
* Re: [PATCH REPOST 3/3] powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
From: Greg Kurz @ 2014-11-27 9:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1417045163.5089.67.camel@kernel.crashing.org>
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.
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: [RESEND, V3] powerpc, xmon: Enable HW instruction breakpoint on POWER8
From: Anshuman Khandual @ 2014-11-27 8:16 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey
In-Reply-To: <20141126082548.34D5114017D@ozlabs.org>
On 11/26/2014 01:55 PM, Michael Ellerman wrote:
> On Tue, 2014-25-11 at 10:08:48 UTC, Anshuman Khandual wrote:
>> This patch enables support for hardware instruction breakpoints
>> on POWER8 with the help of a new register CIABR (Completed
>> Instruction Address Breakpoint Register). With this patch, single
>> hardware instruction breakpoint can be added and cleared during
>> any active xmon debug session. This hardware based instruction
>> breakpoint mechanism works correctly along with the existing TRAP
>> based instruction breakpoints available on xmon.
>
>
> Hi Anshuman,
>
>> diff --git a/arch/powerpc/include/asm/xmon.h b/arch/powerpc/include/asm/xmon.h
>> index 5eb8e59..5d17aec 100644
>> --- a/arch/powerpc/include/asm/xmon.h
>> +++ b/arch/powerpc/include/asm/xmon.h
>> @@ -29,5 +29,11 @@ static inline void xmon_register_spus(struct list_head *list) { };
>> extern int cpus_are_in_xmon(void);
>> #endif
>
> This file is the exported interface *of xmon*, it's not the place to put things
> that xmon needs internally.
>
> For now just put it in xmon.c
Okay.
>
>> +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_PPC_SPLPAR)
>> +#include <asm/plpar_wrappers.h>
>> +#else
>> +static inline long plapr_set_ciabr(unsigned long ciabr) {return 0; };
>> +#endif
>
> Also the ifdef is overly verbose, CONFIG_PPC_SPLPAR essentially depends on
> CONFIG_PPC_BOOK3S_64. So you can just use #ifdef CONFIG_PPC_SPLPAR.
Yeah, thats correct.
>
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index b988b5a..c2f601a 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -271,6 +273,55 @@ static inline void cinval(void *p)
>> }
>>
>> /*
>> + * write_ciabr
>> + *
>> + * 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.
>> + */
>
> I'm not really sure a function this small needs a documentation block.
>
> But if you're going to add one, PLEASE make sure it's an actual kernel-doc
> style comment.
>
> You can check with:
>
> $ ./scripts/kernel-doc -text arch/powerpc/xmon/xmon.c
>
> Which you'll notice prints:
>
> Warning(arch/powerpc/xmon/xmon.c): no structured comments found
>
> You need something like:
>
> /**
> * write_ciabr() - write the CIABR SPR
> * @ciabr: The value to write.
> *
> * This function writes a value to the CIABR register either directly through
> * mtspr instruction if the kernel is in HV privilege mode or calls a
> * hypervisor function to achieve the same in case the kernel is in supervisor
> * privilege mode.
> */
Sure.
>
>
>
> The rest of the patch is OK. But I was hoping you'd notice that we no longer
> support any cpus that implement CPU_FTR_IABR. And so you can just repurpose all
> the iabr logic for ciabr.
Okay.
>
> 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 ?
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-27 8:03 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-arch, Michael S. Tsirkin, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141127070919.GA4390@osiris>
> Code like
> spin_lock(&lock);
> if (copy_to_user(...))
> rc = ...
> spin_unlock(&lock);
> really *should* generate warnings like it did before.
>
> And *only* code like
> spin_lock(&lock);
Is only code like this valid or also with the spin_lock() dropped?
(e.g. the access in patch1 if I remember correctly)
So should page_fault_disable() increment the pagefault counter and the preempt
counter or only the first one?
> page_fault_disable();
> if (copy_to_user(...))
> rc = ...
> page_fault_enable();
> spin_unlock(&lock);
> should not generate warnings, since the author hopefully knew what he did.
>
> We could achieve that by e.g. adding a couple of pagefault disabled bits
> within current_thread_info()->preempt_count, which would allow
> pagefault_disable() and pagefault_enable() to modify a different part of
> preempt_count than it does now, so there is a way to tell if pagefaults have
> been explicitly disabled or are just a side effect of preemption being
> disabled.
> This would allow might_fault() to restore its old sane behaviour for the
> !page_fault_disabled() case.
So we would have pagefault code rely on:
in_disabled_pagefault() ( pagefault_disabled() ... whatever ) instead of
in_atomic().
I agree with this approach, as this is basically what I suggested in one of my
previous mails.
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-27 7:40 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-arch, David Hildenbrand, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141127070919.GA4390@osiris>
On Thu, Nov 27, 2014 at 08:09:19AM +0100, Heiko Carstens wrote:
> On Wed, Nov 26, 2014 at 07:04:47PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 26, 2014 at 05:51:08PM +0100, Christian Borntraeger wrote:
> > > > But this one was > giving users in field false positives.
> > >
> > > So lets try to fix those, ok? If we cant, then tough luck.
> >
> > Sure.
> > I think the simplest way might be to make spinlock disable
> > premption when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.
> >
> > As a result, userspace access will fail and caller will
> > get a nice error.
>
> Yes, _userspace_ now sees unpredictable behaviour, instead of that the
> kernel emits a big loud warning to the console.
So I don't object to adding more debugging at all.
Sure, would be nice.
But the fix is not an unconditional might_sleep
within might_fault, this would trigger false positives.
Rather, detect that you took a spinlock
without disabling preemption.
> Please consider this simple example:
>
> int bar(char __user *ptr)
> {
> ...
> if (copy_to_user(ptr, ...)
> return -EFAULT;
> ...
> }
>
> SYSCALL_DEFINE1(foo, char __user *, ptr)
> {
> int rc;
>
> ...
> rc = bar(ptr);
> if (rc)
> goto out;
> ...
> out:
> return rc;
> }
>
> The above simple system call just works fine, with and without your change,
> however if somebody (incorrectly) changes sys_foo() to the code below:
>
> spin_lock(&lock);
> rc = bar(ptr);
> if (rc)
> goto out;
> out:
> spin_unlock(&lock);
> return rc;
>
> Broken code like above used to generate warnings. With your change we won't
> see any warnings anymore. Instead we get random and bad behaviour:
>
> For !CONFIG_PREEMPT if the page at ptr is not mapped, the kernel will see
> a fault, potentially schedule and potentially deadlock on &lock.
> Without _any_ warning anymore.
>
> For CONFIG_PREEMPT if the page at ptr is mapped, everthing works. However if
> the page is not mapped, userspace now all of the sudden will see an invalid(!)
> -EFAULT return code, instead of that the kernel resolved the page fault.
> Yes, the kernel can't resolve the fault since we hold a spinlock. But the
> above bogus code did give warnings to give you an idea that something probably
> is not correct.
>
> Who on earth is supposed to debug crap like this???
>
> What we really want is:
>
> Code like
> spin_lock(&lock);
> if (copy_to_user(...))
> rc = ...
> spin_unlock(&lock);
> really *should* generate warnings like it did before.
>
> And *only* code like
> spin_lock(&lock);
> page_fault_disable();
> if (copy_to_user(...))
> rc = ...
> page_fault_enable();
> spin_unlock(&lock);
> should not generate warnings, since the author hopefully knew what he did.
>
> We could achieve that by e.g. adding a couple of pagefault disabled bits
> within current_thread_info()->preempt_count, which would allow
> pagefault_disable() and pagefault_enable() to modify a different part of
> preempt_count than it does now, so there is a way to tell if pagefaults have
> been explicitly disabled or are just a side effect of preemption being
> disabled.
> This would allow might_fault() to restore its old sane behaviour for the
> !page_fault_disabled() case.
Exactly. I agree, that would be a useful debugging tool.
In fact this comment in mm/memory.c hints at this:
* it would be nicer only to annotate paths which are not under
* pagefault_disable,
it further says
* however that requires a larger audit and
* providing helpers like get_user_atomic.
but I think that what you outline is a better way to do this.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Heiko Carstens @ 2014-11-27 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, David Hildenbrand, linux-kernel,
Christian Borntraeger, paulus, schwidefsky, akpm, linuxppc-dev,
mingo
In-Reply-To: <20141126170447.GC11202@redhat.com>
On Wed, Nov 26, 2014 at 07:04:47PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 26, 2014 at 05:51:08PM +0100, Christian Borntraeger wrote:
> > > But this one was > giving users in field false positives.
> >
> > So lets try to fix those, ok? If we cant, then tough luck.
>
> Sure.
> I think the simplest way might be to make spinlock disable
> premption when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.
>
> As a result, userspace access will fail and caller will
> get a nice error.
Yes, _userspace_ now sees unpredictable behaviour, instead of that the
kernel emits a big loud warning to the console.
Please consider this simple example:
int bar(char __user *ptr)
{
...
if (copy_to_user(ptr, ...)
return -EFAULT;
...
}
SYSCALL_DEFINE1(foo, char __user *, ptr)
{
int rc;
...
rc = bar(ptr);
if (rc)
goto out;
...
out:
return rc;
}
The above simple system call just works fine, with and without your change,
however if somebody (incorrectly) changes sys_foo() to the code below:
spin_lock(&lock);
rc = bar(ptr);
if (rc)
goto out;
out:
spin_unlock(&lock);
return rc;
Broken code like above used to generate warnings. With your change we won't
see any warnings anymore. Instead we get random and bad behaviour:
For !CONFIG_PREEMPT if the page at ptr is not mapped, the kernel will see
a fault, potentially schedule and potentially deadlock on &lock.
Without _any_ warning anymore.
For CONFIG_PREEMPT if the page at ptr is mapped, everthing works. However if
the page is not mapped, userspace now all of the sudden will see an invalid(!)
-EFAULT return code, instead of that the kernel resolved the page fault.
Yes, the kernel can't resolve the fault since we hold a spinlock. But the
above bogus code did give warnings to give you an idea that something probably
is not correct.
Who on earth is supposed to debug crap like this???
What we really want is:
Code like
spin_lock(&lock);
if (copy_to_user(...))
rc = ...
spin_unlock(&lock);
really *should* generate warnings like it did before.
And *only* code like
spin_lock(&lock);
page_fault_disable();
if (copy_to_user(...))
rc = ...
page_fault_enable();
spin_unlock(&lock);
should not generate warnings, since the author hopefully knew what he did.
We could achieve that by e.g. adding a couple of pagefault disabled bits
within current_thread_info()->preempt_count, which would allow
pagefault_disable() and pagefault_enable() to modify a different part of
preempt_count than it does now, so there is a way to tell if pagefaults have
been explicitly disabled or are just a side effect of preemption being
disabled.
This would allow might_fault() to restore its old sane behaviour for the
!page_fault_disabled() case.
^ permalink raw reply
* Re: [PATCH v2 4/4] powernv: powerpc: Add winkle support for offline cpus
From: Shreyas B Prabhu @ 2014-11-27 6:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <1417053311.5089.74.camel@kernel.crashing.org>
Hi Ben,
On Thursday 27 November 2014 07:25 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2014-11-25 at 16:47 +0530, Shreyas B. Prabhu wrote:
>
>> diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S
>> index 4673353..66874aa 100644
>> --- a/arch/powerpc/kernel/cpu_setup_power.S
>> +++ b/arch/powerpc/kernel/cpu_setup_power.S
>> @@ -55,6 +55,8 @@ _GLOBAL(__setup_cpu_power8)
>> beqlr
>> li r0,0
>> mtspr SPRN_LPID,r0
>> + mtspr SPRN_WORT,r0
>> + mtspr SPRN_WORC,r0
>> mfspr r3,SPRN_LPCR
>> ori r3, r3, LPCR_PECEDH
>> bl __init_LPCR
>> @@ -75,6 +77,8 @@ _GLOBAL(__restore_cpu_power8)
>> li r0,0
>> mtspr SPRN_LPID,r0
>> mfspr r3,SPRN_LPCR
>> + mtspr SPRN_WORT,r0
>> + mtspr SPRN_WORC,r0
>> ori r3, r3, LPCR_PECEDH
>> bl __init_LPCR
>> bl __init_HFSCR
>
> Clearing WORT and WORC might not be the best thing. We know the HW folks
> have been trying to tune those values and we might need to preserve what
> the boot FW has set.
>
> Can you get in touch with them and double check what we should do here ?
>
I observed these were always 0. I'll speak to HW folks as you suggested.
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 3311c8d..c9897cb 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -112,6 +112,16 @@ BEGIN_FTR_SECTION
>>
>> cmpwi cr1,r13,2
>>
>> + /* Check if last bit of HSPGR0 is set. This indicates whether we are
>> + * waking up from winkle */
>> + li r3,1
>> + mfspr r4,SPRN_HSPRG0
>> + and r5,r4,r3
>> + cmpwi cr4,r5,1 /* Store result in cr4 for later use */
>> +
>> + andc r4,r4,r3
>> + mtspr SPRN_HSPRG0,r4
>> +
>
> There is an open question here whether adding a beq cr4,+8 after the
> cmpwi (or a +4 after the andc) is worthwhile. Can you check ? (either
> measure or talk to HW folks).
Okay. This because mtspr is heavier op than beq?
> Also we could write directly to r13...
You mean use mr r13,r4 instead or GET_PACA?
>> GET_PACA(r13)
>> lbz r0,PACA_THREAD_IDLE_STATE(r13)
>> cmpwi cr2,r0,PNV_THREAD_NAP
>> diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
>> index c1d590f..78c30b0 100644
>> --- a/arch/powerpc/kernel/idle_power7.S
>> +++ b/arch/powerpc/kernel/idle_power7.S
>> @@ -19,8 +19,22 @@
>> #include <asm/kvm_book3s_asm.h>
>> #include <asm/opal.h>
>> #include <asm/cpuidle.h>
>> +#include <asm/mmu-hash64.h>
>>
>> #undef DEBUG
>> +/*
>> + * Use unused space in the interrupt stack to save and restore
>> + * registers for winkle support.
>> + */
>> +#define _SDR1 GPR3
>> +#define _RPR GPR4
>> +#define _SPURR GPR5
>> +#define _PURR GPR6
>> +#define _TSCR GPR7
>> +#define _DSCR GPR8
>> +#define _AMOR GPR9
>> +#define _PMC5 GPR10
>> +#define _PMC6 GPR11
>
> WORT/WORTC need saving restoring
The reason I skipped this was because these were always 0. But since its
set by FW, I'll save and restore them.
>
>> /* Idle state entry routines */
>>
>> @@ -153,32 +167,60 @@ 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.
>> */
>> + LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround)
>> + lbz r3,0(r3)
>> + cmpwi r3,1
>> + bne common_enter
>> +
>> ori r15,r15,PNV_CORE_IDLE_LOCK_BIT
>> stwcx. r15,0,r14
>> bne- lwarx_loop1
>>
>> /* Fast sleep workaround */
>> + mfcr r16 /* Backup CR to a non-volatile register */
>> li r3,1
>> li r4,1
>> li r0,OPAL_CONFIG_CPU_IDLE_STATE
>> bl opal_call_realmode
>> + mtcr r16 /* Restore CR */
>
> Why isn't the above already in the previous patch ? Also see my comment
> about using a non-volatile CR instead.
In the previous patch I wasn't using any CR after this OPAL call. Hence
I had skipped it. As you suggested I'll avoid this by using CR[234].
>
>> /* Clear Lock bit */
>> andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
>> stw r15,0(r14)
>>
>> -common_enter: /* common code for all the threads entering sleep */
>> +common_enter: /* common code for all the threads entering sleep or winkle*/
>> + bgt cr1,enter_winkle
>> IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>> +enter_winkle:
>> + /*
>> + * Note all register i.e per-core, per-subcore or per-thread is saved
>> + * here since any thread in the core might wake up first
>> + */
>> + mfspr r3,SPRN_SDR1
>> + std r3,_SDR1(r1)
>> + mfspr r3,SPRN_RPR
>> + std r3,_RPR(r1)
>> + mfspr r3,SPRN_SPURR
>> + std r3,_SPURR(r1)
>> + mfspr r3,SPRN_PURR
>> + std r3,_PURR(r1)
>> + mfspr r3,SPRN_TSCR
>> + std r3,_TSCR(r1)
>> + mfspr r3,SPRN_DSCR
>> + std r3,_DSCR(r1)
>> + mfspr r3,SPRN_AMOR
>> + std r3,_AMOR(r1)
>> + mfspr r3,SPRN_PMC5
>> + std r3,_PMC5(r1)
>> + mfspr r3,SPRN_PMC6
>> + std r3,_PMC6(r1)
>> + IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
>>
>> _GLOBAL(power7_idle)
>> /* Now check if user or arch enabled NAP mode */
>> @@ -201,6 +243,12 @@ _GLOBAL(power7_sleep)
>> b power7_powersave_common
>> /* No return */
>>
>> +_GLOBAL(power7_winkle)
>> + li r3,PNV_THREAD_WINKLE
>> + li r4,1
>> + b power7_powersave_common
>> + /* No return */
>> +
>> #define CHECK_HMI_INTERRUPT \
>> mfspr r0,SPRN_SRR1; \
>> BEGIN_FTR_SECTION_NESTED(66); \
>> @@ -250,22 +298,54 @@ lwarx_loop2:
>> */
>> bne lwarx_loop2
>>
>> - cmpwi cr2,r15,0
>> + cmpwi cr2,r15,0 /* Check if first in core */
>> + lbz r4,PACA_SUBCORE_SIBLING_MASK(r13)
>> + and r4,r4,r15
>> + cmpwi cr3,r4,0 /* Check if first in subcore */
>> +
>> + /*
>> + * At this stage
>> + * cr1 - 01 if waking up from sleep or winkle
>> + * cr2 - 10 if first thread to wakeup in core
>> + * cr3 - 10 if first thread to wakeup in subcore
>> + * cr4 - 10 if waking up from winkle
>> + */
>> +
>> or r15,r15,r7 /* Set thread bit */
>>
>> - beq cr2,first_thread
>> + beq cr3,first_thread_in_subcore
>>
>> - /* Not first thread in core to wake up */
>> + /* Not first thread in subcore to wake up */
>> stwcx. r15,0,r14
>> bne- lwarx_loop2
>> b common_exit
>>
>> -first_thread:
>> - /* First thread in core to wakeup */
>> +first_thread_in_subcore:
>> + /* First thread in subcore to wakeup set the lock bit */
>> ori r15,r15,PNV_CORE_IDLE_LOCK_BIT
>> stwcx. r15,0,r14
>> bne- lwarx_loop2
>>
>> + /*
>> + * If waking up from sleep, subcore state is not lost. Hence
>> + * skip subcore state restore
>> + */
>> + bne cr4,subcore_state_restored
>> +
>> + /* Restore per-subcore state */
>> + ld r4,_SDR1(r1)
>> + mtspr SPRN_SDR1,r4
>> + ld r4,_RPR(r1)
>> + mtspr SPRN_RPR,r4
>> + ld r4,_AMOR(r1)
>> + mtspr SPRN_AMOR,r4
>> +
>> +subcore_state_restored:
>> + /* Check if the thread is also the first thread in the core. If not,
>> + * skip to clear_lock */
>> + bne cr2,clear_lock
>> +
>> +first_thread_in_core:
>> LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround)
>> lbz r3,0(r3)
>> cmpwi r3,1
>> @@ -280,21 +360,71 @@ first_thread:
>> 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 */
>> +timebase_resync:
>> + /* Do timebase resync only if the core truly woke up from
>> + * sleep/winkle */
>> ble cr1,clear_lock
>>
>> -timebase_resync:
>> /* Time base re-sync */
>> + mfcr r16 /* Backup CR into a non-volatile register */
>> li r0,OPAL_RESYNC_TIMEBASE
>> bl opal_call_realmode;
>> /* TODO: Check r3 for failure */
>> + mtcr r16 /* Restore CR */
>> +
>> + /*
>> + * If waking up from sleep, per core state is not lost, skip to
>> + * clear_lock.
>> + */
>> + bne cr4,clear_lock
>> +
>> + /* Restore per core state */
>> + ld r4,_TSCR(r1)
>> + mtspr SPRN_TSCR,r4
>>
>> clear_lock:
>> andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
>> stw r15,0(r14)
>>
>> common_exit:
>> + /* Common to all threads
>> + *
>> + * If waking up from sleep, hypervisor state is not lost. Hence
>> + * skip hypervisor state restore.
>> + */
>> + bne cr4,hypervisor_state_restored
>> +
>> + /* Waking up from winkle */
>> +
>> + /* Restore per thread state */
>> + bl __restore_cpu_power8
>> +
>> + /* Restore SLB from PACA */
>> + ld r8,PACA_SLBSHADOWPTR(r13)
>> +
>> + .rept SLB_NUM_BOLTED
>> + li r3, SLBSHADOW_SAVEAREA
>> + LDX_BE r5, r8, r3
>> + addi r3, r3, 8
>> + LDX_BE r6, r8, r3
>> + andis. r7,r5,SLB_ESID_V@h
>> + beq 1f
>> + slbmte r6,r5
>> +1: addi r8,r8,16
>> + .endr
>> +
>> + ld r4,_SPURR(r1)
>> + mtspr SPRN_SPURR,r4
>> + ld r4,_PURR(r1)
>> + mtspr SPRN_PURR,r4
>> + ld r4,_DSCR(r1)
>> + mtspr SPRN_DSCR,r4
>> + ld r4,_PMC5(r1)
>> + mtspr SPRN_PMC5,r4
>> + ld r4,_PMC6(r1)
>> + mtspr SPRN_PMC6,r4
>> +
>> +hypervisor_state_restored:
>> li r5,PNV_THREAD_RUNNING
>> stb r5,PACA_THREAD_IDLE_STATE(r13)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
>> index b2aa93b..e1e91e0 100644
>> --- a/arch/powerpc/platforms/powernv/opal-wrappers.S
>> +++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
>> @@ -191,6 +191,7 @@ return_from_opal_call:
>> #ifdef __LITTLE_ENDIAN__
>> FIXUP_ENDIAN
>> #endif
>> + ld r2,PACATOC(r13)
>> ld r12,_LINK(r1)
>> mtlr r12
>> blr
>> @@ -284,6 +285,7 @@ OPAL_CALL(opal_sensor_read, OPAL_SENSOR_READ);
>> OPAL_CALL(opal_get_param, OPAL_GET_PARAM);
>> OPAL_CALL(opal_set_param, OPAL_SET_PARAM);
>> OPAL_CALL(opal_handle_hmi, OPAL_HANDLE_HMI);
>> +OPAL_CALL(opal_slw_set_reg, OPAL_SLW_SET_REG);
>> OPAL_CALL(opal_register_dump_region, OPAL_REGISTER_DUMP_REGION);
>> OPAL_CALL(opal_unregister_dump_region, OPAL_UNREGISTER_DUMP_REGION);
>> OPAL_CALL(opal_pci_set_phb_cxl_mode, OPAL_PCI_SET_PHB_CXL_MODE);
>> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
>> index 17fb98c..4a886a1 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -40,6 +40,7 @@
>> #include <asm/cpuidle.h>
>>
>> #include "powernv.h"
>> +#include "subcore.h"
>>
>> static void __init pnv_setup_arch(void)
>> {
>> @@ -293,6 +294,74 @@ static void __init pnv_setup_machdep_rtas(void)
>> #endif /* CONFIG_PPC_POWERNV_RTAS */
>>
>> static u32 supported_cpuidle_states;
>> +int pnv_save_sprs_for_winkle(void)
>> +{
>> + int cpu;
>> + int rc;
>> +
>> + /*
>> + * hid0, hid1, hid4, hid5, hmeer and lpcr values are symmetric accross
>> + * all cpus at boot. Get these reg values of current cpu and use the
>> + * same accross all cpus.
>> + */
>> + uint64_t lpcr_val = mfspr(SPRN_LPCR);
>> + uint64_t hid0_val = mfspr(SPRN_HID0);
>> + uint64_t hid1_val = mfspr(SPRN_HID1);
>> + uint64_t hid4_val = mfspr(SPRN_HID4);
>> + uint64_t hid5_val = mfspr(SPRN_HID5);
>> + uint64_t hmeer_val = mfspr(SPRN_HMEER);
>> +
>> + for_each_possible_cpu(cpu) {
>> + uint64_t pir = get_hard_smp_processor_id(cpu);
>> + uint64_t hsprg0_val = (uint64_t)&paca[cpu];
>> +
>> + /*
>> + * HSPRG0 is used to store the cpu's pointer to paca. Hence last
>> + * 3 bits are guaranteed to be 0. Program slw to restore HSPRG0
>> + * with 63rd bit set, so that when a thread wakes up at 0x100 we
>> + * can use this bit to distinguish between fastsleep and
>> + * deep winkle.
>> + */
>> + hsprg0_val |= 1;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HSPRG0, hsprg0_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + /* HIDs are per core registers */
>> + if (cpu_thread_in_core(cpu) == 0) {
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HMEER, hmeer_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HID0, hid0_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HID1, hid1_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HID4, hid4_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + rc = opal_slw_set_reg(pir, SPRN_HID5, hid5_val);
>> + if (rc != 0)
>> + return rc;
>> +
>> + }
>> +
>> + }
>> +
>> + return 0;
>> +
>> +}
>>
>> static void pnv_alloc_idle_core_states(void)
>> {
>> @@ -324,6 +393,10 @@ static void pnv_alloc_idle_core_states(void)
>>
>> }
>> }
>> + update_subcore_sibling_mask();
>> + if (supported_cpuidle_states & OPAL_PM_WINKLE_ENABLED)
>> + pnv_save_sprs_for_winkle();
>> +
>> }
>>
>> u32 pnv_get_supported_cpuidle_states(void)
>> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
>> index 12b761a..5e35857 100644
>> --- a/arch/powerpc/platforms/powernv/smp.c
>> +++ b/arch/powerpc/platforms/powernv/smp.c
>> @@ -167,7 +167,9 @@ static void pnv_smp_cpu_kill_self(void)
>> mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
>> while (!generic_check_cpu_restart(cpu)) {
>> ppc64_runlatch_off();
>> - if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
>> + if (idle_states & OPAL_PM_WINKLE_ENABLED)
>> + power7_winkle();
>> + else if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
>> (idle_states & OPAL_PM_SLEEP_ENABLED_ER1))
>> power7_sleep();
>> else
>> diff --git a/arch/powerpc/platforms/powernv/subcore.c b/arch/powerpc/platforms/powernv/subcore.c
>> index c87f96b..f60f80a 100644
>> --- a/arch/powerpc/platforms/powernv/subcore.c
>> +++ b/arch/powerpc/platforms/powernv/subcore.c
>> @@ -160,6 +160,18 @@ static void wait_for_sync_step(int step)
>> mb();
>> }
>>
>> +static void update_hid_in_slw(u64 hid0)
>> +{
>> + u64 idle_states = pnv_get_supported_cpuidle_states();
>> +
>> + if (idle_states & OPAL_PM_WINKLE_ENABLED) {
>> + /* OPAL call to patch slw with the new HID0 value */
>> + u64 cpu_pir = hard_smp_processor_id();
>> +
>> + opal_slw_set_reg(cpu_pir, SPRN_HID0, hid0);
>> + }
>> +}
>> +
>> static void unsplit_core(void)
>> {
>> u64 hid0, mask;
>> @@ -179,6 +191,7 @@ static void unsplit_core(void)
>> hid0 = mfspr(SPRN_HID0);
>> hid0 &= ~HID0_POWER8_DYNLPARDIS;
>> mtspr(SPRN_HID0, hid0);
>> + update_hid_in_slw(hid0);
>>
>> while (mfspr(SPRN_HID0) & mask)
>> cpu_relax();
>> @@ -215,6 +228,7 @@ static void split_core(int new_mode)
>> hid0 = mfspr(SPRN_HID0);
>> hid0 |= HID0_POWER8_DYNLPARDIS | split_parms[i].value;
>> mtspr(SPRN_HID0, hid0);
>> + update_hid_in_slw(hid0);
>>
>> /* Wait for it to happen */
>> while (!(mfspr(SPRN_HID0) & split_parms[i].mask))
>> @@ -251,6 +265,25 @@ bool cpu_core_split_required(void)
>> return true;
>> }
>>
>> +void update_subcore_sibling_mask(void)
>> +{
>> + int cpu;
>> + /*
>> + * sibling mask for the first cpu. Left shift this by required bits
>> + * to get sibling mask for the rest of the cpus.
>> + */
>> + int sibling_mask_first_cpu = (1 << threads_per_subcore) - 1;
>> +
>> + for_each_possible_cpu(cpu) {
>> + int tid = cpu_thread_in_core(cpu);
>> + int offset = (tid / threads_per_subcore) * threads_per_subcore;
>> + int mask = sibling_mask_first_cpu << offset;
>> +
>> + paca[cpu].subcore_sibling_mask = mask;
>> +
>> + }
>> +}
>> +
>> static int cpu_update_split_mode(void *data)
>> {
>> int cpu, new_mode = *(int *)data;
>> @@ -284,6 +317,7 @@ static int cpu_update_split_mode(void *data)
>> /* Make the new mode public */
>> subcores_per_core = new_mode;
>> threads_per_subcore = threads_per_core / subcores_per_core;
>> + update_subcore_sibling_mask();
>>
>> /* Make sure the new mode is written before we exit */
>> mb();
>> diff --git a/arch/powerpc/platforms/powernv/subcore.h b/arch/powerpc/platforms/powernv/subcore.h
>> index 148abc9..604eb40 100644
>> --- a/arch/powerpc/platforms/powernv/subcore.h
>> +++ b/arch/powerpc/platforms/powernv/subcore.h
>> @@ -15,4 +15,5 @@
>>
>> #ifndef __ASSEMBLY__
>> void split_core_secondary_loop(u8 *state);
>> +extern void update_subcore_sibling_mask(void);
>> #endif
>
>
Thanks,
Shreyas
^ 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