* 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
* Re: [PATCH v2 3/4] powernv: cpuidle: Redesign idle states management
From: Shreyas B Prabhu @ 2014-11-27 5:55 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-pm, Rafael J. Wysocki, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1417048678.5089.71.camel@kernel.crashing.org>
Hi Ben,
On Thursday 27 November 2014 06:07 AM, Benjamin Herrenschmidt wrote:
>
>>
>> @@ -37,8 +38,7 @@
>>
>> /*
>> * Pass requested state in r3:
>> - * 0 - nap
>> - * 1 - sleep
>> + * r3 - PNV_THREAD_NAP/SLEEP/WINKLE
>> *
>> * To check IRQ_HAPPENED in r4
>> * 0 - don't check
>> @@ -123,12 +123,62 @@ power7_enter_nap_mode:
>> li r4,KVM_HWTHREAD_IN_NAP
>> stb r4,HSTATE_HWTHREAD_STATE(r13)
>> #endif
>> - cmpwi cr0,r3,1
>> - beq 2f
>> + stb r3,PACA_THREAD_IDLE_STATE(r13)
>> + cmpwi cr1,r3,PNV_THREAD_SLEEP
>> + bge cr1,2f
>> IDLE_STATE_ENTER_SEQ(PPC_NAP)
>> /* No return */
>> -2: IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>> - /* No return */
>> +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
>> +
>> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13)
>
> I assume we have already saved all non-volatile registers ? Because you
> are clobbering one here and more below.
Yes. At this stage the all non-volatile registers are already saved in
stack.
>
>> +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
>
> This looks wrong. If the workaround is 0, we don't do the stwcx. at
> all... Did you try with pnv_need_fastsleep_workaround set to 0 ? It
> should work most of the time as long as you don't hit the fairly
> rare race window :)
>
My bad. I missed the stwcx. in the pnv_need_fastsleep_workaround = 0 path.
> Also it would be nice to make the above a dynamically patches feature
> section, though that means pnv_need_fastsleep_workaround needs to turn
> into a CPU feature bit and that needs to be done *very* early on.
>
> Another option is to patch out manually from the pnv code the pair:
>
> andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
> beq last_thread
>
> To turn them into nops by hand rather than using the feature system.
>
Okay. I'll see which works out best here.
>> + /*
>> + * 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 */
>
> It's a lock, I would add a lwsync here to be safe, and I would add an
> isync before the bne- above. Just to ensure that whatever is done
> inside that locked section remains in there.
>
Okay. Will add it.
>> + andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
>> + stw r15,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 */
>
> I'd be more comfortable if we patched that instruction at boot with the
> right mask.
>
Okay. I'll make the change.
>> + 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
>
> We should do some smt priority games here otherwise the spinning threads
> are going to slow down the one with the lock. Basically, if we see the
> lock held, go out of line, smt_low, spin on a normal load, and when
> smt_medium and go back to lwarx
>
Okay.
>> + 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
>
> Same comment about dynamic patching.
Okay.
>
>> + /* skip fastsleep workaround if its not needed */
>> + bne timebase_resync
>> +
>> + /* Undo fast sleep workaround */
>> + mfcr r16 /* Backup CR into a non-volatile register */
>
> Why ? If you have your non-volatiles saved you can use an NV CR like CR2
> no ? You need to restore those anyway...
>
I wasn't sure CRs were preserved during OPAL call. I'll make the change
to use CR[234].
> Also same comments as on the way down vs barriers when doing
> lock/unlock.
>
Okay.
>> + 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;
>> -
>> /* 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
>> +
>> 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)
>> + 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
>> +
>> OPAL_CALL(opal_invalid_call, OPAL_INVALID_CALL);
>> OPAL_CALL(opal_console_write, OPAL_CONSOLE_WRITE);
>> OPAL_CALL(opal_console_read, OPAL_CONSOLE_READ);
>> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
>> index 34c6665..17fb98c 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -36,6 +36,8 @@
>> #include <asm/opal.h>
>> #include <asm/kexec.h>
>> #include <asm/smp.h>
>> +#include <asm/cputhreads.h>
>> +#include <asm/cpuidle.h>
>>
>> #include "powernv.h"
>>
>> @@ -292,11 +294,45 @@ static void __init pnv_setup_machdep_rtas(void)
>>
>> static u32 supported_cpuidle_states;
>>
>> +static void pnv_alloc_idle_core_states(void)
>> +{
>> + int i, j;
>> + int nr_cores = cpu_nr_cores();
>> + u32 *core_idle_state;
>> +
>> + /*
>> + * core_idle_state - First 8 bits track the idle state of each thread
>> + * of the core. The 8th bit is the lock bit. Initially all thread bits
>> + * are set. They are cleared when the thread enters deep idle state
>> + * like sleep and winkle. Initially the lock bit is cleared.
>> + * The lock bit has 2 purposes
>> + * a. While the first thread is restoring core state, it prevents
>> + * from other threads in the core from switching to prcoess context.
>> + * b. While the last thread in the core is saving the core state, it
>> + * prevent a different thread from waking up.
>> + */
>> + for (i = 0; i < nr_cores; i++) {
>> + int first_cpu = i * threads_per_core;
>> + int node = cpu_to_node(first_cpu);
>> +
>> + core_idle_state = kmalloc_node(sizeof(u32), GFP_KERNEL, node);
>> + for (j = 0; j < threads_per_core; j++) {
>> + int cpu = first_cpu + j;
>> +
>> + paca[cpu].core_idle_state_ptr = core_idle_state;
>> + paca[cpu].thread_idle_state = PNV_THREAD_RUNNING;
>> +
>> + }
>> + }
>> +}
>> +
>> u32 pnv_get_supported_cpuidle_states(void)
>> {
>> return supported_cpuidle_states;
>> }
>> +EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
>>
>> +u8 pnv_need_fastsleep_workaround;
>> static int __init pnv_init_idle_states(void)
>> {
>> struct device_node *power_mgt;
>> @@ -306,6 +342,7 @@ static int __init pnv_init_idle_states(void)
>> int i;
>>
>> supported_cpuidle_states = 0;
>> + pnv_need_fastsleep_workaround = 0;
>>
>> if (cpuidle_disable != IDLE_NO_OVERRIDE)
>> return 0;
>> @@ -332,13 +369,14 @@ static int __init pnv_init_idle_states(void)
>> flags = be32_to_cpu(idle_state_flags[i]);
>> supported_cpuidle_states |= flags;
>> }
>> -
>> + if (supported_cpuidle_states & OPAL_PM_SLEEP_ENABLED_ER1)
>> + pnv_need_fastsleep_workaround = 1;
>> + pnv_alloc_idle_core_states();
>> return 0;
>> }
>>
>> subsys_initcall(pnv_init_idle_states);
>>
>> -
>> static int __init pnv_probe(void)
>> {
>> unsigned long root = of_get_flat_dt_root();
>> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
>> index 3dc4cec..12b761a 100644
>> --- a/arch/powerpc/platforms/powernv/smp.c
>> +++ b/arch/powerpc/platforms/powernv/smp.c
>> @@ -167,7 +167,8 @@ 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_SLEEP_ENABLED) ||
>> + (idle_states & OPAL_PM_SLEEP_ENABLED_ER1))
>> power7_sleep();
>> else
>> power7_nap(1);
>> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
>> index 0a7d827..a489b56 100644
>> --- a/drivers/cpuidle/cpuidle-powernv.c
>> +++ b/drivers/cpuidle/cpuidle-powernv.c
>> @@ -208,7 +208,8 @@ static int powernv_add_idle_states(void)
>> nr_idle_states++;
>> }
>>
>> - if (flags & OPAL_PM_SLEEP_ENABLED) {
>> + if (flags & OPAL_PM_SLEEP_ENABLED ||
>> + flags & OPAL_PM_SLEEP_ENABLED_ER1) {
>> /* Add FASTSLEEP state */
>> strcpy(powernv_states[nr_idle_states].name, "FastSleep");
>> strcpy(powernv_states[nr_idle_states].desc, "FastSleep");
>
>
Thanks,
Shreyas
^ permalink raw reply
* [git pull] Please pull mpe.git for-linus branch (for powerpc)
From: Michael Ellerman @ 2014-11-27 5:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: mahesh, linux-kernel, gwshan, anton, ldufour, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]
Hi Linus,
Here are five fixes for you to pull please.
I think these are all rc6 material, but I'm still learning so let me know if
you disagree :)
They're all CC'ed to stable except the "Fix PE state format" one which went in
this release.
cheers
The following changes since commit d7ce4377494adfaf8afb15ecf4f07d399bbf13d9:
powerpc/fsl_msi: mark the msi cascade handler IRQF_NO_THREAD (2014-11-17 22:00:30 -0600)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux.git for-linus
for you to fetch changes up to 152d44a853e42952f6c8a504fb1f8eefd21fd5fd:
powerpc: 32 bit getcpu VDSO function uses 64 bit instructions (2014-11-27 09:42:12 +1100)
----------------------------------------------------------------
Anton Blanchard (1):
powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
Gavin Shan (2):
powerpc/eeh: Fix PE state format
powerpc/powernv: Replace OPAL_DEASSERT_RESET with EEH_RESET_DEACTIVATE
Laurent Dufour (1):
powerpc/pseries: Fix endiannes issue in RTAS call from xmon
Mahesh Salgaonkar (1):
powerpc/powernv: Fix the hmi event version check.
arch/powerpc/kernel/eeh_sysfs.c | 2 +-
arch/powerpc/kernel/vdso32/getcpu.S | 4 ++--
arch/powerpc/platforms/powernv/opal-hmi.c | 2 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 2 +-
arch/powerpc/xmon/xmon.c | 6 +++---
5 files changed, 8 insertions(+), 8 deletions(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Peter Bergner @ 2014-11-26 23:50 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, paulus, Anton Blanchard
In-Reply-To: <1417041497.12952.2.camel@concordia>
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.
> 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.
Peter
^ permalink raw reply
* RE: [PATCH v2 0/3] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled
From: Yuantian Tang @ 2014-11-27 3:30 UTC (permalink / raw)
To: Mike Turquette
Cc: Scott Wood, Kevin Hao, Gerhard Sittig,
linuxppc-dev@lists.ozlabs.org, Jingchang Lu
In-Reply-To: <1415333263.23458.417.camel@snotra.buserror.net>
SGVsbG8gTWlrZSwNCg0KQ291bGQgeW91IHBsZWFzZSBhcHBseSB0aGlzIHBhdGNoPw0KVGhpcyBw
YXRjaCBoYXMgYmVlbiBhY2tlZCBmb3IgYSB3aGlsZS4NCg0KVGhhbmtzLA0KWXVhbnRpYW4NCg0K
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBMaW51eHBwYy1kZXYNCj4gW21h
aWx0bzpsaW51eHBwYy1kZXYtYm91bmNlcytiMjk5ODM9ZnJlZXNjYWxlLmNvbUBsaXN0cy5vemxh
YnMub3JnXSBPbg0KPiBCZWhhbGYgT2YgU2NvdHQgV29vZA0KPiBTZW50OiBGcmlkYXksIE5vdmVt
YmVyIDA3LCAyMDE0IDEyOjA4IFBNDQo+IFRvOiBLZXZpbiBIYW8NCj4gQ2M6IE1pa2UgVHVycXVl
dHRlOyBHZXJoYXJkIFNpdHRpZzsgTHUgSmluZ2NoYW5nLUIzNTA4MzsNCj4gbGludXhwcGMtZGV2
QGxpc3RzLm96bGFicy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MiAwLzNdIGZpeCBhIGtl
cm5lbCBwYW5pYyBvbiBmc2wgY29yZW5ldCBib2FyZCB3aGVuDQo+IENPTkZJR19DTEtfUFBDX0NP
UkVORVQgaXMgZW5hYmxlZA0KPiANCj4gT24gU3VuLCAyMDE0LTEwLTE5IGF0IDE0OjExICswODAw
LCBLZXZpbiBIYW8gd3JvdGU6DQo+ID4gSGksDQo+ID4NCj4gPiBJIGhhdmUgZG9uZSBhIGJvb3Qg
dGVzdCBvbiBwMjAxNHJkYiBhbmQgdDQyNDBxZHMgYm9hcmRzLiBJIGRvbid0IGhhdmUNCj4gPiBh
biBhY2Nlc3MgdG8gbXBjNTEyeCBib2FyZCwgc28gb25seSBidWlsZCB0ZXN0IGZvciB0aGF0Lg0K
PiA+DQo+ID4gdjI6DQo+ID4gIC0gUmV2ZXJ0IHRoZSBjb21taXQgZGE3ODhhY2IyODM4IGZpcnN0
Lg0KPiA+ICAtIEludm9rZSBvZl9jbGtfaW5pdCgpIGZyb20gYSBjb21tb24gcGxhY2UuDQo+ID4N
Cj4gPiB2MQ0KPiA+IFRoaXMgdHJpZXMgdG8gZml4IGEga2VybmVsIHBhbmljIGludHJvZHVjZWQg
YnkgY29tbWl0IGRhNzg4YWNiMjgzOA0KPiA+ICgiY2xrOiBwcGMtY29yZW5ldDogRml4IFNlY3Rp
b24gbWlzbWF0Y2ggd2FybmluZyIpLg0KPiA+DQo+ID4gS2V2aW4gSGFvICgzKToNCj4gPiAgIFJl
dmVydCAiY2xrOiBwcGMtY29yZW5ldDogRml4IFNlY3Rpb24gbWlzbWF0Y2ggd2FybmluZyINCj4g
PiAgIHBvd2VycGM6IGNhbGwgb2ZfY2xrX2luaXQoKSBmcm9tIHRpbWVfaW5pdCgpDQo+ID4gICBj
bGs6IHBwYy1jb3JlbmV0OiBmaXggc2VjdGlvbiBtaXNtYXRjaCB3YXJuaW5nDQo+ID4NCj4gPiAg
YXJjaC9wb3dlcnBjL2tlcm5lbC90aW1lLmMgICAgICAgICAgICAgICAgICAgIHwgIDUgKysrKw0K
PiA+ICBhcmNoL3Bvd2VycGMvcGxhdGZvcm1zLzUxMngvY2xvY2stY29tbW9uY2xrLmMgfCAxMSAr
KysrLS0tDQo+ID4gIGRyaXZlcnMvY2xrL2Nsay1wcGMtY29yZW5ldC5jICAgICAgICAgICAgICAg
ICB8IDQzICsrKystLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+ICAzIGZpbGVzIGNoYW5nZWQs
IDE2IGluc2VydGlvbnMoKyksIDQzIGRlbGV0aW9ucygtKQ0KPiA+DQo+IA0KPiBBY2tlZC1ieTog
U2NvdHQgV29vZCA8c2NvdHR3b29kQGZyZWVzY2FsZS5jb20+DQo+IA0KPiBXaG9zZSB0cmVlIHNo
b3VsZCB0aGlzIGdvIHRocm91Z2g/DQo+IA0KPiAtU2NvdHQNCj4gDQo+IA0KPiBfX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXw0KPiBMaW51eHBwYy1kZXYgbWFp
bGluZyBsaXN0DQo+IExpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnDQo+IGh0dHBzOi8vbGlz
dHMub3psYWJzLm9yZy9saXN0aW5mby9saW51eHBwYy1kZXYNCg==
^ permalink raw reply
* Right location in sysfs for dlpar file
From: Benjamin Herrenschmidt @ 2014-11-27 3:12 UTC (permalink / raw)
To: Greg KH; +Cc: Nathan Fontenot, linuxppc-dev
Hi Greg,
So Nathan is working on a patch series to cleanup and improve our
"DLPAR" infrastructure which is basically our hotplug mechanism when
running under the PowerVM (aka pHyp) and KVM hypervisors.
I'll let Nathan give you a bit more details/background and answer
subsequent question you might have as this is really his area of
expertise.
To cut a long story short, we need a sysfs file that allows our
userspace tools to notify the kernel of hotplug events coming from
the management console (which talks to userspace daemons using a
proprietary protocol) to "initiate" the hotplug operations, which in
turn get dispatched internally in the kernel to the right subsystem
(memory, cpu, pci, ...) based on the resource type.
On IRC, Greg suggested /sys/firmware and /sys/hypervisor which both
look like a reasonable option to me, probably better than dlpar...
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v2 4/4] powernv: powerpc: Add winkle support for offline cpus
From: Benjamin Herrenschmidt @ 2014-11-27 1:55 UTC (permalink / raw)
To: Shreyas B. Prabhu; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <1416914279-30384-5-git-send-email-shreyas@linux.vnet.ibm.com>
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 ?
> 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). Also we could write directly to r13...
> 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
> /* 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.
> /* 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
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability
From: Paul Mackerras @ 2014-11-27 0:45 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, agraf, kvm-ppc, kvm
In-Reply-To: <1413815340-22426-3-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
On Mon, Oct 20, 2014 at 07:59:00PM +0530, Aneesh Kumar K.V wrote:
> Minor cleanup
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 78e689b066f1..2922f8d127ff 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -523,7 +523,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
> unsigned long *args = &vcpu->arch.gpr[4];
> __be64 *hp, *hptes[4];
> unsigned long tlbrb[4];
> - long int i, j, k, n, found, indexes[4];
> + long int i, j, k, collected_hpte, found, indexes[4];
Hmmm... I don't find it more readable, because "collected_hpte" sounds
like it contains a HPTE value. Also I don't like using a long name
for something that is just a temporary value inside a function, and
"n" is a suitable name for a temporary variable counting the number of
things we have. I would prefer just adding a comment like this:
- n = 0;
+ n = 0; /* # values collected in tlbrb[], indexes[] etc. */
Paul.
^ permalink raw reply
* Re: [PATCH v2 3/4] powernv: cpuidle: Redesign idle states management
From: Benjamin Herrenschmidt @ 2014-11-27 0:37 UTC (permalink / raw)
To: Shreyas B. Prabhu
Cc: linux-pm, Rafael J. Wysocki, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1416914279-30384-4-git-send-email-shreyas@linux.vnet.ibm.com>
>
> @@ -37,8 +38,7 @@
>
> /*
> * Pass requested state in r3:
> - * 0 - nap
> - * 1 - sleep
> + * r3 - PNV_THREAD_NAP/SLEEP/WINKLE
> *
> * To check IRQ_HAPPENED in r4
> * 0 - don't check
> @@ -123,12 +123,62 @@ power7_enter_nap_mode:
> li r4,KVM_HWTHREAD_IN_NAP
> stb r4,HSTATE_HWTHREAD_STATE(r13)
> #endif
> - cmpwi cr0,r3,1
> - beq 2f
> + stb r3,PACA_THREAD_IDLE_STATE(r13)
> + cmpwi cr1,r3,PNV_THREAD_SLEEP
> + bge cr1,2f
> IDLE_STATE_ENTER_SEQ(PPC_NAP)
> /* No return */
> -2: IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
> - /* No return */
> +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
> +
> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13)
I assume we have already saved all non-volatile registers ? Because you
are clobbering one here and more below.
> +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
This looks wrong. If the workaround is 0, we don't do the stwcx. at
all... Did you try with pnv_need_fastsleep_workaround set to 0 ? It
should work most of the time as long as you don't hit the fairly
rare race window :)
Also it would be nice to make the above a dynamically patches feature
section, though that means pnv_need_fastsleep_workaround needs to turn
into a CPU feature bit and that needs to be done *very* early on.
Another option is to patch out manually from the pnv code the pair:
andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
beq last_thread
To turn them into nops by hand rather than using the feature system.
> + /*
> + * 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 */
It's a lock, I would add a lwsync here to be safe, and I would add an
isync before the bne- above. Just to ensure that whatever is done
inside that locked section remains in there.
> + andi. r15,r15,PNV_CORE_IDLE_THREAD_BITS
> + stw r15,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 */
I'd be more comfortable if we patched that instruction at boot with the
right mask.
> + 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
We should do some smt priority games here otherwise the spinning threads
are going to slow down the one with the lock. Basically, if we see the
lock held, go out of line, smt_low, spin on a normal load, and when
smt_medium and go back to lwarx
> + 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
Same comment about dynamic patching.
> + /* skip fastsleep workaround if its not needed */
> + bne timebase_resync
> +
> + /* Undo fast sleep workaround */
> + mfcr r16 /* Backup CR into a non-volatile register */
Why ? If you have your non-volatiles saved you can use an NV CR like CR2
no ? You need to restore those anyway...
Also same comments as on the way down vs barriers when doing
lock/unlock.
> + 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;
> -
> /* 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
> +
> 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)
> + 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
> +
> OPAL_CALL(opal_invalid_call, OPAL_INVALID_CALL);
> OPAL_CALL(opal_console_write, OPAL_CONSOLE_WRITE);
> OPAL_CALL(opal_console_read, OPAL_CONSOLE_READ);
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index 34c6665..17fb98c 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -36,6 +36,8 @@
> #include <asm/opal.h>
> #include <asm/kexec.h>
> #include <asm/smp.h>
> +#include <asm/cputhreads.h>
> +#include <asm/cpuidle.h>
>
> #include "powernv.h"
>
> @@ -292,11 +294,45 @@ static void __init pnv_setup_machdep_rtas(void)
>
> static u32 supported_cpuidle_states;
>
> +static void pnv_alloc_idle_core_states(void)
> +{
> + int i, j;
> + int nr_cores = cpu_nr_cores();
> + u32 *core_idle_state;
> +
> + /*
> + * core_idle_state - First 8 bits track the idle state of each thread
> + * of the core. The 8th bit is the lock bit. Initially all thread bits
> + * are set. They are cleared when the thread enters deep idle state
> + * like sleep and winkle. Initially the lock bit is cleared.
> + * The lock bit has 2 purposes
> + * a. While the first thread is restoring core state, it prevents
> + * from other threads in the core from switching to prcoess context.
> + * b. While the last thread in the core is saving the core state, it
> + * prevent a different thread from waking up.
> + */
> + for (i = 0; i < nr_cores; i++) {
> + int first_cpu = i * threads_per_core;
> + int node = cpu_to_node(first_cpu);
> +
> + core_idle_state = kmalloc_node(sizeof(u32), GFP_KERNEL, node);
> + for (j = 0; j < threads_per_core; j++) {
> + int cpu = first_cpu + j;
> +
> + paca[cpu].core_idle_state_ptr = core_idle_state;
> + paca[cpu].thread_idle_state = PNV_THREAD_RUNNING;
> +
> + }
> + }
> +}
> +
> u32 pnv_get_supported_cpuidle_states(void)
> {
> return supported_cpuidle_states;
> }
> +EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
>
> +u8 pnv_need_fastsleep_workaround;
> static int __init pnv_init_idle_states(void)
> {
> struct device_node *power_mgt;
> @@ -306,6 +342,7 @@ static int __init pnv_init_idle_states(void)
> int i;
>
> supported_cpuidle_states = 0;
> + pnv_need_fastsleep_workaround = 0;
>
> if (cpuidle_disable != IDLE_NO_OVERRIDE)
> return 0;
> @@ -332,13 +369,14 @@ static int __init pnv_init_idle_states(void)
> flags = be32_to_cpu(idle_state_flags[i]);
> supported_cpuidle_states |= flags;
> }
> -
> + if (supported_cpuidle_states & OPAL_PM_SLEEP_ENABLED_ER1)
> + pnv_need_fastsleep_workaround = 1;
> + pnv_alloc_idle_core_states();
> return 0;
> }
>
> subsys_initcall(pnv_init_idle_states);
>
> -
> static int __init pnv_probe(void)
> {
> unsigned long root = of_get_flat_dt_root();
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index 3dc4cec..12b761a 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -167,7 +167,8 @@ 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_SLEEP_ENABLED) ||
> + (idle_states & OPAL_PM_SLEEP_ENABLED_ER1))
> power7_sleep();
> else
> power7_nap(1);
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 0a7d827..a489b56 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -208,7 +208,8 @@ static int powernv_add_idle_states(void)
> nr_idle_states++;
> }
>
> - if (flags & OPAL_PM_SLEEP_ENABLED) {
> + if (flags & OPAL_PM_SLEEP_ENABLED ||
> + flags & OPAL_PM_SLEEP_ENABLED_ER1) {
> /* Add FASTSLEEP state */
> strcpy(powernv_states[nr_idle_states].name, "FastSleep");
> strcpy(powernv_states[nr_idle_states].desc, "FastSleep");
^ permalink raw reply
* Re: [PATCH v2 3/6] pseries: Create new device hotplug entry point
From: Benjamin Herrenschmidt @ 2014-11-26 23:44 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <546A6DEE.8080708@linux.vnet.ibm.com>
On Mon, 2014-11-17 at 15:51 -0600, Nathan Fontenot wrote:
>
> For PowerVM systems, this patch creates the /sys/kernel/dlpar file that rtas
> hotplug events can be written to by drmgr and passed to the common entry point.
> There is no chance of updating how we receive hotplug requests on PowerVM
> systems.
I'm not convinced this is the right place for that file, might be worth asking
Greg KH what he thinks here.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH REPOST 3/3] powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
From: Benjamin Herrenschmidt @ 2014-11-26 23:39 UTC (permalink / raw)
To: Greg Kurz; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20141117174244.7717.83028.stgit@bahia.local>
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 ...
>
> #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 ?
> + 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 (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: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Segher Boessenkool @ 2014-11-26 23:28 UTC (permalink / raw)
To: Michael Ellerman; +Cc: paulus, linuxppc-dev, Anton Blanchard
In-Reply-To: <20141126232318.GA6567@gate.crashing.org>
On Wed, Nov 26, 2014 at 05:23:18PM -0600, Segher Boessenkool wrote:
> GCC has added -many to the assembler flags for over ten years now, so
> no that will not work. You can use -mppc or similar with the assembler
> if you invoke it correctly (use $(CC) -print-prog-name=as to figure
s/correctly/directly/
> out how to call the assembler, if you want to stay sane and use the
> same one as the rest of the toolchain you use).
>
>
> Segher
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Segher Boessenkool @ 2014-11-26 23:23 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, paulus, Anton Blanchard
In-Reply-To: <1417041497.12952.2.camel@concordia>
On Thu, Nov 27, 2014 at 09:38:17AM +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?
>
> Could we catch this by forcing -m32 in the CFLAGS for vdso32 ?
GCC has added -many to the assembler flags for over ten years now, so
no that will not work. You can use -mppc or similar with the assembler
if you invoke it correctly (use $(CC) -print-prog-name=as to figure
out how to call the assembler, if you want to stay sane and use the
same one as the rest of the toolchain you use).
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Michael Ellerman @ 2014-11-26 22:38 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
In-Reply-To: <1417036288-22079-1-git-send-email-anton@samba.org>
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?
Could we catch this by forcing -m32 in the CFLAGS for vdso32 ?
cheers
^ permalink raw reply
* Re: powerpc/powernv: Fix the hmi event version check.
From: Michael Ellerman @ 2014-11-26 22:33 UTC (permalink / raw)
To: Mahesh Jagannath Salgaonkar; +Cc: linuxppc-dev
In-Reply-To: <5475AAC8.7040105@linux.vnet.ibm.com>
On Wed, 2014-11-26 at 15:56 +0530, Mahesh Jagannath Salgaonkar wrote:
> On 11/26/2014 09:14 AM, Michael Ellerman wrote:
> > On Thu, 2014-20-11 at 04:14:36 UTC, Mahesh Salgaonkar wrote:
> >> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> >>
> >> The current HMI event structure is an ABI and carries a version field to
> >> accommodate future changes without affecting/rearranging current structure
> >> members that are valid for previous versions. The current version check
> >> "if (hmi_evt->version != OpalHMIEvt_V1)" seems to consider that version
> >> will always be V1 which may not be true in future. If we start supporting
> >> HMI event > V1, this check would fail without printing anything on older
> >> kernels. This patch fixes this issue.
> >
> > It's not clear what you mean when you say "this check would fail without
> > printing anything". The check will fail, and it will print something, ie. the
> > error message.
> >
> > What you mean is "the check will fail, and the HMI info will not be printed".
>
> My Bad, Yes. I meant 'HMI info will not be printed'. Do you want me to
> re spin the patch with correction.
No that's fine, I've fixed it up to be:
The current HMI event structure is an ABI and carries a version field to
accommodate future changes without affecting/rearranging current structure
members that are valid for previous versions.
The current version check "if (hmi_evt->version != OpalHMIEvt_V1)"
doesn't accomodate the fact that the version number may change in
future.
If firmware starts returning an HMI event with version > 1, this check
will fail and no HMI information will be printed on older kernels.
This patch fixes this issue.
> > I'll CC this to stable unless you disagree.
>
> Yes. This patch needs to go to stable.
Yep I've added Cc stable to the patch.
cheers
^ permalink raw reply
* Re: [RFC PATCH v1 1/1] powerpc/85xx: Add support for Emerson/Artesyn MVME2500.
From: Scott Wood @ 2014-11-26 22:21 UTC (permalink / raw)
To: Alessio Igor Bogani; +Cc: linuxppc-dev
In-Reply-To: <1417011468-13485-1-git-send-email-alessio.bogani@elettra.eu>
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.
> + ranges = <0x0 0 0xffe00000 0x100000>;
> +
> + i2c@3000 {
> + hwmon@4c {
> + compatible = "adi,adt7461";
> + reg = <0x4c>;
> + };
> +
> + rtc@68 {
> + compatible = "dallas,ds1337";
> + reg = <0x68>;
> + interrupts = <8 1 0 0>;
> + };
> +
> + 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.
> +
> + eeprom@52 {
> + compatible = "atmel,24c512";
> + reg = <0x52>;
> + };
> +
> + eeprom@53 {
> + compatible = "atmel,24c512";
> + reg = <0x53>;
> + };
> +
> + spd@50 {
> + compatible = "atmel,24c02";
> + reg = <0x50>;
> + };
Likewise, I suspect this is also an eeprom.
> + };
> +
> + spi0: spi@7000 {
> + fsl,espi-num-chipselects = <2>;
> +
> + flash@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "atmel,at25df641";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> + 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.
> + lbc: localbus@ffe05000 {
> + reg = <0 0xffe05000 0 0x1000>;
> +
> + ranges = <0x0 0x0 0x0 0xfff00000 0x00080000
> + 0x1 0x0 0x0 0xffc40000 0x00010000
> + 0x2 0x0 0x0 0xffc50000 0x00010000
> + 0x3 0x0 0x0 0xffc60000 0x00010000
> + 0x4 0x0 0x0 0xffc70000 0x00010000
> + 0x6 0x0 0x0 0xffc80000 0x00010000
> + 0x5 0x0 0x0 0xffdf0000 0x00001000>;
It's not possible to program the LBC with a window of only 0x1000 bytes.
> +
> + serial2: serial@1,0 {
> + #cell-index = <2>;
> + device_type = "serial";
> + compatible = "ns16550";
> + reg = <0x1 0x0 0x100>;
> + clock-frequency = <1843200>;
> + interrupts = <11 2 0 0>;
> + };
> +
> + serial3: serial@2,0 {
> + #cell-index = <3>;
> + device_type = "serial";
> + compatible = "ns16550";
> + reg = <0x2 0x0 0x100>;
> + clock-frequency = <1843200>;
> + interrupts = <1 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?
> + interrupts = <9 1 0 0 >;
Whitespace
> +/include/ "mvme2500.dtsi"
Are you going to have more than one .dts using this .dtsi? If not, why
separate this part?
> 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?
> 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...
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.
> +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.
> +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?
> +CONFIG_STAGING=y
What do you need from staging?
> 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?
> +#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.
> +#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()
> +}
> +
> +machine_arch_initcall(mvme2500, mpc85xx_common_publish_devices);
> +machine_arch_initcall(mvme2500, swiotlb_setup_bus_notifier);
> +
> +/*
> + * Called very early, device-tree isn't unflattened
> + */
> +static int __init mvme2500_probe(void)
> +{
> + unsigned long root = of_get_flat_dt_root();
> +
> + 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.
-Scott
^ permalink raw reply
* [PATCH] powerpc: 32 bit getcpu VDSO function uses 64 bit instructions
From: Anton Blanchard @ 2014-11-26 21:11 UTC (permalink / raw)
To: benh, paulus, mpe; +Cc: linuxppc-dev
I used some 64 bit instructions when adding the 32 bit getcpu VDSO
function. Fix it.
Fixes: 18ad51dd342a ("powerpc: Add VDSO version of getcpu")
Cc: stable@vger.kernel.org
Signed-off-by: Anton Blanchard <anton@samba.org>
---
arch/powerpc/kernel/vdso32/getcpu.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/vdso32/getcpu.S b/arch/powerpc/kernel/vdso32/getcpu.S
index 23eb9a9..c62be60 100644
--- a/arch/powerpc/kernel/vdso32/getcpu.S
+++ b/arch/powerpc/kernel/vdso32/getcpu.S
@@ -30,8 +30,8 @@
V_FUNCTION_BEGIN(__kernel_getcpu)
.cfi_startproc
mfspr r5,SPRN_SPRG_VDSO_READ
- cmpdi cr0,r3,0
- cmpdi cr1,r4,0
+ cmpwi cr0,r3,0
+ cmpwi cr1,r4,0
clrlwi r6,r5,16
rlwinm r7,r5,16,31-15,31-0
beq cr0,1f
--
2.1.0
^ permalink raw reply related
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 17:21 UTC (permalink / raw)
To: Christian Borntraeger
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
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.
Specifically maybe DEBUG_ATOMIC_SLEEP should select PREEMPT_COUNT?
> As a result, userspace access will fail and caller will
> get a nice error.
>
>
>
> > But coming up with wrong statements is not helpful.
>
> True. Sorry that I did that.
>
> > >
> > > The point is that *_user is safe with preempt off.
> > > It returns an error gracefully.
> > > It does not sleep.
> > > It does not trigger the scheduler in that context.
> >
> > There are special cases where your statement is true. But its not in general.
> > copy_to_user might fault and that fault might sleep and reschedule.
>
> Yes. But not if called inatomic.
>
>
>
> > For example handle_mm_fault might go down to pud_alloc, pmd_alloc etc and all these functions could do an GFP_KERNEL allocation. Which might sleep. Which will schedule.
> >
> >
> > >
> > >
> > > David's patch makes it say it does, so it's wrong.
> > >
> > >
> > >
>
> Absolutely.
> I think you can already debug your case easily, by enabling CONFIG_PREEMPT.
> This seems counter-intuitive, and distro debug kernels don't seem to do this.
>
> --
> MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 17:04 UTC (permalink / raw)
To: Christian Borntraeger
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <547604FC.4030300@de.ibm.com>
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.
> But coming up with wrong statements is not helpful.
True. Sorry that I did that.
> >
> > The point is that *_user is safe with preempt off.
> > It returns an error gracefully.
> > It does not sleep.
> > It does not trigger the scheduler in that context.
>
> There are special cases where your statement is true. But its not in general.
> copy_to_user might fault and that fault might sleep and reschedule.
Yes. But not if called inatomic.
> For example handle_mm_fault might go down to pud_alloc, pmd_alloc etc and all these functions could do an GFP_KERNEL allocation. Which might sleep. Which will schedule.
>
>
> >
> >
> > David's patch makes it say it does, so it's wrong.
> >
> >
> >
Absolutely.
I think you can already debug your case easily, by enabling CONFIG_PREEMPT.
This seems counter-intuitive, and distro debug kernels don't seem to do this.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 16:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126163216.GB10850@redhat.com>
Am 26.11.2014 um 17:32 schrieb Michael S. Tsirkin:
[...]
>>>> This is what happened on our side (very recent kernel):
>>>>
>>>> spin_lock(&lock)
>>>> copy_to_user(...)
>>>> spin_unlock(&lock)
>>>
>>> That's a deadlock even without copy_to_user - it's
>>> enough for the thread to be preempted and another one
>>> to try taking the lock.
>>
>> Huh? With CONFIG_PREEMPT spin_lock will disable preemption. (we had preempt = server anyway).
>
> Are you sure? Can you point me where it does this please?
spin_lock --> raw_spin_lock --> _raw_spin_lock --> __raw_spin_lock
static inline void __raw_spin_lock(raw_spinlock_t *lock)
{
----> preempt_disable(); <-----
spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
}
Michael, please be serious. The whole kernel would be broken if spin_lock would not disable preemption.
>
>> But please: One step back. The problem is not the good path. The problem is that we lost a debugging aid for a known to be broken case. In other words: Our code had a bug. Older kernels detected that kind of bug. With your change we no longer saw the sleeping while atomic. Thats it. See my other mail.
>>
>> Christian
>
> You want to add more debugging tools, fine.
We dont want to add, we want to fix something that used to work
> But this one was > giving users in field false positives.
So lets try to fix those, ok? If we cant, then tough luck. But coming up with wrong statements is not helpful.
>
> The point is that *_user is safe with preempt off.
> It returns an error gracefully.
> It does not sleep.
> It does not trigger the scheduler in that context.
There are special cases where your statement is true. But its not in general.
copy_to_user might fault and that fault might sleep and reschedule. For example handle_mm_fault might go down to pud_alloc, pmd_alloc etc and all these functions could do an GFP_KERNEL allocation. Which might sleep. Which will schedule.
>
>
> David's patch makes it say it does, so it's wrong.
>
>
>
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 16:50 UTC (permalink / raw)
To: Christian Borntraeger
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <5476002B.30900@de.ibm.com>
On Wed, Nov 26, 2014 at 05:30:35PM +0100, Christian Borntraeger wrote:
> Am 26.11.2014 um 17:19 schrieb Michael S. Tsirkin:
> > On Wed, Nov 26, 2014 at 05:02:23PM +0100, David Hildenbrand wrote:
> >>>> This is what happened on our side (very recent kernel):
> >>>>
> >>>> spin_lock(&lock)
> >>>> copy_to_user(...)
> >>>> spin_unlock(&lock)
> >>>
> >>> That's a deadlock even without copy_to_user - it's
> >>> enough for the thread to be preempted and another one
> >>> to try taking the lock.
> >>>
> >>>
> >>>> 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
> >>>> as "old value"
> >>>> 2. we slept during copy_to_user()
> >>>> 3. the thread got scheduled onto another cpu
> >>>> 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
> >>>> the spinlock tried to unlocked it).
> >>>> 5. lock remained locked -> deadlock
> >>>>
> >>>> Christian came up with the following explanation:
> >>>> Without preemption, spin_lock() will not touch the preempt counter.
> >>>> disable_pfault() will always touch it.
> >>>>
> >>>> Therefore, with preemption disabled, copy_to_user() has no idea that it is
> >>>> running in atomic context - and will therefore try to sleep.
> >>>>
> >>>> So copy_to_user() will on s390:
> >>>> 1. run "as atomic" while spin_lock() with preemption enabled.
> >>>> 2. run "as not atomic" while spin_lock() with preemption disabled.
> >>>> 3. run "as atomic" while pagefault_disabled() with preemption enabled or
> >>>> disabled.
> >>>> 4. run "as not atomic" when really not atomic.
> >>
> >> should have been more clear at that point:
> >> preemption enabled == kernel compiled with preemption support
> >> preemption disabled == kernel compiled without preemption support
> >>
> >>>>
> >>>> And exactly nr 2. is the thing that produced the deadlock in our scenario and
> >>>> the reason why I want a might_sleep() :)
> >>>
> >>> IMHO it's not copy to user that causes the problem.
> >>> It's the misuse of spinlocks with preemption on.
> >>
> >> As I said, preemption was off.
> >
> > off -> disabled at compile time?
> >
> > But the code is broken for people that do enable it.
> [...]
> > You should normally disable preemption if you take
> > spinlocks.
>
> Your are telling that any sequence of
> spin_lock
> ...
> spin_unlock
>
> is broken with CONFIG_PREEMPT?
> Michael, that is bullshit. spin_lock will take care of CONFIG_PREEMPT just fine.
>
> Only sequences like
> spin_lock
> ...
> schedule
> ...
> spin_unlock
> are broken.
>
> But as I said. That is not the problem that we are discussing here.
>
> Christian
I'm saying spin_lock without _irqsave is often a bug.
I am also saying this code in mm/fault.c:
__do_page_fault
...
/*
* If we're in an interrupt, have no user context or are running
* in an atomic region then we must not take the fault:
*/
if (unlikely(in_atomic() || !mm)) {
bad_area_nosemaphore(regs, error_code, address);
return;
}
means that a fault won't cause sleep if called in atomic context.
And a bunch of code relies on this.
This is why might_fault does:
* 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 you see this violated, let's figure out why.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 16:32 UTC (permalink / raw)
To: Christian Borntraeger
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <5475FAB1.1000802@de.ibm.com>
On Wed, Nov 26, 2014 at 05:07:13PM +0100, Christian Borntraeger wrote:
> Am 26.11.2014 um 16:47 schrieb Michael S. Tsirkin:
> > On Wed, Nov 26, 2014 at 04:32:07PM +0100, David Hildenbrand wrote:
> >>> On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
> >>>> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> >>>>>> What's the path you are trying to debug?
> >>>>>
> >>>>> Well, we had a problem where we held a spin_lock and called
> >>>>> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> >>>>> almost a week to debug. The simple might_sleep() check would have showed this
> >>>>> error immediately.
> >>>>
> >>>> This must have been a very old kernel.
> >>>> A modern kernel will return an error from copy_to_user.
> >>>> Which is really the point of the patch you are trying to revert.
> >>>
> >>> That's assuming you disabled preemption. If you didn't, and take
> >>> a spinlock, you have deadlocks even without userspace access.
> >>>
> >>
> >> (Thanks for your resent, my first email was sent directly to you ... grml)
> >>
> >> This is what happened on our side (very recent kernel):
> >>
> >> spin_lock(&lock)
> >> copy_to_user(...)
> >> spin_unlock(&lock)
> >
> > That's a deadlock even without copy_to_user - it's
> > enough for the thread to be preempted and another one
> > to try taking the lock.
>
> Huh? With CONFIG_PREEMPT spin_lock will disable preemption. (we had preempt = server anyway).
Are you sure? Can you point me where it does this please?
> But please: One step back. The problem is not the good path. The problem is that we lost a debugging aid for a known to be broken case. In other words: Our code had a bug. Older kernels detected that kind of bug. With your change we no longer saw the sleeping while atomic. Thats it. See my other mail.
>
> Christian
You want to add more debugging tools, fine. But this one was
giving users in field false positives.
The point is that *_user is safe with preempt off.
It returns an error gracefully.
It does not sleep.
It does not trigger the scheduler in that context.
David's patch makes it say it does, so it's wrong.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 16:30 UTC (permalink / raw)
To: Michael S. Tsirkin, David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, paulus, schwidefsky,
akpm, linuxppc-dev, mingo
In-Reply-To: <20141126161947.GA10850@redhat.com>
Am 26.11.2014 um 17:19 schrieb Michael S. Tsirkin:
> On Wed, Nov 26, 2014 at 05:02:23PM +0100, David Hildenbrand wrote:
>>>> This is what happened on our side (very recent kernel):
>>>>
>>>> spin_lock(&lock)
>>>> copy_to_user(...)
>>>> spin_unlock(&lock)
>>>
>>> That's a deadlock even without copy_to_user - it's
>>> enough for the thread to be preempted and another one
>>> to try taking the lock.
>>>
>>>
>>>> 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
>>>> as "old value"
>>>> 2. we slept during copy_to_user()
>>>> 3. the thread got scheduled onto another cpu
>>>> 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
>>>> the spinlock tried to unlocked it).
>>>> 5. lock remained locked -> deadlock
>>>>
>>>> Christian came up with the following explanation:
>>>> Without preemption, spin_lock() will not touch the preempt counter.
>>>> disable_pfault() will always touch it.
>>>>
>>>> Therefore, with preemption disabled, copy_to_user() has no idea that it is
>>>> running in atomic context - and will therefore try to sleep.
>>>>
>>>> So copy_to_user() will on s390:
>>>> 1. run "as atomic" while spin_lock() with preemption enabled.
>>>> 2. run "as not atomic" while spin_lock() with preemption disabled.
>>>> 3. run "as atomic" while pagefault_disabled() with preemption enabled or
>>>> disabled.
>>>> 4. run "as not atomic" when really not atomic.
>>
>> should have been more clear at that point:
>> preemption enabled == kernel compiled with preemption support
>> preemption disabled == kernel compiled without preemption support
>>
>>>>
>>>> And exactly nr 2. is the thing that produced the deadlock in our scenario and
>>>> the reason why I want a might_sleep() :)
>>>
>>> IMHO it's not copy to user that causes the problem.
>>> It's the misuse of spinlocks with preemption on.
>>
>> As I said, preemption was off.
>
> off -> disabled at compile time?
>
> But the code is broken for people that do enable it.
[...]
> You should normally disable preemption if you take
> spinlocks.
Your are telling that any sequence of
spin_lock
...
spin_unlock
is broken with CONFIG_PREEMPT?
Michael, that is bullshit. spin_lock will take care of CONFIG_PREEMPT just fine.
Only sequences like
spin_lock
...
schedule
...
spin_unlock
are broken.
But as I said. That is not the problem that we are discussing here.
Christian
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 16:19 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126170223.3b108b94@thinkpad-w530>
On Wed, Nov 26, 2014 at 05:02:23PM +0100, David Hildenbrand wrote:
> > > This is what happened on our side (very recent kernel):
> > >
> > > spin_lock(&lock)
> > > copy_to_user(...)
> > > spin_unlock(&lock)
> >
> > That's a deadlock even without copy_to_user - it's
> > enough for the thread to be preempted and another one
> > to try taking the lock.
> >
> >
> > > 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
> > > as "old value"
> > > 2. we slept during copy_to_user()
> > > 3. the thread got scheduled onto another cpu
> > > 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
> > > the spinlock tried to unlocked it).
> > > 5. lock remained locked -> deadlock
> > >
> > > Christian came up with the following explanation:
> > > Without preemption, spin_lock() will not touch the preempt counter.
> > > disable_pfault() will always touch it.
> > >
> > > Therefore, with preemption disabled, copy_to_user() has no idea that it is
> > > running in atomic context - and will therefore try to sleep.
> > >
> > > So copy_to_user() will on s390:
> > > 1. run "as atomic" while spin_lock() with preemption enabled.
> > > 2. run "as not atomic" while spin_lock() with preemption disabled.
> > > 3. run "as atomic" while pagefault_disabled() with preemption enabled or
> > > disabled.
> > > 4. run "as not atomic" when really not atomic.
>
> should have been more clear at that point:
> preemption enabled == kernel compiled with preemption support
> preemption disabled == kernel compiled without preemption support
>
> > >
> > > And exactly nr 2. is the thing that produced the deadlock in our scenario and
> > > the reason why I want a might_sleep() :)
> >
> > IMHO it's not copy to user that causes the problem.
> > It's the misuse of spinlocks with preemption on.
>
> As I said, preemption was off.
off -> disabled at compile time?
But the code is broken for people that do enable it.
> >
> > So might_sleep would make you think copy_to_user is
> > the problem, and e.g. let you paper over it by
> > moving copy_to_user out.
>
> Actually implementing different way of locking easily fixed the problem for us.
> The old might_sleep() checks would have given us the problem within a few
> seconds (I tested it).
Or enable CONFIG_PREMPT, with same effect (copy_to_user will report
an error).
Do you check return code from copy to user?
If not then you have another bug ...
> >
> > Enable lock prover and you will see what the real
> > issue is, which is you didn't disable preempt.
> > and if you did, copy_to_user would be okay.
> >
>
> Our kernel is compiled without preemption and we turned on all lock/atomic
> sleep debugging aid. No problem was detected.
But your code is still buggy with preemption on, isn't it?
> ----
> But the question is if we shouldn't rather provide a:
>
> copy_to_user_nosleep() implementation that can be called from
> pagefault_disable() because it won't sleep.
> and a
> copy_to_user_sleep() implementation that cannot be called from
> pagefault_disable().
>
> Another way to fix it would be a reworked pagefault_disable() function that
> somehow sets "a flag", so copy_to_user() knows that it is in fact called from a
> valid context, not just from "some atomic" context. So we could trigger
> might_sleep() when detecting a !pagefault_disable contex
I think all this is just directing people to paper over the
problem. You should normally disable preemption if you take
spinlocks.
Yes it might happen to work if preempt is compiled out
and you don't trigger scheduler, but Linux might
add scheduler calls at any point without notice,
code must be preempt safe.
Maybe add a debug option warning about spinlocks taken
with preempt on.
That would make sense I think.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 16:07 UTC (permalink / raw)
To: Michael S. Tsirkin, David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, paulus, schwidefsky,
akpm, linuxppc-dev, mingo
In-Reply-To: <20141126154717.GB10568@redhat.com>
Am 26.11.2014 um 16:47 schrieb Michael S. Tsirkin:
> On Wed, Nov 26, 2014 at 04:32:07PM +0100, David Hildenbrand wrote:
>>> On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
>>>>>> What's the path you are trying to debug?
>>>>>
>>>>> Well, we had a problem where we held a spin_lock and called
>>>>> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
>>>>> almost a week to debug. The simple might_sleep() check would have showed this
>>>>> error immediately.
>>>>
>>>> This must have been a very old kernel.
>>>> A modern kernel will return an error from copy_to_user.
>>>> Which is really the point of the patch you are trying to revert.
>>>
>>> That's assuming you disabled preemption. If you didn't, and take
>>> a spinlock, you have deadlocks even without userspace access.
>>>
>>
>> (Thanks for your resent, my first email was sent directly to you ... grml)
>>
>> This is what happened on our side (very recent kernel):
>>
>> spin_lock(&lock)
>> copy_to_user(...)
>> spin_unlock(&lock)
>
> That's a deadlock even without copy_to_user - it's
> enough for the thread to be preempted and another one
> to try taking the lock.
Huh? With CONFIG_PREEMPT spin_lock will disable preemption. (we had preempt = server anyway).
But please: One step back. The problem is not the good path. The problem is that we lost a debugging aid for a known to be broken case. In other words: Our code had a bug. Older kernels detected that kind of bug. With your change we no longer saw the sleeping while atomic. Thats it. See my other mail.
Christian
^ 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