[PATCH 0/3] powerpc/powernv: Cpuidle related cleanup

[PATCH 0/3] powerpc/powernv: Cpuidle related cleanup

[Shreyas B. Prabhu]

These patches are purely code movement and cleanup.
There is no functionality change.

Note, there are multiple style error reported for patch 1 and 2.
I think this is because checkpatch script mistakes the patches
as C code because of the presence of semi-colon in each line. I
for now have largely ignored these errors.

Shreyas B. Prabhu (3):
  powerpc/powernv: Move CHECK_HMI_INTERRUPT to exception-64s header
  powerpc/powernv: Encapsulate idle preparation steps in a macro
  powerpc/powernv: make hypervisor state restore a function

 arch/powerpc/include/asm/cpuidle.h       |  68 ++++++++++++++
 arch/powerpc/include/asm/exception-64s.h |  19 ++++
 arch/powerpc/kernel/exceptions-64s.S     |  29 +-----
 arch/powerpc/kernel/idle_power7.S        | 148 ++++++++-----------------------
 4 files changed, 129 insertions(+), 135 deletions(-)

-- 
1.9.3

[PATCH 1/3] powerpc/powernv: Move CHECK_HMI_INTERRUPT to exception-64s header

[Shreyas B. Prabhu]

CHECK_HMI_INTERRUPT is used to check for HMI's in reset vector. Move
the macro to a common location (exception-64s.h)
This patch does not change any functionality.

Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/exception-64s.h | 19 +++++++++++++++++++
 arch/powerpc/kernel/idle_power7.S        | 20 +-------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 93ae809fe5ea..0082290314eb 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -254,6 +254,25 @@ do_kvm_##n:								\
 #define KVM_HANDLER_SKIP(area, h, n)
 #endif
 
+#define CHECK_HMI_INTERRUPT						\
+	mfspr	r0,SPRN_SRR1;						\
+BEGIN_FTR_SECTION_NESTED(66);						\
+	rlwinm	r0,r0,45-31,0xf;  /* extract wake reason field (P8) */	\
+FTR_SECTION_ELSE_NESTED(66);						\
+	rlwinm	r0,r0,45-31,0xe;  /* P7 wake reason field is 3 bits */	\
+ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
+	cmpwi	r0,0xa;			/* Hypervisor maintenance ? */	\
+	bne	20f;							\
+	/* Invoke opal call to handle hmi */				\
+	ld	r2,PACATOC(r13);					\
+	ld	r1,PACAR1(r13);						\
+	std	r3,ORIG_GPR3(r1);	/* Save original r3 */		\
+	li	r0,OPAL_HANDLE_HMI;	/* Pass opal token argument*/	\
+	bl	opal_call_realmode;					\
+	ld	r3,ORIG_GPR3(r1);	/* Restore original r3 */	\
+20:	nop;
+
+
 #define NOTEST(n)
 
 /*
diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
index cf4fb5429cf1..abc53e88a5b4 100644
--- a/arch/powerpc/kernel/idle_power7.S
+++ b/arch/powerpc/kernel/idle_power7.S
@@ -20,6 +20,7 @@
 #include <asm/opal.h>
 #include <asm/cpuidle.h>
 #include <asm/mmu-hash64.h>
+#include <asm/exception-64s.h>
 
 #undef DEBUG
 
@@ -257,25 +258,6 @@ _GLOBAL(power7_winkle)
 	b	power7_powersave_common
 	/* No return */
 
-#define CHECK_HMI_INTERRUPT						\
-	mfspr	r0,SPRN_SRR1;						\
-BEGIN_FTR_SECTION_NESTED(66);						\
-	rlwinm	r0,r0,45-31,0xf;  /* extract wake reason field (P8) */	\
-FTR_SECTION_ELSE_NESTED(66);						\
-	rlwinm	r0,r0,45-31,0xe;  /* P7 wake reason field is 3 bits */	\
-ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
-	cmpwi	r0,0xa;			/* Hypervisor maintenance ? */	\
-	bne	20f;							\
-	/* Invoke opal call to handle hmi */				\
-	ld	r2,PACATOC(r13);					\
-	ld	r1,PACAR1(r13);						\
-	std	r3,ORIG_GPR3(r1);	/* Save original r3 */		\
-	li	r0,OPAL_HANDLE_HMI;	/* Pass opal token argument*/	\
-	bl	opal_call_realmode;					\
-	ld	r3,ORIG_GPR3(r1);	/* Restore original r3 */	\
-20:	nop;
-
-
 _GLOBAL(power7_wakeup_tb_loss)
 	ld	r2,PACATOC(r13);
 	ld	r1,PACAR1(r13)
