* [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-07-26 14:30 ` sxwjean
0 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, christophe.leroy,
npiggin, aneesh.kumar, sandipan, efremov, peterx, akpm,
linux-kernel, Xiongwei Song
From: Xiongwei Song <sxwjean@gmail.com>
Create an anonymous union for dsisr and esr regsiters, we can reference
esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
Otherwise, reference dsisr. This makes code more clear.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 5 ++++-
arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
arch/powerpc/kernel/traps.c | 2 +-
arch/powerpc/mm/fault.c | 16 ++++++++++++++--
arch/powerpc/platforms/44x/machine_check.c | 4 ++--
arch/powerpc/platforms/4xx/machine_check.c | 2 +-
8 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 3e5d470a6155..c252d04b1206 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -44,7 +44,10 @@ struct pt_regs
#endif
unsigned long trap;
unsigned long dar;
- unsigned long dsisr;
+ union {
+ unsigned long dsisr;
+ unsigned long esr;
+ };
unsigned long result;
};
};
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 7004cfea3f5f..e357288b5f34 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -53,7 +53,10 @@ struct pt_regs
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
unsigned long dar; /* Fault registers */
- unsigned long dsisr; /* on 4xx/Book-E used for ESR */
+ union {
+ unsigned long dsisr; /* on Book-S used for DSISR */
+ unsigned long esr; /* on 4xx/Book-E used for ESR */
+ };
unsigned long result; /* Result of a system call */
};
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 185beb290580..f74af8f9133c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
trap == INTERRUPT_DATA_STORAGE ||
trap == INTERRUPT_ALIGNMENT) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
+ pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
else
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 0a0a33eb0d28..00789ad2c4a3 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -375,6 +375,8 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, dar));
BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
offsetof(struct user_pt_regs, dsisr));
+ BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
+ offsetof(struct user_pt_regs, esr));
BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
offsetof(struct user_pt_regs, result));
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index dfbce527c98e..2164f5705a0b 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
/* On 4xx, the reason for the machine check or program exception
is in the ESR. */
-#define get_reason(regs) ((regs)->dsisr)
+#define get_reason(regs) ((regs)->esr)
#define REASON_FP ESR_FP
#define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
#define REASON_PRIVILEGED ESR_PPR
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a8d0ce85d39a..62953d4e7c93 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
{
long err;
- err = ___do_page_fault(regs, regs->dar, regs->dsisr);
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ err = ___do_page_fault(regs, regs->dar, regs->esr);
+ else
+ err = ___do_page_fault(regs, regs->dar, regs->dsisr);
+
if (unlikely(err))
bad_page_fault(regs, err);
}
@@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
*/
static void __bad_page_fault(struct pt_regs *regs, int sig)
{
- int is_write = page_fault_is_write(regs->dsisr);
+ unsigned long err_reg;
+ int is_write;
+
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ err_reg = regs->esr;
+ else
+ err_reg = regs->dsisr;
+
+ is_write = page_fault_is_write(err_reg);
/* kernel has accessed a bad area */
diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
index a5c898bb9bab..5d19daacd78a 100644
--- a/arch/powerpc/platforms/44x/machine_check.c
+++ b/arch/powerpc/platforms/44x/machine_check.c
@@ -11,7 +11,7 @@
int machine_check_440A(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
printk("Machine check in kernel mode.\n");
if (reason & ESR_IMCP){
@@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
#ifdef CONFIG_PPC_47x
int machine_check_47x(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
u32 mcsr;
printk(KERN_ERR "Machine check in kernel mode.\n");
diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
index a71c29892a91..a905da1d6f41 100644
--- a/arch/powerpc/platforms/4xx/machine_check.c
+++ b/arch/powerpc/platforms/4xx/machine_check.c
@@ -10,7 +10,7 @@
int machine_check_4xx(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
if (reason & ESR_IMCP) {
printk("Instruction");
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-07-26 14:30 ` sxwjean
0 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, aneesh.kumar, oleg, linux-kernel,
efremov, paulus, npiggin, peterx, akpm, sandipan
From: Xiongwei Song <sxwjean@gmail.com>
Create an anonymous union for dsisr and esr regsiters, we can reference
esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
Otherwise, reference dsisr. This makes code more clear.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 5 ++++-
arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
arch/powerpc/kernel/traps.c | 2 +-
arch/powerpc/mm/fault.c | 16 ++++++++++++++--
arch/powerpc/platforms/44x/machine_check.c | 4 ++--
arch/powerpc/platforms/4xx/machine_check.c | 2 +-
8 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 3e5d470a6155..c252d04b1206 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -44,7 +44,10 @@ struct pt_regs
#endif
unsigned long trap;
unsigned long dar;
- unsigned long dsisr;
+ union {
+ unsigned long dsisr;
+ unsigned long esr;
+ };
unsigned long result;
};
};
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index 7004cfea3f5f..e357288b5f34 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -53,7 +53,10 @@ struct pt_regs
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
unsigned long dar; /* Fault registers */
- unsigned long dsisr; /* on 4xx/Book-E used for ESR */
+ union {
+ unsigned long dsisr; /* on Book-S used for DSISR */
+ unsigned long esr; /* on 4xx/Book-E used for ESR */
+ };
unsigned long result; /* Result of a system call */
};
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 185beb290580..f74af8f9133c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
trap == INTERRUPT_DATA_STORAGE ||
trap == INTERRUPT_ALIGNMENT) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
+ pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
else
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 0a0a33eb0d28..00789ad2c4a3 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -375,6 +375,8 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, dar));
BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
offsetof(struct user_pt_regs, dsisr));
+ BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
+ offsetof(struct user_pt_regs, esr));
BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
offsetof(struct user_pt_regs, result));
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index dfbce527c98e..2164f5705a0b 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
/* On 4xx, the reason for the machine check or program exception
is in the ESR. */
-#define get_reason(regs) ((regs)->dsisr)
+#define get_reason(regs) ((regs)->esr)
#define REASON_FP ESR_FP
#define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
#define REASON_PRIVILEGED ESR_PPR
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a8d0ce85d39a..62953d4e7c93 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
{
long err;
- err = ___do_page_fault(regs, regs->dar, regs->dsisr);
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ err = ___do_page_fault(regs, regs->dar, regs->esr);
+ else
+ err = ___do_page_fault(regs, regs->dar, regs->dsisr);
+
if (unlikely(err))
bad_page_fault(regs, err);
}
@@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
*/
static void __bad_page_fault(struct pt_regs *regs, int sig)
{
- int is_write = page_fault_is_write(regs->dsisr);
+ unsigned long err_reg;
+ int is_write;
+
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ err_reg = regs->esr;
+ else
+ err_reg = regs->dsisr;
+
+ is_write = page_fault_is_write(err_reg);
/* kernel has accessed a bad area */
diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
index a5c898bb9bab..5d19daacd78a 100644
--- a/arch/powerpc/platforms/44x/machine_check.c
+++ b/arch/powerpc/platforms/44x/machine_check.c
@@ -11,7 +11,7 @@
int machine_check_440A(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
printk("Machine check in kernel mode.\n");
if (reason & ESR_IMCP){
@@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
#ifdef CONFIG_PPC_47x
int machine_check_47x(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
u32 mcsr;
printk(KERN_ERR "Machine check in kernel mode.\n");
diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
index a71c29892a91..a905da1d6f41 100644
--- a/arch/powerpc/platforms/4xx/machine_check.c
+++ b/arch/powerpc/platforms/4xx/machine_check.c
@@ -10,7 +10,7 @@
int machine_check_4xx(struct pt_regs *regs)
{
- unsigned long reason = regs->dsisr;
+ unsigned long reason = regs->esr;
if (reason & ESR_IMCP) {
printk("Instruction");
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 2/4] powerpc/64e: Get esr offset with _ESR macro
2021-07-26 14:30 ` sxwjean
@ 2021-07-26 14:30 ` sxwjean
-1 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, christophe.leroy,
npiggin, aneesh.kumar, sandipan, efremov, peterx, akpm,
linux-kernel, Xiongwei Song
From: Xiongwei Song <sxwjean@gmail.com>
Use _ESR to get the offset of esr register in pr_regs for 64e cpus.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/exceptions-64e.S | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a47eefa09bcb..f4ebc435fd78 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -287,6 +287,7 @@ int main(void)
STACK_PT_REGS_OFFSET(_XER, xer);
STACK_PT_REGS_OFFSET(_DAR, dar);
STACK_PT_REGS_OFFSET(_DSISR, dsisr);
+ STACK_PT_REGS_OFFSET(_ESR, esr);
STACK_PT_REGS_OFFSET(ORIG_GPR3, orig_gpr3);
STACK_PT_REGS_OFFSET(RESULT, result);
STACK_PT_REGS_OFFSET(_TRAP, trap);
@@ -298,7 +299,6 @@ int main(void)
* we use them to hold SRR0 and SRR1.
*/
STACK_PT_REGS_OFFSET(_DEAR, dar);
- STACK_PT_REGS_OFFSET(_ESR, dsisr);
#else /* CONFIG_PPC64 */
STACK_PT_REGS_OFFSET(SOFTE, softe);
STACK_PT_REGS_OFFSET(_PPR, ppr);
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 1401787b0b93..bf8c4c2f98ea 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -546,7 +546,7 @@ __end_interrupts:
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x300)
@@ -559,7 +559,7 @@ __end_interrupts:
li r15,0
mr r14,r10
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x400)
@@ -576,7 +576,7 @@ __end_interrupts:
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x600)
@@ -587,7 +587,7 @@ __end_interrupts:
NORMAL_EXCEPTION_PROLOG(0x700, BOOKE_INTERRUPT_PROGRAM,
PROLOG_ADDITION_1REG)
mfspr r14,SPRN_ESR
- std r14,_DSISR(r1)
+ std r14,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
EXCEPTION_COMMON(0x700)
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -1058,7 +1058,7 @@ bad_stack_book3e:
mfspr r10,SPRN_DEAR
mfspr r11,SPRN_ESR
std r10,_DAR(r1)
- std r11,_DSISR(r1)
+ std r11,_ESR(r1)
std r0,GPR0(r1); /* save r0 in stackframe */ \
std r2,GPR2(r1); /* save r2 in stackframe */ \
SAVE_4GPRS(3, r1); /* save r3 - r6 in stackframe */ \
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 2/4] powerpc/64e: Get esr offset with _ESR macro
@ 2021-07-26 14:30 ` sxwjean
0 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, aneesh.kumar, oleg, linux-kernel,
efremov, paulus, npiggin, peterx, akpm, sandipan
From: Xiongwei Song <sxwjean@gmail.com>
Use _ESR to get the offset of esr register in pr_regs for 64e cpus.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/exceptions-64e.S | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a47eefa09bcb..f4ebc435fd78 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -287,6 +287,7 @@ int main(void)
STACK_PT_REGS_OFFSET(_XER, xer);
STACK_PT_REGS_OFFSET(_DAR, dar);
STACK_PT_REGS_OFFSET(_DSISR, dsisr);
+ STACK_PT_REGS_OFFSET(_ESR, esr);
STACK_PT_REGS_OFFSET(ORIG_GPR3, orig_gpr3);
STACK_PT_REGS_OFFSET(RESULT, result);
STACK_PT_REGS_OFFSET(_TRAP, trap);
@@ -298,7 +299,6 @@ int main(void)
* we use them to hold SRR0 and SRR1.
*/
STACK_PT_REGS_OFFSET(_DEAR, dar);
- STACK_PT_REGS_OFFSET(_ESR, dsisr);
#else /* CONFIG_PPC64 */
STACK_PT_REGS_OFFSET(SOFTE, softe);
STACK_PT_REGS_OFFSET(_PPR, ppr);
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 1401787b0b93..bf8c4c2f98ea 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -546,7 +546,7 @@ __end_interrupts:
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x300)
@@ -559,7 +559,7 @@ __end_interrupts:
li r15,0
mr r14,r10
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x400)
@@ -576,7 +576,7 @@ __end_interrupts:
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
std r14,_DAR(r1)
- std r15,_DSISR(r1)
+ std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
EXCEPTION_COMMON(0x600)
@@ -587,7 +587,7 @@ __end_interrupts:
NORMAL_EXCEPTION_PROLOG(0x700, BOOKE_INTERRUPT_PROGRAM,
PROLOG_ADDITION_1REG)
mfspr r14,SPRN_ESR
- std r14,_DSISR(r1)
+ std r14,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
EXCEPTION_COMMON(0x700)
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -1058,7 +1058,7 @@ bad_stack_book3e:
mfspr r10,SPRN_DEAR
mfspr r11,SPRN_ESR
std r10,_DAR(r1)
- std r11,_DSISR(r1)
+ std r11,_ESR(r1)
std r0,GPR0(r1); /* save r0 in stackframe */ \
std r2,GPR2(r1); /* save r2 in stackframe */ \
SAVE_4GPRS(3, r1); /* save r3 - r6 in stackframe */ \
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
2021-07-26 14:30 ` sxwjean
@ 2021-07-26 14:30 ` sxwjean
-1 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, christophe.leroy,
npiggin, aneesh.kumar, sandipan, efremov, peterx, akpm,
linux-kernel, Xiongwei Song
From: Xiongwei Song <sxwjean@gmail.com>
Create an anonymous union for dar and dear regsiters, we can reference
dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
Otherwise, reference dar. This makes code more clear.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 5 ++++-
arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
arch/powerpc/kernel/traps.c | 5 ++++-
arch/powerpc/mm/fault.c | 2 +-
6 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index c252d04b1206..fa725e3238c2 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -43,7 +43,10 @@ struct pt_regs
unsigned long mq;
#endif
unsigned long trap;
- unsigned long dar;
+ union {
+ unsigned long dar;
+ unsigned long dear;
+ };
union {
unsigned long dsisr;
unsigned long esr;
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index e357288b5f34..9ae150fb4c4b 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -52,7 +52,10 @@ struct pt_regs
unsigned long trap; /* Reason for being here */
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
- unsigned long dar; /* Fault registers */
+ union {
+ unsigned long dar; /* Fault registers */
+ unsigned long dear;
+ };
union {
unsigned long dsisr; /* on Book-S used for DSISR */
unsigned long esr; /* on 4xx/Book-E used for ESR */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f74af8f9133c..50436b52c213 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
trap == INTERRUPT_DATA_STORAGE ||
trap == INTERRUPT_ALIGNMENT) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
+ pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
else
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 00789ad2c4a3..969dca8b0718 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -373,6 +373,8 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, trap));
BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
offsetof(struct user_pt_regs, dar));
+ BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
+ offsetof(struct user_pt_regs, dear));
BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
offsetof(struct user_pt_regs, dsisr));
BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2164f5705a0b..0796630d3d23 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
}
bad:
if (user_mode(regs))
- _exception(sig, regs, code, regs->dar);
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ _exception(sig, regs, code, regs->dear);
+ else
+ _exception(sig, regs, code, regs->dar);
else
bad_page_fault(regs, sig);
}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 62953d4e7c93..3db6b39a1178 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
long err;
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- err = ___do_page_fault(regs, regs->dar, regs->esr);
+ err = ___do_page_fault(regs, regs->dear, regs->esr);
else
err = ___do_page_fault(regs, regs->dar, regs->dsisr);
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
@ 2021-07-26 14:30 ` sxwjean
0 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, aneesh.kumar, oleg, linux-kernel,
efremov, paulus, npiggin, peterx, akpm, sandipan
From: Xiongwei Song <sxwjean@gmail.com>
Create an anonymous union for dar and dear regsiters, we can reference
dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
Otherwise, reference dar. This makes code more clear.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 5 ++++-
arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
arch/powerpc/kernel/traps.c | 5 ++++-
arch/powerpc/mm/fault.c | 2 +-
6 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index c252d04b1206..fa725e3238c2 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -43,7 +43,10 @@ struct pt_regs
unsigned long mq;
#endif
unsigned long trap;
- unsigned long dar;
+ union {
+ unsigned long dar;
+ unsigned long dear;
+ };
union {
unsigned long dsisr;
unsigned long esr;
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index e357288b5f34..9ae150fb4c4b 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -52,7 +52,10 @@ struct pt_regs
unsigned long trap; /* Reason for being here */
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
- unsigned long dar; /* Fault registers */
+ union {
+ unsigned long dar; /* Fault registers */
+ unsigned long dear;
+ };
union {
unsigned long dsisr; /* on Book-S used for DSISR */
unsigned long esr; /* on 4xx/Book-E used for ESR */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f74af8f9133c..50436b52c213 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
trap == INTERRUPT_DATA_STORAGE ||
trap == INTERRUPT_ALIGNMENT) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
+ pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
else
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 00789ad2c4a3..969dca8b0718 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -373,6 +373,8 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, trap));
BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
offsetof(struct user_pt_regs, dar));
+ BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
+ offsetof(struct user_pt_regs, dear));
BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
offsetof(struct user_pt_regs, dsisr));
BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2164f5705a0b..0796630d3d23 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
}
bad:
if (user_mode(regs))
- _exception(sig, regs, code, regs->dar);
+ if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
+ _exception(sig, regs, code, regs->dear);
+ else
+ _exception(sig, regs, code, regs->dar);
else
bad_page_fault(regs, sig);
}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 62953d4e7c93..3db6b39a1178 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
long err;
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
- err = ___do_page_fault(regs, regs->dar, regs->esr);
+ err = ___do_page_fault(regs, regs->dear, regs->esr);
else
err = ___do_page_fault(regs, regs->dar, regs->dsisr);
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 4/4] powerpc/64e: Get dear offset with _DEAR macro
2021-07-26 14:30 ` sxwjean
@ 2021-07-26 14:30 ` sxwjean
-1 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, christophe.leroy,
npiggin, aneesh.kumar, sandipan, efremov, peterx, akpm,
linux-kernel, Xiongwei Song
From: Xiongwei Song <sxwjean@gmail.com>
Use _DEAR to get the offset of dear register in pr_regs for 64e cpus.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/asm-offsets.c | 13 +++----------
arch/powerpc/kernel/exceptions-64e.S | 8 ++++----
2 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index f4ebc435fd78..8357d5fcd09e 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -286,23 +286,16 @@ int main(void)
STACK_PT_REGS_OFFSET(_CCR, ccr);
STACK_PT_REGS_OFFSET(_XER, xer);
STACK_PT_REGS_OFFSET(_DAR, dar);
+ STACK_PT_REGS_OFFSET(_DEAR, dear);
STACK_PT_REGS_OFFSET(_DSISR, dsisr);
STACK_PT_REGS_OFFSET(_ESR, esr);
STACK_PT_REGS_OFFSET(ORIG_GPR3, orig_gpr3);
STACK_PT_REGS_OFFSET(RESULT, result);
STACK_PT_REGS_OFFSET(_TRAP, trap);
-#ifndef CONFIG_PPC64
- /*
- * The PowerPC 400-class & Book-E processors have neither the DAR
- * nor the DSISR SPRs. Hence, we overload them to hold the similar
- * DEAR and ESR SPRs for such processors. For critical interrupts
- * we use them to hold SRR0 and SRR1.
- */
- STACK_PT_REGS_OFFSET(_DEAR, dar);
-#else /* CONFIG_PPC64 */
+#ifdef CONFIG_PPC64
STACK_PT_REGS_OFFSET(SOFTE, softe);
STACK_PT_REGS_OFFSET(_PPR, ppr);
-#endif /* CONFIG_PPC64 */
+#endif
#ifdef CONFIG_PPC_PKEY
STACK_PT_REGS_OFFSET(STACK_REGS_AMR, amr);
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index bf8c4c2f98ea..221e085e8c8c 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -545,7 +545,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -558,7 +558,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
li r15,0
mr r14,r10
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -575,7 +575,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -1057,7 +1057,7 @@ bad_stack_book3e:
std r11,_CCR(r1)
mfspr r10,SPRN_DEAR
mfspr r11,SPRN_ESR
- std r10,_DAR(r1)
+ std r10,_DEAR(r1)
std r11,_ESR(r1)
std r0,GPR0(r1); /* save r0 in stackframe */ \
std r2,GPR2(r1); /* save r2 in stackframe */ \
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC PATCH 4/4] powerpc/64e: Get dear offset with _DEAR macro
@ 2021-07-26 14:30 ` sxwjean
0 siblings, 0 replies; 26+ messages in thread
From: sxwjean @ 2021-07-26 14:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, aneesh.kumar, oleg, linux-kernel,
efremov, paulus, npiggin, peterx, akpm, sandipan
From: Xiongwei Song <sxwjean@gmail.com>
Use _DEAR to get the offset of dear register in pr_regs for 64e cpus.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/kernel/asm-offsets.c | 13 +++----------
arch/powerpc/kernel/exceptions-64e.S | 8 ++++----
2 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index f4ebc435fd78..8357d5fcd09e 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -286,23 +286,16 @@ int main(void)
STACK_PT_REGS_OFFSET(_CCR, ccr);
STACK_PT_REGS_OFFSET(_XER, xer);
STACK_PT_REGS_OFFSET(_DAR, dar);
+ STACK_PT_REGS_OFFSET(_DEAR, dear);
STACK_PT_REGS_OFFSET(_DSISR, dsisr);
STACK_PT_REGS_OFFSET(_ESR, esr);
STACK_PT_REGS_OFFSET(ORIG_GPR3, orig_gpr3);
STACK_PT_REGS_OFFSET(RESULT, result);
STACK_PT_REGS_OFFSET(_TRAP, trap);
-#ifndef CONFIG_PPC64
- /*
- * The PowerPC 400-class & Book-E processors have neither the DAR
- * nor the DSISR SPRs. Hence, we overload them to hold the similar
- * DEAR and ESR SPRs for such processors. For critical interrupts
- * we use them to hold SRR0 and SRR1.
- */
- STACK_PT_REGS_OFFSET(_DEAR, dar);
-#else /* CONFIG_PPC64 */
+#ifdef CONFIG_PPC64
STACK_PT_REGS_OFFSET(SOFTE, softe);
STACK_PT_REGS_OFFSET(_PPR, ppr);
-#endif /* CONFIG_PPC64 */
+#endif
#ifdef CONFIG_PPC_PKEY
STACK_PT_REGS_OFFSET(STACK_REGS_AMR, amr);
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index bf8c4c2f98ea..221e085e8c8c 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -545,7 +545,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -558,7 +558,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
li r15,0
mr r14,r10
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -575,7 +575,7 @@ __end_interrupts:
PROLOG_ADDITION_2REGS)
mfspr r14,SPRN_DEAR
mfspr r15,SPRN_ESR
- std r14,_DAR(r1)
+ std r14,_DEAR(r1)
std r15,_ESR(r1)
ld r14,PACA_EXGEN+EX_R14(r13)
ld r15,PACA_EXGEN+EX_R15(r13)
@@ -1057,7 +1057,7 @@ bad_stack_book3e:
std r11,_CCR(r1)
mfspr r10,SPRN_DEAR
mfspr r11,SPRN_ESR
- std r10,_DAR(r1)
+ std r10,_DEAR(r1)
std r11,_ESR(r1)
std r0,GPR0(r1); /* save r0 in stackframe */ \
std r2,GPR2(r1); /* save r2 in stackframe */ \
--
2.30.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-07-26 14:30 ` sxwjean
@ 2021-08-05 10:06 ` Christophe Leroy
-1 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-05 10:06 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, npiggin, aneesh.kumar,
sandipan, efremov, peterx, akpm, linux-kernel, Xiongwei Song
Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dsisr and esr regsiters, we can reference
> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dsisr. This makes code more clear.
I'm not sure it is worth doing that.
What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
the same:
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> +
Or even
> - int is_write = page_fault_is_write(regs->dsisr);
> + unsigned long err_reg;
> + int is_write;
> +
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err_reg = regs->esr;
> + else
> + err_reg = regs->dsisr;
> +
> + is_write = page_fault_is_write(err_reg);
Artificially growing the code for that makes no sense to me.
To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
that we know it represents the status register, which is DSISR or ESR depending on the platform.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 2 +-
> arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> 8 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 3e5d470a6155..c252d04b1206 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -44,7 +44,10 @@ struct pt_regs
> #endif
> unsigned long trap;
> unsigned long dar;
> - unsigned long dsisr;
> + union {
> + unsigned long dsisr;
> + unsigned long esr;
> + };
> unsigned long result;
> };
> };
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index 7004cfea3f5f..e357288b5f34 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -53,7 +53,10 @@ struct pt_regs
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> unsigned long dar; /* Fault registers */
> - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> + union {
> + unsigned long dsisr; /* on Book-S used for DSISR */
> + unsigned long esr; /* on 4xx/Book-E used for ESR */
> + };
> unsigned long result; /* Result of a system call */
> };
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 185beb290580..f74af8f9133c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 0a0a33eb0d28..00789ad2c4a3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, dar));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> + offsetof(struct user_pt_regs, esr));
> BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> offsetof(struct user_pt_regs, result));
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index dfbce527c98e..2164f5705a0b 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> /* On 4xx, the reason for the machine check or program exception
> is in the ESR. */
> -#define get_reason(regs) ((regs)->dsisr)
> +#define get_reason(regs) ((regs)->esr)
> #define REASON_FP ESR_FP
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a8d0ce85d39a..62953d4e7c93 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> {
> long err;
>
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> +
> if (unlikely(err))
> bad_page_fault(regs, err);
> }
> @@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
> */
> static void __bad_page_fault(struct pt_regs *regs, int sig)
> {
> - int is_write = page_fault_is_write(regs->dsisr);
> + unsigned long err_reg;
> + int is_write;
> +
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err_reg = regs->esr;
> + else
> + err_reg = regs->dsisr;
> +
> + is_write = page_fault_is_write(err_reg);
>
> /* kernel has accessed a bad area */
>
> diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
> index a5c898bb9bab..5d19daacd78a 100644
> --- a/arch/powerpc/platforms/44x/machine_check.c
> +++ b/arch/powerpc/platforms/44x/machine_check.c
> @@ -11,7 +11,7 @@
>
> int machine_check_440A(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
>
> printk("Machine check in kernel mode.\n");
> if (reason & ESR_IMCP){
> @@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
> #ifdef CONFIG_PPC_47x
> int machine_check_47x(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
> u32 mcsr;
>
> printk(KERN_ERR "Machine check in kernel mode.\n");
> diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
> index a71c29892a91..a905da1d6f41 100644
> --- a/arch/powerpc/platforms/4xx/machine_check.c
> +++ b/arch/powerpc/platforms/4xx/machine_check.c
> @@ -10,7 +10,7 @@
>
> int machine_check_4xx(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
>
> if (reason & ESR_IMCP) {
> printk("Instruction");
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-05 10:06 ` Christophe Leroy
0 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-05 10:06 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin, linux-kernel,
efremov, paulus, aneesh.kumar, peterx, akpm, sandipan
Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dsisr and esr regsiters, we can reference
> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dsisr. This makes code more clear.
I'm not sure it is worth doing that.
What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
the same:
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> +
Or even
> - int is_write = page_fault_is_write(regs->dsisr);
> + unsigned long err_reg;
> + int is_write;
> +
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err_reg = regs->esr;
> + else
> + err_reg = regs->dsisr;
> +
> + is_write = page_fault_is_write(err_reg);
Artificially growing the code for that makes no sense to me.
To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
that we know it represents the status register, which is DSISR or ESR depending on the platform.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 2 +-
> arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> 8 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 3e5d470a6155..c252d04b1206 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -44,7 +44,10 @@ struct pt_regs
> #endif
> unsigned long trap;
> unsigned long dar;
> - unsigned long dsisr;
> + union {
> + unsigned long dsisr;
> + unsigned long esr;
> + };
> unsigned long result;
> };
> };
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index 7004cfea3f5f..e357288b5f34 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -53,7 +53,10 @@ struct pt_regs
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> unsigned long dar; /* Fault registers */
> - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> + union {
> + unsigned long dsisr; /* on Book-S used for DSISR */
> + unsigned long esr; /* on 4xx/Book-E used for ESR */
> + };
> unsigned long result; /* Result of a system call */
> };
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 185beb290580..f74af8f9133c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 0a0a33eb0d28..00789ad2c4a3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, dar));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> + offsetof(struct user_pt_regs, esr));
> BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> offsetof(struct user_pt_regs, result));
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index dfbce527c98e..2164f5705a0b 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> /* On 4xx, the reason for the machine check or program exception
> is in the ESR. */
> -#define get_reason(regs) ((regs)->dsisr)
> +#define get_reason(regs) ((regs)->esr)
> #define REASON_FP ESR_FP
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a8d0ce85d39a..62953d4e7c93 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> {
> long err;
>
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> +
> if (unlikely(err))
> bad_page_fault(regs, err);
> }
> @@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
> */
> static void __bad_page_fault(struct pt_regs *regs, int sig)
> {
> - int is_write = page_fault_is_write(regs->dsisr);
> + unsigned long err_reg;
> + int is_write;
> +
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err_reg = regs->esr;
> + else
> + err_reg = regs->dsisr;
> +
> + is_write = page_fault_is_write(err_reg);
>
> /* kernel has accessed a bad area */
>
> diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
> index a5c898bb9bab..5d19daacd78a 100644
> --- a/arch/powerpc/platforms/44x/machine_check.c
> +++ b/arch/powerpc/platforms/44x/machine_check.c
> @@ -11,7 +11,7 @@
>
> int machine_check_440A(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
>
> printk("Machine check in kernel mode.\n");
> if (reason & ESR_IMCP){
> @@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
> #ifdef CONFIG_PPC_47x
> int machine_check_47x(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
> u32 mcsr;
>
> printk(KERN_ERR "Machine check in kernel mode.\n");
> diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
> index a71c29892a91..a905da1d6f41 100644
> --- a/arch/powerpc/platforms/4xx/machine_check.c
> +++ b/arch/powerpc/platforms/4xx/machine_check.c
> @@ -10,7 +10,7 @@
>
> int machine_check_4xx(struct pt_regs *regs)
> {
> - unsigned long reason = regs->dsisr;
> + unsigned long reason = regs->esr;
>
> if (reason & ESR_IMCP) {
> printk("Instruction");
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
2021-07-26 14:30 ` sxwjean
@ 2021-08-05 10:09 ` Christophe Leroy
-1 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-05 10:09 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: oleg, mpe, benh, paulus, ravi.bangoria, npiggin, aneesh.kumar,
sandipan, efremov, peterx, akpm, linux-kernel, Xiongwei Song
Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dar and dear regsiters, we can reference
> dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dar. This makes code more clear.
Same comment here as for patch 1.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 5 ++++-
> arch/powerpc/mm/fault.c | 2 +-
> 6 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index c252d04b1206..fa725e3238c2 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -43,7 +43,10 @@ struct pt_regs
> unsigned long mq;
> #endif
> unsigned long trap;
> - unsigned long dar;
> + union {
> + unsigned long dar;
> + unsigned long dear;
> + };
> union {
> unsigned long dsisr;
> unsigned long esr;
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index e357288b5f34..9ae150fb4c4b 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -52,7 +52,10 @@ struct pt_regs
> unsigned long trap; /* Reason for being here */
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> - unsigned long dar; /* Fault registers */
> + union {
> + unsigned long dar; /* Fault registers */
> + unsigned long dear;
> + };
> union {
> unsigned long dsisr; /* on Book-S used for DSISR */
> unsigned long esr; /* on 4xx/Book-E used for ESR */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f74af8f9133c..50436b52c213 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 00789ad2c4a3..969dca8b0718 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -373,6 +373,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, trap));
> BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
> offsetof(struct user_pt_regs, dar));
> + BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
> + offsetof(struct user_pt_regs, dear));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 2164f5705a0b..0796630d3d23 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
> }
> bad:
> if (user_mode(regs))
> - _exception(sig, regs, code, regs->dar);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + _exception(sig, regs, code, regs->dear);
> + else
> + _exception(sig, regs, code, regs->dar);
> else
> bad_page_fault(regs, sig);
> }
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 62953d4e7c93..3db6b39a1178 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> long err;
>
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - err = ___do_page_fault(regs, regs->dar, regs->esr);
> + err = ___do_page_fault(regs, regs->dear, regs->esr);
> else
> err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
@ 2021-08-05 10:09 ` Christophe Leroy
0 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-05 10:09 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin, linux-kernel,
efremov, paulus, aneesh.kumar, peterx, akpm, sandipan
Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dar and dear regsiters, we can reference
> dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dar. This makes code more clear.
Same comment here as for patch 1.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 5 ++++-
> arch/powerpc/mm/fault.c | 2 +-
> 6 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index c252d04b1206..fa725e3238c2 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -43,7 +43,10 @@ struct pt_regs
> unsigned long mq;
> #endif
> unsigned long trap;
> - unsigned long dar;
> + union {
> + unsigned long dar;
> + unsigned long dear;
> + };
> union {
> unsigned long dsisr;
> unsigned long esr;
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index e357288b5f34..9ae150fb4c4b 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -52,7 +52,10 @@ struct pt_regs
> unsigned long trap; /* Reason for being here */
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> - unsigned long dar; /* Fault registers */
> + union {
> + unsigned long dar; /* Fault registers */
> + unsigned long dear;
> + };
> union {
> unsigned long dsisr; /* on Book-S used for DSISR */
> unsigned long esr; /* on 4xx/Book-E used for ESR */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f74af8f9133c..50436b52c213 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 00789ad2c4a3..969dca8b0718 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -373,6 +373,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, trap));
> BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
> offsetof(struct user_pt_regs, dar));
> + BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
> + offsetof(struct user_pt_regs, dear));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 2164f5705a0b..0796630d3d23 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
> }
> bad:
> if (user_mode(regs))
> - _exception(sig, regs, code, regs->dar);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + _exception(sig, regs, code, regs->dear);
> + else
> + _exception(sig, regs, code, regs->dar);
> else
> bad_page_fault(regs, sig);
> }
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 62953d4e7c93..3db6b39a1178 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> long err;
>
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - err = ___do_page_fault(regs, regs->dar, regs->esr);
> + err = ___do_page_fault(regs, regs->dear, regs->esr);
> else
> err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-08-05 10:06 ` Christophe Leroy
@ 2021-08-06 3:16 ` Xiongwei Song
-1 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 3:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: Xiongwei Song, PowerPC, oleg, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, ravi.bangoria, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm,
Linux Kernel Mailing List
On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dsisr and esr regsiters, we can reference
> > esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dsisr. This makes code more clear.
>
> I'm not sure it is worth doing that.
Why don't we use "esr" as reference manauls mentioned?
>
> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
> the same:
>
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > +
Yes, we can drop this. But it's a bit vague.
> Or even
>
> > - int is_write = page_fault_is_write(regs->dsisr);
> > + unsigned long err_reg;
> > + int is_write;
> > +
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err_reg = regs->esr;
> > + else
> > + err_reg = regs->dsisr;
> > +
> > + is_write = page_fault_is_write(err_reg);
>
>
> Artificially growing the code for that makes no sense to me.
We can drop this too.
>
>
> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
> that we know it represents the status register, which is DSISR or ESR depending on the platform.
If so, this would make other people more confused. My consideration is
to follow what the reference
manuals represent.
Thank you so much for your comments.
Regards,
Xiongwei
>
>
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 2 +-
> > arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> > arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> > arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> > 8 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..c252d04b1206 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -44,7 +44,10 @@ struct pt_regs
> > #endif
> > unsigned long trap;
> > unsigned long dar;
> > - unsigned long dsisr;
> > + union {
> > + unsigned long dsisr;
> > + unsigned long esr;
> > + };
> > unsigned long result;
> > };
> > };
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index 7004cfea3f5f..e357288b5f34 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -53,7 +53,10 @@ struct pt_regs
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > unsigned long dar; /* Fault registers */
> > - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> > + union {
> > + unsigned long dsisr; /* on Book-S used for DSISR */
> > + unsigned long esr; /* on 4xx/Book-E used for ESR */
> > + };
> > unsigned long result; /* Result of a system call */
> > };
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 185beb290580..f74af8f9133c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 0a0a33eb0d28..00789ad2c4a3 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, dar));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > + offsetof(struct user_pt_regs, esr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> > offsetof(struct user_pt_regs, result));
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index dfbce527c98e..2164f5705a0b 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> > /* On 4xx, the reason for the machine check or program exception
> > is in the ESR. */
> > -#define get_reason(regs) ((regs)->dsisr)
> > +#define get_reason(regs) ((regs)->esr)
> > #define REASON_FP ESR_FP
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a8d0ce85d39a..62953d4e7c93 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > {
> > long err;
> >
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > +
> > if (unlikely(err))
> > bad_page_fault(regs, err);
> > }
> > @@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
> > */
> > static void __bad_page_fault(struct pt_regs *regs, int sig)
> > {
> > - int is_write = page_fault_is_write(regs->dsisr);
> > + unsigned long err_reg;
> > + int is_write;
> > +
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err_reg = regs->esr;
> > + else
> > + err_reg = regs->dsisr;
> > +
> > + is_write = page_fault_is_write(err_reg);
> >
> > /* kernel has accessed a bad area */
> >
> > diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
> > index a5c898bb9bab..5d19daacd78a 100644
> > --- a/arch/powerpc/platforms/44x/machine_check.c
> > +++ b/arch/powerpc/platforms/44x/machine_check.c
> > @@ -11,7 +11,7 @@
> >
> > int machine_check_440A(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> >
> > printk("Machine check in kernel mode.\n");
> > if (reason & ESR_IMCP){
> > @@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_47x
> > int machine_check_47x(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> > u32 mcsr;
> >
> > printk(KERN_ERR "Machine check in kernel mode.\n");
> > diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
> > index a71c29892a91..a905da1d6f41 100644
> > --- a/arch/powerpc/platforms/4xx/machine_check.c
> > +++ b/arch/powerpc/platforms/4xx/machine_check.c
> > @@ -10,7 +10,7 @@
> >
> > int machine_check_4xx(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> >
> > if (reason & ESR_IMCP) {
> > printk("Instruction");
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 3:16 ` Xiongwei Song
0 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 3:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin,
Linux Kernel Mailing List, efremov, Paul Mackerras, aneesh.kumar,
peterx, PowerPC, akpm, sandipan
On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dsisr and esr regsiters, we can reference
> > esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dsisr. This makes code more clear.
>
> I'm not sure it is worth doing that.
Why don't we use "esr" as reference manauls mentioned?
>
> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
> the same:
>
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > +
Yes, we can drop this. But it's a bit vague.
> Or even
>
> > - int is_write = page_fault_is_write(regs->dsisr);
> > + unsigned long err_reg;
> > + int is_write;
> > +
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err_reg = regs->esr;
> > + else
> > + err_reg = regs->dsisr;
> > +
> > + is_write = page_fault_is_write(err_reg);
>
>
> Artificially growing the code for that makes no sense to me.
We can drop this too.
>
>
> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
> that we know it represents the status register, which is DSISR or ESR depending on the platform.
If so, this would make other people more confused. My consideration is
to follow what the reference
manuals represent.
Thank you so much for your comments.
Regards,
Xiongwei
>
>
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 2 +-
> > arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> > arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> > arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> > 8 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..c252d04b1206 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -44,7 +44,10 @@ struct pt_regs
> > #endif
> > unsigned long trap;
> > unsigned long dar;
> > - unsigned long dsisr;
> > + union {
> > + unsigned long dsisr;
> > + unsigned long esr;
> > + };
> > unsigned long result;
> > };
> > };
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index 7004cfea3f5f..e357288b5f34 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -53,7 +53,10 @@ struct pt_regs
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > unsigned long dar; /* Fault registers */
> > - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> > + union {
> > + unsigned long dsisr; /* on Book-S used for DSISR */
> > + unsigned long esr; /* on 4xx/Book-E used for ESR */
> > + };
> > unsigned long result; /* Result of a system call */
> > };
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 185beb290580..f74af8f9133c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 0a0a33eb0d28..00789ad2c4a3 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, dar));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > + offsetof(struct user_pt_regs, esr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> > offsetof(struct user_pt_regs, result));
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index dfbce527c98e..2164f5705a0b 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> > /* On 4xx, the reason for the machine check or program exception
> > is in the ESR. */
> > -#define get_reason(regs) ((regs)->dsisr)
> > +#define get_reason(regs) ((regs)->esr)
> > #define REASON_FP ESR_FP
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a8d0ce85d39a..62953d4e7c93 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > {
> > long err;
> >
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > +
> > if (unlikely(err))
> > bad_page_fault(regs, err);
> > }
> > @@ -567,7 +571,15 @@ NOKPROBE_SYMBOL(hash__do_page_fault);
> > */
> > static void __bad_page_fault(struct pt_regs *regs, int sig)
> > {
> > - int is_write = page_fault_is_write(regs->dsisr);
> > + unsigned long err_reg;
> > + int is_write;
> > +
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err_reg = regs->esr;
> > + else
> > + err_reg = regs->dsisr;
> > +
> > + is_write = page_fault_is_write(err_reg);
> >
> > /* kernel has accessed a bad area */
> >
> > diff --git a/arch/powerpc/platforms/44x/machine_check.c b/arch/powerpc/platforms/44x/machine_check.c
> > index a5c898bb9bab..5d19daacd78a 100644
> > --- a/arch/powerpc/platforms/44x/machine_check.c
> > +++ b/arch/powerpc/platforms/44x/machine_check.c
> > @@ -11,7 +11,7 @@
> >
> > int machine_check_440A(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> >
> > printk("Machine check in kernel mode.\n");
> > if (reason & ESR_IMCP){
> > @@ -48,7 +48,7 @@ int machine_check_440A(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_47x
> > int machine_check_47x(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> > u32 mcsr;
> >
> > printk(KERN_ERR "Machine check in kernel mode.\n");
> > diff --git a/arch/powerpc/platforms/4xx/machine_check.c b/arch/powerpc/platforms/4xx/machine_check.c
> > index a71c29892a91..a905da1d6f41 100644
> > --- a/arch/powerpc/platforms/4xx/machine_check.c
> > +++ b/arch/powerpc/platforms/4xx/machine_check.c
> > @@ -10,7 +10,7 @@
> >
> > int machine_check_4xx(struct pt_regs *regs)
> > {
> > - unsigned long reason = regs->dsisr;
> > + unsigned long reason = regs->esr;
> >
> > if (reason & ESR_IMCP) {
> > printk("Instruction");
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
2021-08-05 10:09 ` Christophe Leroy
@ 2021-08-06 3:16 ` Xiongwei Song
-1 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 3:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: Xiongwei Song, PowerPC, oleg, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, ravi.bangoria, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm,
Linux Kernel Mailing List
On Thu, Aug 5, 2021 at 6:09 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dar and dear regsiters, we can reference
> > dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dar. This makes code more clear.
>
> Same comment here as for patch 1.
Same reply for the patch 1.
Thank you.
>
>
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 5 ++++-
> > arch/powerpc/mm/fault.c | 2 +-
> > 6 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index c252d04b1206..fa725e3238c2 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -43,7 +43,10 @@ struct pt_regs
> > unsigned long mq;
> > #endif
> > unsigned long trap;
> > - unsigned long dar;
> > + union {
> > + unsigned long dar;
> > + unsigned long dear;
> > + };
> > union {
> > unsigned long dsisr;
> > unsigned long esr;
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index e357288b5f34..9ae150fb4c4b 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -52,7 +52,10 @@ struct pt_regs
> > unsigned long trap; /* Reason for being here */
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > - unsigned long dar; /* Fault registers */
> > + union {
> > + unsigned long dar; /* Fault registers */
> > + unsigned long dear;
> > + };
> > union {
> > unsigned long dsisr; /* on Book-S used for DSISR */
> > unsigned long esr; /* on 4xx/Book-E used for ESR */
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index f74af8f9133c..50436b52c213 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 00789ad2c4a3..969dca8b0718 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -373,6 +373,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, trap));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
> > offsetof(struct user_pt_regs, dar));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
> > + offsetof(struct user_pt_regs, dear));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index 2164f5705a0b..0796630d3d23 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
> > }
> > bad:
> > if (user_mode(regs))
> > - _exception(sig, regs, code, regs->dar);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + _exception(sig, regs, code, regs->dear);
> > + else
> > + _exception(sig, regs, code, regs->dar);
> > else
> > bad_page_fault(regs, sig);
> > }
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 62953d4e7c93..3db6b39a1178 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > long err;
> >
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + err = ___do_page_fault(regs, regs->dear, regs->esr);
> > else
> > err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 3/4] powerpc: Optimize register usage for dear register
@ 2021-08-06 3:16 ` Xiongwei Song
0 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 3:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin,
Linux Kernel Mailing List, efremov, Paul Mackerras, aneesh.kumar,
peterx, PowerPC, akpm, sandipan
On Thu, Aug 5, 2021 at 6:09 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dar and dear regsiters, we can reference
> > dear to get the effective address when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dar. This makes code more clear.
>
> Same comment here as for patch 1.
Same reply for the patch 1.
Thank you.
>
>
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 5 ++++-
> > arch/powerpc/mm/fault.c | 2 +-
> > 6 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index c252d04b1206..fa725e3238c2 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -43,7 +43,10 @@ struct pt_regs
> > unsigned long mq;
> > #endif
> > unsigned long trap;
> > - unsigned long dar;
> > + union {
> > + unsigned long dar;
> > + unsigned long dear;
> > + };
> > union {
> > unsigned long dsisr;
> > unsigned long esr;
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index e357288b5f34..9ae150fb4c4b 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -52,7 +52,10 @@ struct pt_regs
> > unsigned long trap; /* Reason for being here */
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > - unsigned long dar; /* Fault registers */
> > + union {
> > + unsigned long dar; /* Fault registers */
> > + unsigned long dear;
> > + };
> > union {
> > unsigned long dsisr; /* on Book-S used for DSISR */
> > unsigned long esr; /* on 4xx/Book-E used for ESR */
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index f74af8f9133c..50436b52c213 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dear, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 00789ad2c4a3..969dca8b0718 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -373,6 +373,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, trap));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dar) !=
> > offsetof(struct user_pt_regs, dar));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, dear) !=
> > + offsetof(struct user_pt_regs, dear));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index 2164f5705a0b..0796630d3d23 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -1609,7 +1609,10 @@ DEFINE_INTERRUPT_HANDLER(alignment_exception)
> > }
> > bad:
> > if (user_mode(regs))
> > - _exception(sig, regs, code, regs->dar);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + _exception(sig, regs, code, regs->dear);
> > + else
> > + _exception(sig, regs, code, regs->dar);
> > else
> > bad_page_fault(regs, sig);
> > }
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 62953d4e7c93..3db6b39a1178 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -542,7 +542,7 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > long err;
> >
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + err = ___do_page_fault(regs, regs->dear, regs->esr);
> > else
> > err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-07-26 14:30 ` sxwjean
@ 2021-08-06 6:53 ` Michael Ellerman
-1 siblings, 0 replies; 26+ messages in thread
From: Michael Ellerman @ 2021-08-06 6:53 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: oleg, benh, paulus, ravi.bangoria, christophe.leroy, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm, linux-kernel,
Xiongwei Song
sxwjean@me.com writes:
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dsisr and esr regsiters, we can reference
> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dsisr. This makes code more clear.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 2 +-
> arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> 8 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 3e5d470a6155..c252d04b1206 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -44,7 +44,10 @@ struct pt_regs
> #endif
> unsigned long trap;
> unsigned long dar;
> - unsigned long dsisr;
> + union {
> + unsigned long dsisr;
> + unsigned long esr;
> + };
I don't mind doing that.
> unsigned long result;
> };
> };
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index 7004cfea3f5f..e357288b5f34 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -53,7 +53,10 @@ struct pt_regs
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> unsigned long dar; /* Fault registers */
> - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> + union {
> + unsigned long dsisr; /* on Book-S used for DSISR */
> + unsigned long esr; /* on 4xx/Book-E used for ESR */
> + };
> unsigned long result; /* Result of a system call */
> };
But I'm not sure about the use of anonymous unions in UAPI headers. Old
compilers don't support them, so there's a risk of breakage.
I'd rather we didn't touch the uapi version.
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 185beb290580..f74af8f9133c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 0a0a33eb0d28..00789ad2c4a3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, dar));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> + offsetof(struct user_pt_regs, esr));
> BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> offsetof(struct user_pt_regs, result));
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index dfbce527c98e..2164f5705a0b 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> /* On 4xx, the reason for the machine check or program exception
> is in the ESR. */
> -#define get_reason(regs) ((regs)->dsisr)
> +#define get_reason(regs) ((regs)->esr)
> #define REASON_FP ESR_FP
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a8d0ce85d39a..62953d4e7c93 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> {
> long err;
>
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
As Christophe said, I don't thinks this is an improvement.
It makes the code less readable. If anyone is confused about what is
passed to ___do_page_fault() they can either read the comment above it,
or look at the definition of pt_regs to see that esr and dsisr share
storage.
cheers
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 6:53 ` Michael Ellerman
0 siblings, 0 replies; 26+ messages in thread
From: Michael Ellerman @ 2021-08-06 6:53 UTC (permalink / raw)
To: sxwjean, linuxppc-dev
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin, linux-kernel,
efremov, paulus, aneesh.kumar, peterx, akpm, sandipan
sxwjean@me.com writes:
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create an anonymous union for dsisr and esr regsiters, we can reference
> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> Otherwise, reference dsisr. This makes code more clear.
>
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
> arch/powerpc/include/asm/ptrace.h | 5 ++++-
> arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> arch/powerpc/kernel/process.c | 2 +-
> arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> arch/powerpc/kernel/traps.c | 2 +-
> arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> 8 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 3e5d470a6155..c252d04b1206 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -44,7 +44,10 @@ struct pt_regs
> #endif
> unsigned long trap;
> unsigned long dar;
> - unsigned long dsisr;
> + union {
> + unsigned long dsisr;
> + unsigned long esr;
> + };
I don't mind doing that.
> unsigned long result;
> };
> };
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index 7004cfea3f5f..e357288b5f34 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -53,7 +53,10 @@ struct pt_regs
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> unsigned long dar; /* Fault registers */
> - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> + union {
> + unsigned long dsisr; /* on Book-S used for DSISR */
> + unsigned long esr; /* on 4xx/Book-E used for ESR */
> + };
> unsigned long result; /* Result of a system call */
> };
But I'm not sure about the use of anonymous unions in UAPI headers. Old
compilers don't support them, so there's a risk of breakage.
I'd rather we didn't touch the uapi version.
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 185beb290580..f74af8f9133c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> trap == INTERRUPT_DATA_STORAGE ||
> trap == INTERRUPT_ALIGNMENT) {
> if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> else
> pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index 0a0a33eb0d28..00789ad2c4a3 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> offsetof(struct user_pt_regs, dar));
> BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> offsetof(struct user_pt_regs, dsisr));
> + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> + offsetof(struct user_pt_regs, esr));
> BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> offsetof(struct user_pt_regs, result));
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index dfbce527c98e..2164f5705a0b 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> /* On 4xx, the reason for the machine check or program exception
> is in the ESR. */
> -#define get_reason(regs) ((regs)->dsisr)
> +#define get_reason(regs) ((regs)->esr)
> #define REASON_FP ESR_FP
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a8d0ce85d39a..62953d4e7c93 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> {
> long err;
>
> - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> + err = ___do_page_fault(regs, regs->dar, regs->esr);
> + else
> + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
As Christophe said, I don't thinks this is an improvement.
It makes the code less readable. If anyone is confused about what is
passed to ___do_page_fault() they can either read the comment above it,
or look at the definition of pt_regs to see that esr and dsisr share
storage.
cheers
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-08-06 3:16 ` Xiongwei Song
@ 2021-08-06 7:32 ` Christophe Leroy
-1 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-06 7:32 UTC (permalink / raw)
To: Xiongwei Song
Cc: Xiongwei Song, PowerPC, oleg, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, ravi.bangoria, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm,
Linux Kernel Mailing List
Le 06/08/2021 à 05:16, Xiongwei Song a écrit :
> On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>>
>>
>> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>>
>>> Create an anonymous union for dsisr and esr regsiters, we can reference
>>> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
>>> Otherwise, reference dsisr. This makes code more clear.
>>
>> I'm not sure it is worth doing that.
> Why don't we use "esr" as reference manauls mentioned?
>
>>
>> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
>> the same:
>>
>> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
>> > + else
>> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>> > +
> Yes, we can drop this. But it's a bit vague.
>
>> Or even
>>
>> > - int is_write = page_fault_is_write(regs->dsisr);
>> > + unsigned long err_reg;
>> > + int is_write;
>> > +
>> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>> > + err_reg = regs->esr;
>> > + else
>> > + err_reg = regs->dsisr;
>> > +
>> > + is_write = page_fault_is_write(err_reg);
>>
>>
>> Artificially growing the code for that makes no sense to me.
>
> We can drop this too.
>>
>>
>> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
>> that we know it represents the status register, which is DSISR or ESR depending on the platform.
>
> If so, this would make other people more confused. My consideration is
> to follow what the reference
> manuals represent.
Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear
That would be more explicit for everyone.
The UAPI header however should remain as is because anonymous unions are not supported by old
compilers as mentioned by Michael.
But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have
in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1
register into regs->dsisr.
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 7:32 ` Christophe Leroy
0 siblings, 0 replies; 26+ messages in thread
From: Christophe Leroy @ 2021-08-06 7:32 UTC (permalink / raw)
To: Xiongwei Song
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin,
Linux Kernel Mailing List, efremov, Paul Mackerras, aneesh.kumar,
peterx, PowerPC, akpm, sandipan
Le 06/08/2021 à 05:16, Xiongwei Song a écrit :
> On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>>
>>
>> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>>
>>> Create an anonymous union for dsisr and esr regsiters, we can reference
>>> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
>>> Otherwise, reference dsisr. This makes code more clear.
>>
>> I'm not sure it is worth doing that.
> Why don't we use "esr" as reference manauls mentioned?
>
>>
>> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
>> the same:
>>
>> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
>> > + else
>> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>> > +
> Yes, we can drop this. But it's a bit vague.
>
>> Or even
>>
>> > - int is_write = page_fault_is_write(regs->dsisr);
>> > + unsigned long err_reg;
>> > + int is_write;
>> > +
>> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>> > + err_reg = regs->esr;
>> > + else
>> > + err_reg = regs->dsisr;
>> > +
>> > + is_write = page_fault_is_write(err_reg);
>>
>>
>> Artificially growing the code for that makes no sense to me.
>
> We can drop this too.
>>
>>
>> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
>> that we know it represents the status register, which is DSISR or ESR depending on the platform.
>
> If so, this would make other people more confused. My consideration is
> to follow what the reference
> manuals represent.
Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear
That would be more explicit for everyone.
The UAPI header however should remain as is because anonymous unions are not supported by old
compilers as mentioned by Michael.
But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have
in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1
register into regs->dsisr.
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-08-06 6:53 ` Michael Ellerman
@ 2021-08-06 13:14 ` Xiongwei Song
-1 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 13:14 UTC (permalink / raw)
To: Michael Ellerman
Cc: Xiongwei Song, PowerPC, oleg, Benjamin Herrenschmidt,
Paul Mackerras, ravi.bangoria, Christophe Leroy, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm,
Linux Kernel Mailing List
On Fri, Aug 6, 2021 at 2:53 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> sxwjean@me.com writes:
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dsisr and esr regsiters, we can reference
> > esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dsisr. This makes code more clear.
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 2 +-
> > arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> > arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> > arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> > 8 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..c252d04b1206 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -44,7 +44,10 @@ struct pt_regs
> > #endif
> > unsigned long trap;
> > unsigned long dar;
> > - unsigned long dsisr;
> > + union {
> > + unsigned long dsisr;
> > + unsigned long esr;
> > + };
>
> I don't mind doing that.
>
> > unsigned long result;
> > };
> > };
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index 7004cfea3f5f..e357288b5f34 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -53,7 +53,10 @@ struct pt_regs
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > unsigned long dar; /* Fault registers */
> > - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> > + union {
> > + unsigned long dsisr; /* on Book-S used for DSISR */
> > + unsigned long esr; /* on 4xx/Book-E used for ESR */
> > + };
> > unsigned long result; /* Result of a system call */
> > };
>
> But I'm not sure about the use of anonymous unions in UAPI headers. Old
> compilers don't support them, so there's a risk of breakage.
>
> I'd rather we didn't touch the uapi version.
Ok. Will discard the change.
>
>
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 185beb290580..f74af8f9133c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 0a0a33eb0d28..00789ad2c4a3 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, dar));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > + offsetof(struct user_pt_regs, esr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> > offsetof(struct user_pt_regs, result));
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index dfbce527c98e..2164f5705a0b 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> > /* On 4xx, the reason for the machine check or program exception
> > is in the ESR. */
> > -#define get_reason(regs) ((regs)->dsisr)
> > +#define get_reason(regs) ((regs)->esr)
> > #define REASON_FP ESR_FP
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a8d0ce85d39a..62953d4e7c93 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > {
> > long err;
> >
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
> As Christophe said, I don't thinks this is an improvement.
>
> It makes the code less readable. If anyone is confused about what is
> passed to ___do_page_fault() they can either read the comment above it,
> or look at the definition of pt_regs to see that esr and dsisr share
> storage.
Ok, thanks a lot. Will send v2.
Regards,
Xiongwei
>
> cheers
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 13:14 ` Xiongwei Song
0 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 13:14 UTC (permalink / raw)
To: Michael Ellerman
Cc: ravi.bangoria, Xiongwei Song, oleg, Linux Kernel Mailing List,
efremov, Paul Mackerras, npiggin, aneesh.kumar, peterx, PowerPC,
akpm, sandipan
On Fri, Aug 6, 2021 at 2:53 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> sxwjean@me.com writes:
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Create an anonymous union for dsisr and esr regsiters, we can reference
> > esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> > Otherwise, reference dsisr. This makes code more clear.
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> > arch/powerpc/include/asm/ptrace.h | 5 ++++-
> > arch/powerpc/include/uapi/asm/ptrace.h | 5 ++++-
> > arch/powerpc/kernel/process.c | 2 +-
> > arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> > arch/powerpc/kernel/traps.c | 2 +-
> > arch/powerpc/mm/fault.c | 16 ++++++++++++++--
> > arch/powerpc/platforms/44x/machine_check.c | 4 ++--
> > arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> > 8 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> > index 3e5d470a6155..c252d04b1206 100644
> > --- a/arch/powerpc/include/asm/ptrace.h
> > +++ b/arch/powerpc/include/asm/ptrace.h
> > @@ -44,7 +44,10 @@ struct pt_regs
> > #endif
> > unsigned long trap;
> > unsigned long dar;
> > - unsigned long dsisr;
> > + union {
> > + unsigned long dsisr;
> > + unsigned long esr;
> > + };
>
> I don't mind doing that.
>
> > unsigned long result;
> > };
> > };
> > diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> > index 7004cfea3f5f..e357288b5f34 100644
> > --- a/arch/powerpc/include/uapi/asm/ptrace.h
> > +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> > @@ -53,7 +53,10 @@ struct pt_regs
> > /* N.B. for critical exceptions on 4xx, the dar and dsisr
> > fields are overloaded to hold srr0 and srr1. */
> > unsigned long dar; /* Fault registers */
> > - unsigned long dsisr; /* on 4xx/Book-E used for ESR */
> > + union {
> > + unsigned long dsisr; /* on Book-S used for DSISR */
> > + unsigned long esr; /* on 4xx/Book-E used for ESR */
> > + };
> > unsigned long result; /* Result of a system call */
> > };
>
> But I'm not sure about the use of anonymous unions in UAPI headers. Old
> compilers don't support them, so there's a risk of breakage.
>
> I'd rather we didn't touch the uapi version.
Ok. Will discard the change.
>
>
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 185beb290580..f74af8f9133c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1499,7 +1499,7 @@ static void __show_regs(struct pt_regs *regs)
> > trap == INTERRUPT_DATA_STORAGE ||
> > trap == INTERRUPT_ALIGNMENT) {
> > if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > - pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
> > + pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->esr);
> > else
> > pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
> > }
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 0a0a33eb0d28..00789ad2c4a3 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -375,6 +375,8 @@ void __init pt_regs_check(void)
> > offsetof(struct user_pt_regs, dar));
> > BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> > offsetof(struct user_pt_regs, dsisr));
> > + BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> > + offsetof(struct user_pt_regs, esr));
> > BUILD_BUG_ON(offsetof(struct pt_regs, result) !=
> > offsetof(struct user_pt_regs, result));
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index dfbce527c98e..2164f5705a0b 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -562,7 +562,7 @@ static inline int check_io_access(struct pt_regs *regs)
> > #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> > /* On 4xx, the reason for the machine check or program exception
> > is in the ESR. */
> > -#define get_reason(regs) ((regs)->dsisr)
> > +#define get_reason(regs) ((regs)->esr)
> > #define REASON_FP ESR_FP
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a8d0ce85d39a..62953d4e7c93 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -541,7 +541,11 @@ static __always_inline void __do_page_fault(struct pt_regs *regs)
> > {
> > long err;
> >
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
> As Christophe said, I don't thinks this is an improvement.
>
> It makes the code less readable. If anyone is confused about what is
> passed to ___do_page_fault() they can either read the comment above it,
> or look at the definition of pt_regs to see that esr and dsisr share
> storage.
Ok, thanks a lot. Will send v2.
Regards,
Xiongwei
>
> cheers
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-08-06 7:32 ` Christophe Leroy
@ 2021-08-06 13:22 ` Xiongwei Song
-1 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 13:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: Xiongwei Song, PowerPC, oleg, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, ravi.bangoria, npiggin,
aneesh.kumar, sandipan, efremov, peterx, akpm,
Linux Kernel Mailing List
On Fri, Aug 6, 2021 at 3:32 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 06/08/2021 à 05:16, Xiongwei Song a écrit :
> > On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> >>
> >>
> >>
> >> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> >>> From: Xiongwei Song <sxwjean@gmail.com>
> >>>
> >>> Create an anonymous union for dsisr and esr regsiters, we can reference
> >>> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> >>> Otherwise, reference dsisr. This makes code more clear.
> >>
> >> I'm not sure it is worth doing that.
> > Why don't we use "esr" as reference manauls mentioned?
> >
> >>
> >> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
> >> the same:
> >>
> >> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> >> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> >> > + else
> >> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >> > +
> > Yes, we can drop this. But it's a bit vague.
> >
> >> Or even
> >>
> >> > - int is_write = page_fault_is_write(regs->dsisr);
> >> > + unsigned long err_reg;
> >> > + int is_write;
> >> > +
> >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> >> > + err_reg = regs->esr;
> >> > + else
> >> > + err_reg = regs->dsisr;
> >> > +
> >> > + is_write = page_fault_is_write(err_reg);
> >>
> >>
> >> Artificially growing the code for that makes no sense to me.
> >
> > We can drop this too.
> >>
> >>
> >> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
> >> that we know it represents the status register, which is DSISR or ESR depending on the platform.
> >
> > If so, this would make other people more confused. My consideration is
> > to follow what the reference
> > manuals represent.
>
> Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear
I still prefer my method.
>
> That would be more explicit for everyone.
>
> The UAPI header however should remain as is because anonymous unions are not supported by old
> compilers as mentioned by Michael.
Sure. Will update in v2.
>
> But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have
> in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1
> register into regs->dsisr.
Can I think my method has better expansibility here;-)?
Let me finish esr and dear first. Thank you for the reminder.
Regards,
Xiongwei
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 13:22 ` Xiongwei Song
0 siblings, 0 replies; 26+ messages in thread
From: Xiongwei Song @ 2021-08-06 13:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: ravi.bangoria, Xiongwei Song, oleg, npiggin,
Linux Kernel Mailing List, efremov, Paul Mackerras, aneesh.kumar,
peterx, PowerPC, akpm, sandipan
On Fri, Aug 6, 2021 at 3:32 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 06/08/2021 à 05:16, Xiongwei Song a écrit :
> > On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> >>
> >>
> >>
> >> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :
> >>> From: Xiongwei Song <sxwjean@gmail.com>
> >>>
> >>> Create an anonymous union for dsisr and esr regsiters, we can reference
> >>> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
> >>> Otherwise, reference dsisr. This makes code more clear.
> >>
> >> I'm not sure it is worth doing that.
> > Why don't we use "esr" as reference manauls mentioned?
> >
> >>
> >> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
> >> the same:
> >>
> >> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> >> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> >> > + else
> >> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> >> > +
> > Yes, we can drop this. But it's a bit vague.
> >
> >> Or even
> >>
> >> > - int is_write = page_fault_is_write(regs->dsisr);
> >> > + unsigned long err_reg;
> >> > + int is_write;
> >> > +
> >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> >> > + err_reg = regs->esr;
> >> > + else
> >> > + err_reg = regs->dsisr;
> >> > +
> >> > + is_write = page_fault_is_write(err_reg);
> >>
> >>
> >> Artificially growing the code for that makes no sense to me.
> >
> > We can drop this too.
> >>
> >>
> >> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
> >> that we know it represents the status register, which is DSISR or ESR depending on the platform.
> >
> > If so, this would make other people more confused. My consideration is
> > to follow what the reference
> > manuals represent.
>
> Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear
I still prefer my method.
>
> That would be more explicit for everyone.
>
> The UAPI header however should remain as is because anonymous unions are not supported by old
> compilers as mentioned by Michael.
Sure. Will update in v2.
>
> But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have
> in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1
> register into regs->dsisr.
Can I think my method has better expansibility here;-)?
Let me finish esr and dear first. Thank you for the reminder.
Regards,
Xiongwei
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
2021-08-06 6:53 ` Michael Ellerman
@ 2021-08-06 14:26 ` Segher Boessenkool
-1 siblings, 0 replies; 26+ messages in thread
From: Segher Boessenkool @ 2021-08-06 14:26 UTC (permalink / raw)
To: Michael Ellerman
Cc: sxwjean, linuxppc-dev, ravi.bangoria, Xiongwei Song, oleg,
npiggin, linux-kernel, efremov, paulus, aneesh.kumar, peterx,
akpm, sandipan
On Fri, Aug 06, 2021 at 04:53:14PM +1000, Michael Ellerman wrote:
> But I'm not sure about the use of anonymous unions in UAPI headers. Old
> compilers don't support them, so there's a risk of breakage.
More precisely, it exists only since C11, so even with all not-so-ancient
compilers it will not work if the user uses (say) -std=c99, which still
is popular.
> I'd rather we didn't touch the uapi version.
Yeah.
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
> As Christophe said, I don't thinks this is an improvement.
>
> It makes the code less readable. If anyone is confused about what is
> passed to ___do_page_fault() they can either read the comment above it,
> or look at the definition of pt_regs to see that esr and dsisr share
> storage.
Esp. since the affected platforms are legacy, yup.
Segher
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
@ 2021-08-06 14:26 ` Segher Boessenkool
0 siblings, 0 replies; 26+ messages in thread
From: Segher Boessenkool @ 2021-08-06 14:26 UTC (permalink / raw)
To: Michael Ellerman
Cc: ravi.bangoria, sxwjean, Xiongwei Song, aneesh.kumar, oleg,
npiggin, linux-kernel, peterx, paulus, efremov, akpm,
linuxppc-dev, sandipan
On Fri, Aug 06, 2021 at 04:53:14PM +1000, Michael Ellerman wrote:
> But I'm not sure about the use of anonymous unions in UAPI headers. Old
> compilers don't support them, so there's a risk of breakage.
More precisely, it exists only since C11, so even with all not-so-ancient
compilers it will not work if the user uses (say) -std=c99, which still
is popular.
> I'd rather we didn't touch the uapi version.
Yeah.
> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
> > + err = ___do_page_fault(regs, regs->dar, regs->esr);
> > + else
> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr);
>
> As Christophe said, I don't thinks this is an improvement.
>
> It makes the code less readable. If anyone is confused about what is
> passed to ___do_page_fault() they can either read the comment above it,
> or look at the definition of pt_regs to see that esr and dsisr share
> storage.
Esp. since the affected platforms are legacy, yup.
Segher
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-08-06 14:34 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 14:30 [RFC PATCH 1/4] powerpc: Optimize register usage for esr register sxwjean
2021-07-26 14:30 ` sxwjean
2021-07-26 14:30 ` [RFC PATCH 2/4] powerpc/64e: Get esr offset with _ESR macro sxwjean
2021-07-26 14:30 ` sxwjean
2021-07-26 14:30 ` [RFC PATCH 3/4] powerpc: Optimize register usage for dear register sxwjean
2021-07-26 14:30 ` sxwjean
2021-08-05 10:09 ` Christophe Leroy
2021-08-05 10:09 ` Christophe Leroy
2021-08-06 3:16 ` Xiongwei Song
2021-08-06 3:16 ` Xiongwei Song
2021-07-26 14:30 ` [RFC PATCH 4/4] powerpc/64e: Get dear offset with _DEAR macro sxwjean
2021-07-26 14:30 ` sxwjean
2021-08-05 10:06 ` [RFC PATCH 1/4] powerpc: Optimize register usage for esr register Christophe Leroy
2021-08-05 10:06 ` Christophe Leroy
2021-08-06 3:16 ` Xiongwei Song
2021-08-06 3:16 ` Xiongwei Song
2021-08-06 7:32 ` Christophe Leroy
2021-08-06 7:32 ` Christophe Leroy
2021-08-06 13:22 ` Xiongwei Song
2021-08-06 13:22 ` Xiongwei Song
2021-08-06 6:53 ` Michael Ellerman
2021-08-06 6:53 ` Michael Ellerman
2021-08-06 13:14 ` Xiongwei Song
2021-08-06 13:14 ` Xiongwei Song
2021-08-06 14:26 ` Segher Boessenkool
2021-08-06 14:26 ` Segher Boessenkool
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.