Re: [PATCH v3 18/18] powerpc/64s: Clear gprs on interrupt routine entry
> What about arch/powerpc/kernel/exceptions-64e.S, no change required > inside it ? As interru_64.S applies to both 64s and 64e, I would have > expected changes in exceptions_64e too. As it stands the changes in interrupt_64.S cause non-volatiles to be unconditionally restored. This may lead to a performance regression on Book3E, as previously interrupt_return_srr would restore non-volatiles only after handling a signal, otherwise assuming nvgprs to be intact. As some Book3E systems do feature speculation, it makes sense to perform the same mitigation on these systems as performed on Book3S systems.
Re: [PATCH v3 18/18] powerpc/64s: Clear gprs on interrupt routine entry
Le 19/08/2022 à 05:38, Rohan McLure a écrit :
> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
> other interrupt sources to limit influence of user-space values
> in potential speculation gadgets. The remaining gprs are overwritten by
> entry macros to interrupt handlers, irrespective of whether or not a
> given handler consumes these register values.
>
> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
> exit, but now they are always restored. Remove explicit REST_NVGPRS
> invocations as non-volatiles must now always be restored. 32-bit systems
> do not clear user registers on interrupt, and continue to depend on the
> return value of interrupt_exit_user_prepare to determine whether or not
> to restore non-volatiles.
>
> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
> See ~0.8% performance regression with this mitigation, but this
> indicates the worst-case performance due to heavier-weight interrupt
> handlers.
>
> Signed-off-by: Rohan McLure
> ---
> V1 -> V2: Add benchmark data
> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
> interrupt_exit_user_prepare changes in summary.
> ---
> arch/powerpc/kernel/exceptions-64s.S | 21 -
> arch/powerpc/kernel/interru_64.S | 9 ++---pt
What about arch/powerpc/kernel/exceptions-64e.S, no change required
inside it ? As interru_64.S applies to both 64s and 64e, I would have
expected changes in exceptions_64e too.
> 2 files changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S
> b/arch/powerpc/kernel/exceptions-64s.S
> index a3b51441b039..038e42fb2182 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
> std r10,0(r1) /* make stack chain pointer */
> std r0,GPR0(r1) /* save r0 in stackframe*/
> std r10,GPR1(r1)/* save r1 in stackframe*/
> + ZEROIZE_GPR(0)
>
> /* Mark our [H]SRRs valid for return */
> li r10,1
> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r10,IAREA+EX_R10(r13)
> std r9,GPR9(r1)
> std r10,GPR10(r1)
> + ZEROIZE_GPRS(9, 10)
> ld r9,IAREA+EX_R11(r13)/* move r11 - r13 to stackframe */
> ld r10,IAREA+EX_R12(r13)
> ld r11,IAREA+EX_R13(r13)
> std r9,GPR11(r1)
> std r10,GPR12(r1)
> std r11,GPR13(r1)
> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
> + ZEROIZE_GPR(11)
>
> SAVE_NVGPRS(r1)
> + ZEROIZE_NVGPRS()
>
> .if IDAR
> .if IISIDE
> @@ -577,8 +582,8 @@ BEGIN_FTR_SECTION
> END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> ld r10,IAREA+EX_CTR(r13)
> std r10,_CTR(r1)
> - std r2,GPR2(r1) /* save r2 in stackframe*/
> - SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
> + SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
> + ZEROIZE_GPRS(2, 8)
> mflrr9 /* Get LR, later save to stack */
> ld r2,PACATOC(r13) /* get kernel TOC into r2 */
> std r9,_LINK(r1)
> @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> mtlrr9
> ld r9,_CCR(r1)
> mtcrr9
> + REST_NVGPRS(r1)
> REST_GPRS(2, 13, r1)
> REST_GPR(0, r1)
> /* restore original r1. */
> @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
> b interrupt_return_srr
>
> 1: bl do_break
> - /*
> - * do_break() may have changed the NV GPRS while handling a breakpoint.
> - * If so, we need to restore them with their updated values.
> - */
> - REST_NVGPRS(r1)
> b interrupt_return_srr
>
>
> @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common)
> GEN_COMMON alignment
> addir3,r1,STACK_FRAME_OVERHEAD
> bl alignment_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_srr
>
>
> @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common)
> .Ldo_program_check:
> addir3,r1,STACK_FRAME_OVERHEAD
> bl program_check_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_srr
>
>
> @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common)
> GEN_COMMON emulation_assist
> addir3,r1,STACK_FRAME_OVERHEAD
> bl emulation_assist_interrupt
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_hsrr
>
>
> @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
> GEN_COMMON facility_unavailable
> addi
[PATCH v3 18/18] powerpc/64s: Clear gprs on interrupt routine entry
Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
other interrupt sources to limit influence of user-space values
in potential speculation gadgets. The remaining gprs are overwritten by
entry macros to interrupt handlers, irrespective of whether or not a
given handler consumes these register values.
Prior to this commit, r14-r31 are restored on a per-interrupt basis at
exit, but now they are always restored. Remove explicit REST_NVGPRS
invocations as non-volatiles must now always be restored. 32-bit systems
do not clear user registers on interrupt, and continue to depend on the
return value of interrupt_exit_user_prepare to determine whether or not
to restore non-volatiles.
The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
See ~0.8% performance regression with this mitigation, but this
indicates the worst-case performance due to heavier-weight interrupt
handlers.
Signed-off-by: Rohan McLure
---
V1 -> V2: Add benchmark data
V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
interrupt_exit_user_prepare changes in summary.
---
arch/powerpc/kernel/exceptions-64s.S | 21 -
arch/powerpc/kernel/interrupt_64.S | 9 ++---
2 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S
b/arch/powerpc/kernel/exceptions-64s.S
index a3b51441b039..038e42fb2182 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
std r10,0(r1) /* make stack chain pointer */
std r0,GPR0(r1) /* save r0 in stackframe*/
std r10,GPR1(r1)/* save r1 in stackframe*/
+ ZEROIZE_GPR(0)
/* Mark our [H]SRRs valid for return */
li r10,1
@@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r10,IAREA+EX_R10(r13)
std r9,GPR9(r1)
std r10,GPR10(r1)
+ ZEROIZE_GPRS(9, 10)
ld r9,IAREA+EX_R11(r13)/* move r11 - r13 to stackframe */
ld r10,IAREA+EX_R12(r13)
ld r11,IAREA+EX_R13(r13)
std r9,GPR11(r1)
std r10,GPR12(r1)
std r11,GPR13(r1)
+ /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
+ ZEROIZE_GPR(11)
SAVE_NVGPRS(r1)
+ ZEROIZE_NVGPRS()
.if IDAR
.if IISIDE
@@ -577,8 +582,8 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
ld r10,IAREA+EX_CTR(r13)
std r10,_CTR(r1)
- std r2,GPR2(r1) /* save r2 in stackframe*/
- SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
+ SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
+ ZEROIZE_GPRS(2, 8)
mflrr9 /* Get LR, later save to stack */
ld r2,PACATOC(r13) /* get kernel TOC into r2 */
std r9,_LINK(r1)
@@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
mtlrr9
ld r9,_CCR(r1)
mtcrr9
+ REST_NVGPRS(r1)
REST_GPRS(2, 13, r1)
REST_GPR(0, r1)
/* restore original r1. */
@@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
b interrupt_return_srr
1: bl do_break
- /*
-* do_break() may have changed the NV GPRS while handling a breakpoint.
-* If so, we need to restore them with their updated values.
-*/
- REST_NVGPRS(r1)
b interrupt_return_srr
@@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common)
GEN_COMMON alignment
addir3,r1,STACK_FRAME_OVERHEAD
bl alignment_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common)
.Ldo_program_check:
addir3,r1,STACK_FRAME_OVERHEAD
bl program_check_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common)
GEN_COMMON emulation_assist
addir3,r1,STACK_FRAME_OVERHEAD
bl emulation_assist_interrupt
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_hsrr
@@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
GEN_COMMON facility_unavailable
addir3,r1,STACK_FRAME_OVERHEAD
bl facility_unavailable_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common)
GEN_COMMON h_facility_unavailable
addir3,r1,STACK_FRAME_OVERHEAD
bl