-- 
1.9.3

[PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

[Shreyas B. Prabhu]

Before entering any idle state which can result in a state loss
we currently save the context in the stack before entering idle.
Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
and other macros to commonly accessible location.

Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/cpuidle.h | 68 ++++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/idle_power7.S  | 65 ++----------------------------------
 2 files changed, 70 insertions(+), 63 deletions(-)

diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
index d2f99ca1e3a6..6c678a779e8e 100644
--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h
@@ -15,6 +15,74 @@ extern u32 pnv_fastsleep_workaround_at_entry[];
 extern u32 pnv_fastsleep_workaround_at_exit[];
 #endif
 
+/*
+ * IDLE_STATE_PREP - Will be called in preparation for entering
+ * any hardware idle state. Since idle states can result in a
+ * state loss, we create a regs frame on the stack, fill it up
+ * with the state we care about and stick a pointer to it in
+ * PACAR1. Use interrupt stack frame for this purpose.
+ * r3 - contains idle state to be entered
+ * r13 - PACA pointer
+ */
+#define IDLE_STATE_PREP						\
+	mflr	r0;						\
+	std	r0,16(r1);					\
+	stdu	r1,-INT_FRAME_SIZE(r1);				\
+	std	r0,_LINK(r1);					\
+	std	r0,_NIP(r1);					\
+								\
+	/* Hard disable interrupts */				\
+	mfmsr	r9;						\
+	rldicl	r9,r9,48,1;					\
+	rotldi	r9,r9,16;					\
+	mtmsrd	r9,1;		/* hard-disable interrupts */	\
+								\
+	/* Check if something happened while soft-disabled */	\
+	lbz	r0,PACAIRQHAPPENED(r13);			\
+	andi.	r0,r0,~PACA_IRQ_HARD_DIS@l;			\
+	beq	1f;						\
+	cmpwi	cr0,r4,0;					\
+	beq	1f;						\
+	addi	r1,r1,INT_FRAME_SIZE;				\
+	ld	r0,16(r1);					\
+	li	r3,0;			/* Return 0 (no nap) */	\
+	mtlr	r0;						\
+	blr;							\
+1:	/* We mark irqs hard disabled as this is the state	\
+	 * we'll  be in when returning and we need to tell	\
+	 * arch_local_irq_restore() about it */			\
+	li	r0,PACA_IRQ_HARD_DIS;				\
+	stb	r0,PACAIRQHAPPENED(r13);			\
+								\
+	/* We haven't lost state ... yet */			\
+	li	r0,0;						\
+	stb	r0,PACA_NAPSTATELOST(r13);			\
+								\
+	/* Continue saving state */				\
+	SAVE_GPR(2, r1);					\
+	SAVE_NVGPRS(r1);					\
+	mfcr	r4;						\
+	std	r4,_CCR(r1);					\
+	std	r9,_MSR(r1);					\
+	std	r1,PACAR1(r13);					\
+
+/*
+ * We use interrupt stack to save and restore few registers like
+ * PC, CR, LR and NVGPRs while enter/exiting idle states. Since
+ * volatile registers don't need to be saved/restored use that space
+ * instead to save/restore sprs which lose state during winkle.
+ */
+#define _SDR1	GPR3
+#define _RPR	GPR4
+#define _SPURR	GPR5
+#define _PURR	GPR6
+#define _TSCR	GPR7
+#define _DSCR	GPR8
+#define _AMOR	GPR9
+#define _WORT	GPR10
+#define _WORC	GPR11
+
+
 #endif
 
 #endif
diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
index abc53e88a5b4..6af57e292848 100644
--- a/arch/powerpc/kernel/idle_power7.S
+++ b/arch/powerpc/kernel/idle_power7.S
@@ -24,20 +24,6 @@
 
 #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 _WORT	GPR10
-#define _WORC	GPR11
-
 /* Idle state entry routines */
 
 #define	IDLE_STATE_ENTER_SEQ(IDLE_INST)				\
@@ -77,55 +63,8 @@ core_idle_lock_held:
  * 	1 - check
  */
 _GLOBAL(power7_powersave_common)
-	/* Use r3 to pass state nap/sleep/winkle */
-	/* NAP is a state loss, we create a regs frame on the
-	 * stack, fill it up with the state we care about and
-	 * stick a pointer to it in PACAR1. We really only
-	 * need to save PC, some CR bits and the NV GPRs,
-	 * but for now an interrupt frame will do.
-	 */
-	mflr	r0
-	std	r0,16(r1)
-	stdu	r1,-INT_FRAME_SIZE(r1)
-	std	r0,_LINK(r1)
-	std	r0,_NIP(r1)
-
-	/* Hard disable interrupts */
-	mfmsr	r9
-	rldicl	r9,r9,48,1
-	rotldi	r9,r9,16
-	mtmsrd	r9,1			/* hard-disable interrupts */
-
-	/* Check if something happened while soft-disabled */
-	lbz	r0,PACAIRQHAPPENED(r13)
-	andi.	r0,r0,~PACA_IRQ_HARD_DIS@l
-	beq	1f
-	cmpwi	cr0,r4,0
-	beq	1f
-	addi	r1,r1,INT_FRAME_SIZE
-	ld	r0,16(r1)
-	li	r3,0			/* Return 0 (no nap) */
-	mtlr	r0
-	blr
-
-1:	/* We mark irqs hard disabled as this is the state we'll
-	 * be in when returning and we need to tell arch_local_irq_restore()
-	 * about it
-	 */
-	li	r0,PACA_IRQ_HARD_DIS
-	stb	r0,PACAIRQHAPPENED(r13)
-
-	/* We haven't lost state ... yet */
-	li	r0,0
-	stb	r0,PACA_NAPSTATELOST(r13)
-
-	/* Continue saving state */
-	SAVE_GPR(2, r1)
-	SAVE_NVGPRS(r1)
-	mfcr	r4
-	std	r4,_CCR(r1)
-	std	r9,_MSR(r1)
-	std	r1,PACAR1(r13)
+	/* Save PC, CR, LR and NVGPRs in stack */
+	IDLE_STATE_PREP;
 
 	/*
 	 * Go to real mode to do the nap, as required by the architecture.
-- 
1.9.3

[PATCH 3/3] powerpc/powernv: Refactor hypervisor state restore code

[Shreyas B. Prabhu]

In the current code, when the thread wakes up in reset vector, some
of the state restore code and check for whether a thread needs to
branch to kvm is duplicated. Reorder the code such that this
duplication is avoided.

At a higher level this is what the change looks like-

Before this patch -
===================
power7_wakeup_tb_loss:
	restore hypervisor state
	if (thread needed by kvm)
		goto kvm_start_guest
	restore nvgprs, cr, pc
	rfid to process context

power7_wakeup_loss:
	restore nvgprs, cr, pc
	rfid to process context

reset vector:
	if (waking from deep idle states)
		goto power7_wakeup_tb_loss
	else
		if (thread needed by kvm)
			goto kvm_start_guest
		goto power7_wakeup_loss

After this patch -
==================
power7_wakeup_tb_loss:
	restore hypervisor state
	return

power7_restore_hyp_resource():
	if (waking from deep idle states)
		goto power7_wakeup_tb_loss
	return

power7_wakeup_loss:
	restore nvgprs, cr, pc
	rfid to process context

reset vector:
	power7_restore_hyp_resource()
	if (thread needed by kvm)
                goto kvm_start_guest
	goto power7_wakeup_loss

Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 29 +++-------------
 arch/powerpc/kernel/idle_power7.S    | 67 ++++++++++++++++++++----------------
 2 files changed, 41 insertions(+), 55 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 7716cebf4b8e..7ebfbb0c9992 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -107,25 +107,8 @@ BEGIN_FTR_SECTION
 	beq	9f
 
 	cmpwi	cr3,r13,2
+	bl	power7_restore_hyp_resource
 
-	/*
-	 * Check if last bit of HSPGR0 is set. This indicates whether we are
-	 * waking up from winkle.
-	 */
-	GET_PACA(r13)
-	clrldi	r5,r13,63
-	clrrdi	r13,r13,1
-	cmpwi	cr4,r5,1
-	mtspr	SPRN_HSPRG0,r13
-
-	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
-	cmpwi   cr2,r0,PNV_THREAD_NAP
-	bgt     cr2,8f				/* Either sleep or Winkle */
-
-	/* Waking up from nap should not cause hypervisor state loss */
-	bgt	cr3,.
-
-	/* Waking up from nap */
 	li	r0,PNV_THREAD_RUNNING
 	stb	r0,PACA_THREAD_IDLE_STATE(r13)	/* Clear thread state */
 
@@ -143,13 +126,9 @@ BEGIN_FTR_SECTION
 
 	/* Return SRR1 from power7_nap() */
 	mfspr	r3,SPRN_SRR1
-	beq	cr3,2f
-	b	power7_wakeup_noloss
-2:	b	power7_wakeup_loss
-
-	/* Fast Sleep wakeup on PowerNV */
-8:	GET_PACA(r13)
-	b 	power7_wakeup_tb_loss
+	blt	cr3,2f
+	b	power7_wakeup_loss
+2:	b	power7_wakeup_noloss
 
 9:
 END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
index 6af57e292848..0852685a22b8 100644
--- a/arch/powerpc/kernel/idle_power7.S
+++ b/arch/powerpc/kernel/idle_power7.S
@@ -197,6 +197,35 @@ _GLOBAL(power7_winkle)
 	b	power7_powersave_common
 	/* No return */
 
+/*
+ * Called from reset vector. Check whether we have woken up with
+ * hypervisor state loss. If yes, restore hypervisor state and return
+ * back to reset vector.
+ */
+_GLOBAL(power7_restore_hyp_resource)
+	/*
+	 * Check if last bit of HSPGR0 is set. This indicates whether we are
+	 * waking up from winkle.
+	 */
+	GET_PACA(r13)
+	clrldi	r5,r13,63
+	clrrdi	r13,r13,1
+	cmpwi	cr4,r5,1
+	mtspr	SPRN_HSPRG0,r13
+
+	lbz	r0,PACA_THREAD_IDLE_STATE(r13)
+	cmpwi   cr2,r0,PNV_THREAD_NAP
+	bgt     cr2,power7_wakeup_tb_loss	/* Either sleep or Winkle */
+
+	/*
+	 * We fall through here if PACA_THREAD_IDLE_STATE shows we are waking
+	 * up from nap. At this stage CR3 shouldn't contains 'gt' since that
+	 * indicates we are waking with hypervisor state loss from nap.
+	 */
+	bgt	cr3,.
+
+	blr
+
 _GLOBAL(power7_wakeup_tb_loss)
 	ld	r2,PACATOC(r13);
 	ld	r1,PACAR1(r13)
@@ -205,11 +234,13 @@ _GLOBAL(power7_wakeup_tb_loss)
 	 * and they are restored before switching to the process context. Hence
 	 * until they are restored, they are free to be used.
 	 *
-	 * Save SRR1 in a NVGPR as it might be clobbered in opal_call_realmode
-	 * (called in CHECK_HMI_INTERRUPT). SRR1 is required to determine the
-	 * wakeup reason if we branch to kvm_start_guest.
+	 * Save SRR1 and LR in NVGPRs as they might be clobbered in
+	 * opal_call_realmode (called in CHECK_HMI_INTERRUPT). SRR1 is required
+	 * to determine the wakeup reason if we branch to kvm_start_guest. LR
+	 * is required to return back to reset vector after hypervisor state
+	 * restore is complete.
 	 */
-
+	mflr	r17
 	mfspr	r16,SPRN_SRR1
 BEGIN_FTR_SECTION
 	CHECK_HMI_INTERRUPT
@@ -359,33 +390,9 @@ common_exit:
 
 hypervisor_state_restored:
 
-	li	r5,PNV_THREAD_RUNNING
-	stb     r5,PACA_THREAD_IDLE_STATE(r13)
-
 	mtspr	SPRN_SRR1,r16
-#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)
-	ld	r4,_MSR(r1)
-	ld	r5,_NIP(r1)
-	addi	r1,r1,INT_FRAME_SIZE
-	mtcr	r3
-	mfspr	r3,SPRN_SRR1		/* Return SRR1 */
-	mtspr	SPRN_SRR1,r4
-	mtspr	SPRN_SRR0,r5
-	rfid
+	mtlr	r17
+	blr
 
 fastsleep_workaround_at_exit:
 	li	r3,1
-- 
1.9.3

Re: [PATCH 0/3] powerpc/powernv: Cpuidle related cleanup

[Shreyas B Prabhu]

Hi,
Any thoughts on this?


On 02/29/2016 05:52 PM, Shreyas B. Prabhu wrote:
> These patches are purely code movement and cleanup.
> There is no functionality change.
> 
> Note, there are multiple style error reported for patch 1 and 2.
> I think this is because checkpatch script mistakes the patches
> as C code because of the presence of semi-colon in each line. I
> for now have largely ignored these errors.
> 
> Shreyas B. Prabhu (3):
>   powerpc/powernv: Move CHECK_HMI_INTERRUPT to exception-64s header
>   powerpc/powernv: Encapsulate idle preparation steps in a macro
>   powerpc/powernv: make hypervisor state restore a function
> 
>  arch/powerpc/include/asm/cpuidle.h       |  68 ++++++++++++++
>  arch/powerpc/include/asm/exception-64s.h |  19 ++++
>  arch/powerpc/kernel/exceptions-64s.S     |  29 +-----
>  arch/powerpc/kernel/idle_power7.S        | 148 ++++++++-----------------------
>  4 files changed, 129 insertions(+), 135 deletions(-)
> 

Re: [PATCH 0/3] powerpc/powernv: Cpuidle related cleanup

[Michael Ellerman]

On Thu, 2016-03-10 at 16:15 +0530, Shreyas B Prabhu wrote:

> Hi,
> Any thoughts on this?

Haven't had time to give it a proper review yet.

Will try and get to it.

cheers

Re: [PATCH 1/3] powerpc/powernv: Move CHECK_HMI_INTERRUPT to exception-64s header

[Paul Mackerras]

On Mon, Feb 29, 2016 at 05:52:58PM +0530, Shreyas B. Prabhu wrote:
> CHECK_HMI_INTERRUPT is used to check for HMI's in reset vector. Move
> the macro to a common location (exception-64s.h)
> This patch does not change any functionality.

Comments below...

> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 93ae809fe5ea..0082290314eb 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -254,6 +254,25 @@ do_kvm_##n:								\
>  #define KVM_HANDLER_SKIP(area, h, n)
>  #endif
>  
> +#define CHECK_HMI_INTERRUPT						\
> +	mfspr	r0,SPRN_SRR1;						\
> +BEGIN_FTR_SECTION_NESTED(66);						\
> +	rlwinm	r0,r0,45-31,0xf;  /* extract wake reason field (P8) */	\
> +FTR_SECTION_ELSE_NESTED(66);						\
> +	rlwinm	r0,r0,45-31,0xe;  /* P7 wake reason field is 3 bits */	\
> +ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
> +	cmpwi	r0,0xa;			/* Hypervisor maintenance ? */	\
> +	bne	20f;							\
> +	/* Invoke opal call to handle hmi */				\
> +	ld	r2,PACATOC(r13);					\
> +	ld	r1,PACAR1(r13);						\
> +	std	r3,ORIG_GPR3(r1);	/* Save original r3 */		\
> +	li	r0,OPAL_HANDLE_HMI;	/* Pass opal token argument*/	\
> +	bl	opal_call_realmode;					\
> +	ld	r3,ORIG_GPR3(r1);	/* Restore original r3 */	\
> +20:	nop;
> +
> +
>  #define NOTEST(n)

I'd rather keep NOTEST together with the definitions of KVMTEST etc.
I suggest you add this block at the end of exceptions-64s.h (just
before the final #endif).

Paul.

Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

[Paul Mackerras]

On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote:
> Before entering any idle state which can result in a state loss
> we currently save the context in the stack before entering idle.
> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
> and other macros to commonly accessible location.

There are two problems with this.  First, your new macro does much
more than create a stack frame and save some registers.  It also
messes with interrupts and potentially executes a blr instruction.
That is not what people would expect from the name of the macro or the
comments around it.  It also means that it would be hard to reuse the
macro in another place.

Secondly, I don't think this change helps readability.  Since the
macro is only used in one place, it doesn't reduce the total number of
lines of code, in fact it increases it slightly.  Having the macro
just means that someone reading the code has to look in two places to
see what power7_powersave_common is actually doing, rather than having
all the code in one place.  If what your macro did was a single thing
conceptually that took several instructions, then it might be helpful,
but as it is, it doesn't help readability.

Paul.

Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

[Shreyas B Prabhu]

Hi Paul,

On 03/17/2016 04:45 PM, Paul Mackerras wrote:
> On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote:
>> Before entering any idle state which can result in a state loss
>> we currently save the context in the stack before entering idle.
>> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
>> and other macros to commonly accessible location.
> 
> There are two problems with this.  First, your new macro does much
> more than create a stack frame and save some registers.  It also
> messes with interrupts and potentially executes a blr instruction.
> That is not what people would expect from the name of the macro or the
> comments around it.  It also means that it would be hard to reuse the
> macro in another place.
> 
> Secondly, I don't think this change helps readability.  Since the
> macro is only used in one place, it doesn't reduce the total number of
> lines of code, in fact it increases it slightly. 

This patch was in preparation for support for new POWER ISA v3 idle
states. The idea was to have the common idle preparation steps in a
macro which be reused while adding support for the new idle states. With
this context do you think this macro with better comments make sense?


Thanks,
Shreyas

Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

[Paul Mackerras]

On Fri, Mar 18, 2016 at 08:23:24PM +0530, Shreyas B Prabhu wrote:
> Hi Paul,
> 
> On 03/17/2016 04:45 PM, Paul Mackerras wrote:
> > On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote:
> >> Before entering any idle state which can result in a state loss
> >> we currently save the context in the stack before entering idle.
> >> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
> >> and other macros to commonly accessible location.
> > 
> > There are two problems with this.  First, your new macro does much
> > more than create a stack frame and save some registers.  It also
> > messes with interrupts and potentially executes a blr instruction.
> > That is not what people would expect from the name of the macro or the
> > comments around it.  It also means that it would be hard to reuse the
> > macro in another place.
> > 
> > Secondly, I don't think this change helps readability.  Since the
> > macro is only used in one place, it doesn't reduce the total number of
> > lines of code, in fact it increases it slightly. 
> 
> This patch was in preparation for support for new POWER ISA v3 idle
> states. The idea was to have the common idle preparation steps in a
> macro which be reused while adding support for the new idle states. With
> this context do you think this macro with better comments make sense?

No, it still does too many disparate things.  In particular it's a bad
idea to embed a blr inside a macro unless the name makes it very clear
that the macro can cause a return (e.g. the macro name is
RETURN_IF_<something>).  Yours would need to be called
MAKE_STACK_FRAME_AND_SAVE_SPRS_AND_HARD_DISABLE_AND_RETURN_IF_IRQ_OCCURRED
or something. :)

Paul.

Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

[Shreyas B Prabhu]

On 03/19/2016 05:51 AM, Paul Mackerras wrote:
> On Fri, Mar 18, 2016 at 08:23:24PM +0530, Shreyas B Prabhu wrote:
>> Hi Paul,
>>
>> On 03/17/2016 04:45 PM, Paul Mackerras wrote:
>>> On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote:
>>>> Before entering any idle state which can result in a state loss
>>>> we currently save the context in the stack before entering idle.
>>>> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
>>>> and other macros to commonly accessible location.
>>>
>>> There are two problems with this.  First, your new macro does much
>>> more than create a stack frame and save some registers.  It also
>>> messes with interrupts and potentially executes a blr instruction.
>>> That is not what people would expect from the name of the macro or the
>>> comments around it.  It also means that it would be hard to reuse the
>>> macro in another place.
>>>
>>> Secondly, I don't think this change helps readability.  Since the
>>> macro is only used in one place, it doesn't reduce the total number of
>>> lines of code, in fact it increases it slightly. 
>>
>> This patch was in preparation for support for new POWER ISA v3 idle
>> states. The idea was to have the common idle preparation steps in a
>> macro which be reused while adding support for the new idle states. With
>> this context do you think this macro with better comments make sense?
> 
> No, it still does too many disparate things.  In particular it's a bad
> idea to embed a blr inside a macro unless the name makes it very clear
> that the macro can cause a return (e.g. the macro name is
> RETURN_IF_<something>).  Yours would need to be called
> MAKE_STACK_FRAME_AND_SAVE_SPRS_AND_HARD_DISABLE_AND_RETURN_IF_IRQ_OCCURRED
> or something. :)
> 

Ok :) . I'll drop this patch and work this differently.

Thanks,
Shreyas