Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

From: Michael Ellerman
Date: Wed Jul 19 2017 - 08:07:16 EST


Nicholas Piggin <npiggin@xxxxxxxxx> writes:

> On Wed, 19 Jul 2017 13:48:49 +0530
> "Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx> wrote:
>
>> From: "Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx>
>>
>> The stop4 idle state on POWER9 is a deep idle state which loses
>> hypervisor resources, but whose latency is low enough that it can be
>> exposed via cpuidle.
>>
>> Until now, the deep idle states which lose hypervisor resources (eg:
>> winkle) were only exposed via CPU-Hotplug. Hence currently on wakeup
>> from such states, barring a few SPRs which need to be restored to
>> their older value, rest of the SPRS are reinitialized to their values
>> corresponding to that at boot time.
>>
>> When stop4 is used in the context of cpuidle, we want these additional
>> SPRs to be restored to their older value, to ensure that the context
>> on the CPU coming back from idle is same as it was before going idle.
>>
>> In this patch, we define a SPR save area in PACA (since we have used
>> up the volatile register space in the stack) and on POWER9, we restore
>> SPRN_PID, SPRN_LDBAR, SPRN_FSCR, SPRN_HFSCR, SPRN_MMCRA, SPRN_MMCR1,
>> SPRN_MMCR2 to the values they had before entering stop.
>>
>> Signed-off-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>
>> ---
>> arch/powerpc/include/asm/paca.h | 7 ++++++
>> arch/powerpc/kernel/asm-offsets.c | 12 ++++++++++
>> arch/powerpc/kernel/idle_book3s.S | 46 +++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
>> index dc88a31..a6b9ea6 100644
>> --- a/arch/powerpc/include/asm/paca.h
>> +++ b/arch/powerpc/include/asm/paca.h
>> @@ -48,6 +48,7 @@
>> #define get_lppaca() (get_paca()->lppaca_ptr)
>> #define get_slb_shadow() (get_paca()->slb_shadow_ptr)
>>
>> +#define MAX_STOP_SPRS 7
>> struct task_struct;
>>
>> /*
>> @@ -183,6 +184,12 @@ struct paca_struct {
>> struct paca_struct **thread_sibling_pacas;
>> /* The PSSCR value that the kernel requested before going to stop */
>> u64 requested_psscr;
>> +
>> + /*
>> + * Save area for additional SPRs that need to be
>> + * saved/restored during cpuidle stop.
>> + */
>> + u64 stop_spr_save_area[MAX_STOP_SPRS];
>> #endif
>>
>> #ifdef CONFIG_PPC_STD_MMU_64
>> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
>> index a7b5af3..0262283 100644
>> --- a/arch/powerpc/kernel/asm-offsets.c
>> +++ b/arch/powerpc/kernel/asm-offsets.c
>> @@ -743,6 +743,18 @@ int main(void)
>> OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
>> OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
>> OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
>> +
>> + OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
>> + OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
>> + OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
>> + OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
>> +
>> + /* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
>> + OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
>> + OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
>> + OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);
>
> Don't these offset names go against convention?
>
> Look at e.g., how PACA_EXGEN is used. I would prefer using that
> convention. You could make the name slightly shorter too, e.g.,
> just stop_sprs or so.

Yes please.

If I see PACA_MMCRA I'm expecting that's paca->mmcra.

Also if the same values always go in the same place then please use a
proper struct, rather than an array. ie.

struct stop_sprs
{
u64 pid;
u64 ldbar;
...
}

cheers