linux-kernel - Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1466489657.25838.39.camel@neuling.org>
Date:	Tue, 21 Jun 2016 16:14:17 +1000
From:	Michael Neuling <mikey@...ling.org>
To:	Shreyas B Prabhu <shreyas@...ux.vnet.ibm.com>,
	Michael Ellerman <mpe@...erman.id.au>
Cc:	ego@...ux.vnet.ibm.com, benh@....ibm.com,
	linux-kernel@...r.kernel.org, maddy@...ux.vnet.ibm.com,
	linuxppc-dev@...ts.ozlabs.org,
	Aneesh Kk Veetil <aneesh.kumar@...ibm.com>
Subject: Re: [v6, 08/11] powerpc/powernv: Add platform support for stop
 instruction


> > > +#define OPAL_PM_TIMEBASE_STOP		0x00000002
> > > +#define OPAL_PM_LOSE_HYP_CONTEXT	0x00002000
> > > +#define OPAL_PM_LOSE_FULL_CONTEXT	0x00004000
> > >  #define OPAL_PM_NAP_ENABLED		0x00010000
> > >  #define OPAL_PM_SLEEP_ENABLED		0x00020000
> > >  #define OPAL_PM_WINKLE_ENABLED		0x00040000
> > >  #define OPAL_PM_SLEEP_ENABLED_ER1	0x00080000 /* with
> > > workaround */
> > > +#define OPAL_PM_STOP_INST_FAST		0x00100000
> > > +#define OPAL_PM_STOP_INST_DEEP		0x00200000
> > I don't see the above in skiboot yet?
> I've posted it here -
> http://patchwork.ozlabs.org/patch/617828/

FWIW, this is in now.

https://github.com/open-power/skiboot/commit/952daa69baca407383bc900911f6c40718a0e289

> > 
> > 
> > > 
> > > diff --git a/arch/powerpc/include/asm/paca.h
> > > b/arch/powerpc/include/asm/paca.h
> > > index 546540b..ae91b44 100644
> > > --- a/arch/powerpc/include/asm/paca.h
> > > +++ b/arch/powerpc/include/asm/paca.h
> > > @@ -171,6 +171,8 @@ struct paca_struct {
> > >  	/* Mask to denote subcore sibling threads */
> > >  	u8 subcore_sibling_mask;
> > >  #endif
> > > +	/* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set
> > > */
> > > +	u64 thread_psscr;
> > I'm not entirely clear on why that needs to be in the paca. Could it
> > not be global?
> > 
> While we use Requested Level (RL) field of PSSCR to request a stop
> level, other fields in the SPR like EC, ESL, TR, PSLL, MTL can be
> modified by individual threads less frequently to alter the behaviour of
> stop. So the idea was to have a per-thread variable with all (except RL)
> fields of PSSCR set appropriately. Threads at the time of entering idle,
> can modify the RL field in the variable and execute stop instruction.

But we don't do any of this currently? This is setup at init
in pnv_init_idle_states() and only the RL is changed in power_stop().

So it can still be a global.  It could just be a constant currently even.

>  	.text
> > >  
> > >  /*
> > > @@ -61,8 +75,19 @@ save_sprs_to_stack:
> > >  	 * 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
> > >  	 */
> > > +BEGIN_FTR_SECTION
> > > +	mfspr	r3,SPRN_PTCR
> > > +	std	r3,_PTCR(r1)
> > > +	mfspr	r3,SPRN_LMRR
> > > +	std	r3,_LMRR(r1)
> > > +	mfspr	r3,SPRN_LMSER
> > > +	std	r3,_LMSER(r1)
> > > +	mfspr	r3,SPRN_ASDR
> > > +	std	r3,_ASDR(r1)
> > > +FTR_SECTION_ELSE
> > A comment here saying that SDR1 is removed in ISA 3.0 would be helpful.
> > 
> Ok.

I thought we decided we didn't need LMRR, LMSR, 

https://lkml.org/lkml/2016/6/8/1121

or ASDR isn't actually used at all yet and is only valid for some page
faults, so we don't need it here also.

> +END_MMU_FTR_SECTION_IFCLR(MMU_FTR_RADIX)
> > > +
> > > +	/* Restore per thread state */
> > > +BEGIN_FTR_SECTION
> > > +	bl      __restore_cpu_power9
> > > +
> > > +	ld	r4,_LMRR(r1)
> > > +	mtspr	SPRN_LMRR,r4
> > > +	ld	r4,_LMSER(r1)
> > > +	mtspr	SPRN_LMSER,r4
> > > +	ld	r4,_ASDR(r1)
> > > +	mtspr	SPRN_ASDR,r4
> > Should those be in __restore_cpu_power9 ?
> I was not sure how these registers will be used, but after speaking to
> Aneesh and Mikey I realized these registers will not need restoring.
> LMRR and LMSER are associated with the context and ADSR will be consumed
> before entering stop. So I'll be dropping the this hunk in next revision.

Yep.

> 
> > >  	pnv_alloc_idle_core_states();
> > >  
> > > +	if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
> > > +		for_each_possible_cpu(i) {
> > > +
> > > +			u64 psscr_init_val = PSSCR_ESL | PSSCR_EC |
> > > +					PSSCR_PSLL_MASK | PSSCR_TR_MASK |
> > > +					PSSCR_MTL_MASK;
> > > +
> > > +			paca[i].thread_psscr = psscr_init_val;

This seems to be the only place you set this.  Why put it in the paca, why
not just make this a constant? 

Mikey