Re: [PATCH v2 3/4] powerpc: Optimize register usage for dear register

2021-08-10 Thread Xiongwei Song 
On Sat, Aug 7, 2021 at 2:58 PM Christophe Leroy
 wrote:
>
>
>
> Le 07/08/2021 à 03:02, sxwj...@me.com a écrit :
> > From: Xiongwei Song 
> >
> > 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 
> > ---
> >   arch/powerpc/include/asm/ptrace.h   | 5 -
> >   arch/powerpc/kernel/process.c   | 2 +-
> >   arch/powerpc/kernel/ptrace/ptrace.c | 2 ++
> >   3 files changed, 7 insertions(+), 2 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/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 a222fd4d6334..7c7093c17c45 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, dar));
>
> dar and dear are the same, so checking the same thing a second time is 
> pointless.

Same reply as the patch 1.

Regards,
Xiongwei

>
> >   BUILD_BUG_ON(offsetof(struct pt_regs, dsisr) !=
> >offsetof(struct user_pt_regs, dsisr));
> >   BUILD_BUG_ON(offsetof(struct pt_regs, esr) !=
> >

Re: [PATCH v2 1/4] powerpc: Optimize register usage for esr register

2021-08-10 Thread Xiongwei Song 
On Sat, Aug 7, 2021 at 2:57 PM Christophe Leroy
 wrote:
>
>
>
> Le 07/08/2021 à 03:02, sxwj...@me.com a écrit :
> > From: Xiongwei Song 
> >
> > 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 
> > ---
> >   arch/powerpc/include/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/platforms/44x/machine_check.c | 4 ++--
> >   arch/powerpc/platforms/4xx/machine_check.c | 2 +-
> >   6 files changed, 11 insertions(+), 6 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/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..a222fd4d6334 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, dsisr));
>
> esr and dsisr are the same, so checking the same thing a second time is 
> pointless.

Hmm...it's better to check if the changes on pt_regs structure is
expected I think.

Regards,
Xiongwei

>
> >   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/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){
> >

Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register

2021-08-06 Thread Xiongwei Song 
On Fri, Aug 6, 2021 at 3:32 PM Christophe Leroy
 wrote:
>
>
>
> Le 06/08/2021 à 05:16, Xiongwei Song a écrit :
> > On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
> >  wrote:
> >>
> >>
> >>
> >> Le 26/07/2021 à 16:30, sxwj...@me.com a écrit :
> >>> From: Xiongwei Song 
> >>>
> >>> 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

Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register

2021-08-06 Thread Xiongwei Song 
On Fri, Aug 6, 2021 at 2:53 PM Michael Ellerman  wrote:
>
> sxwj...@me.com writes:
> > From: Xiongwei Song 
> >
> > 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 
> > ---
> >  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 i

Re: [RFC PATCH 3/4] powerpc: Optimize register usage for dear register

2021-08-05 Thread Xiongwei Song 
On Thu, Aug 5, 2021 at 6:09 PM Christophe Leroy
 wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwj...@me.com a écrit :
> > From: Xiongwei Song 
> >
> > 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 
> > ---
> >   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, co

Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register

2021-08-05 Thread Xiongwei Song 
On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
 wrote:
>
>
>
> Le 26/07/2021 à 16:30, sxwj...@me.com a écrit :
> > From: Xiongwei Song 
> >
> > 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 
> > ---
> >   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, 
> > reg

Re: [PATCH 1/2] powerpc: Make the code in __show_regs nice-looking

2021-04-24 Thread Xiongwei Song 
On Thu, Apr 22, 2021 at 11:27 PM Christophe Leroy
 wrote:
>
>
>
> Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
> > From: Xiongwei Song 
> >
> > Create a new function named interrupt_detail_printable to judge which
> > interrupts can print esr/dsisr register.
>
> What is the benefit of that function ? It may be interesting if the test was 
> done at several places,
> but as it is done at only one place, I don't thing it is an improvement.
>
> Until know, you new immediately what was the traps that would print it. Now 
> you have to go and look
> into a sub-function.

How about replace if statement with switch statement directly, like
the changes below:

@@ -1467,13 +1481,17 @@ static void __show_regs(struct pt_regs *regs)
trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-   if (trap == INTERRUPT_MACHINE_CHECK ||
-   trap == INTERRUPT_DATA_STORAGE ||
-   trap == INTERRUPT_ALIGNMENT) {
+   switch(trap){
+   case INTERRUPT_MACHINE_CHECK:
+   case INTERRUPT_DATA_STORAGE:
+   case INTERRUPT_ALIGNMENT:
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar,
regs->dsisr);
else
pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar,
regs->dsisr);
+   break;
+   default:
+   break;
}

Thanks,
Xiongwei

Re: [PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt

2021-04-24 Thread Xiongwei Song 
On Fri, Apr 23, 2021 at 12:50 AM Christophe Leroy
 wrote:
>
>
>
> Le 22/04/2021 à 17:29, Christophe Leroy a écrit :
> >
> >
> > Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
> >> From: Xiongwei Song 
> >>
> >> The esr register has the details of Program Interrupt on BookE/4xx cpus,
> >> printing its value is helpful.
> >>
> >> Signed-off-by: Xiongwei Song 
> >> ---
> >>   arch/powerpc/kernel/process.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >> index 5c3830837f3a..664aecf8ee2e 100644
> >> --- a/arch/powerpc/kernel/process.c
> >> +++ b/arch/powerpc/kernel/process.c
> >> @@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
> >>   case INTERRUPT_MACHINE_CHECK:
> >>   case INTERRUPT_DATA_STORAGE:
> >>   case INTERRUPT_ALIGNMENT:
> >> +case INTERRUPT_PROGRAM:
> >
> > With this, it will also print the DSISR on 8xx/6xx so it will print garbage.
> >
> > 8xx/6xx provide the information in SRR1. If you want to proceed, you have 
> > to do the same as in ISI:
> > Copy the content of SRR1 into regs->dsisr in the assembly handler in 
> > head_book3s_32.S and in the
> > instruction TLB error handler in head_8xx.S
>
> In fact, we already have get_reason() called by do_program_check() to 
> retrieve the reason of the
> program check exception. Maybe it could be used to print usefull information 
> instead of starting
> doing almost the same is another way.

Yes, there is the get_reason() function. But if the program interrupt
is triggered in kernel mode,
the reason can be lost , see the code below:

335 static bool exception_common(int signr, struct pt_regs *regs, int code,
 336   unsigned long addr)
 337 {
 338 if (!user_mode(regs)) {
 339 die("Exception in kernel mode", regs, signr);
 340 return false;
 341 }

The third parameter(int code) of exception_common is to pass the
reason, when in kernel
mode, the "code" parameter is lost, hence I append INTERRUPT_PROGRAM here.

This is for __show_regs(), so just printing the content of the
register is fine I think.

>
> Or we do as I suggested above, and we remove that get_reason() stuff. But 
> get_reason() is also used
> by the alignment exception. So that doesn't look easy.
>
> I don't know what the best approach is.

Is it acceptable to print the interrupt reason before invoking die()
in exception_common()?

Regards,
Xiongwei

Re: [PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt

2021-04-24 Thread Xiongwei Song 
On Thu, Apr 22, 2021 at 11:29 PM Christophe Leroy
 wrote:
>
>
>
> Le 22/04/2021 à 17:10, Xiongwei Song a écrit :
> > From: Xiongwei Song 
> >
> > The esr register has the details of Program Interrupt on BookE/4xx cpus,
> > printing its value is helpful.
> >
> > Signed-off-by: Xiongwei Song 
> > ---
> >   arch/powerpc/kernel/process.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 5c3830837f3a..664aecf8ee2e 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
> >   case INTERRUPT_MACHINE_CHECK:
> >   case INTERRUPT_DATA_STORAGE:
> >   case INTERRUPT_ALIGNMENT:
> > + case INTERRUPT_PROGRAM:
>
> With this, it will also print the DSISR on 8xx/6xx so it will print garbage.
>
> 8xx/6xx provide the information in SRR1. If you want to proceed, you have to 
> do the same as in ISI:
> Copy the content of SRR1 into regs->dsisr in the assembly handler in 
> head_book3s_32.S and in the
> instruction TLB error handler in head_8xx.S

Good point.

[PATCH 2/2] powerpc: Print esr register when hitting Program Interrupt

2021-04-22 Thread Xiongwei Song 
From: Xiongwei Song 

The esr register has the details of Program Interrupt on BookE/4xx cpus,
printing its value is helpful.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/kernel/process.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5c3830837f3a..664aecf8ee2e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1459,6 +1459,7 @@ static bool interrupt_detail_printable(int trap)
case INTERRUPT_MACHINE_CHECK:
case INTERRUPT_DATA_STORAGE:
case INTERRUPT_ALIGNMENT:
+   case INTERRUPT_PROGRAM:
return true;
default:
return false;
-- 
2.17.1

[PATCH 1/2] powerpc: Make the code in __show_regs nice-looking

2021-04-22 Thread Xiongwei Song 
From: Xiongwei Song 

Create a new function named interrupt_detail_printable to judge which
interrupts can print esr/dsisr register.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/kernel/process.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 89e34aa273e2..5c3830837f3a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1453,9 +1453,23 @@ static void print_msr_bits(unsigned long val)
 #define REGS_PER_LINE  8
 #endif
 
+static bool interrupt_detail_printable(int trap)
+{
+   switch (trap) {
+   case INTERRUPT_MACHINE_CHECK:
+   case INTERRUPT_DATA_STORAGE:
+   case INTERRUPT_ALIGNMENT:
+   return true;
+   default:
+   return false;
+   }
+
+   return false;
+}
+
 static void __show_regs(struct pt_regs *regs)
 {
-   int i, trap;
+   int i;
 
printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
   regs->nip, regs->link, regs->ctr);
@@ -1464,12 +1478,9 @@ static void __show_regs(struct pt_regs *regs)
printk("MSR:  "REG" ", regs->msr);
print_msr_bits(regs->msr);
pr_cont("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
-   trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-   if (trap == INTERRUPT_MACHINE_CHECK ||
-   trap == INTERRUPT_DATA_STORAGE ||
-   trap == INTERRUPT_ALIGNMENT) {
+   if (interrupt_detail_printable(TRAP(regs))) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, 
regs->dsisr);
else
-- 
2.17.1

Re: [PATCH 1/3] powerpc/8xx: Enhance readability of trap types

2021-04-19 Thread Xiongwei Song 
On Mon, Apr 19, 2021 at 11:48 PM Christophe Leroy
 wrote:
>
> This patch makes use of trap types in head_8xx.S
>
> Signed-off-by: Christophe Leroy 
> ---
>  arch/powerpc/include/asm/interrupt.h | 29 
>  arch/powerpc/kernel/head_8xx.S   | 49 ++--
>  2 files changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h 
> b/arch/powerpc/include/asm/interrupt.h
> index ed2c4042c3d1..cf2c5c3ae716 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -2,13 +2,6 @@
>  #ifndef _ASM_POWERPC_INTERRUPT_H
>  #define _ASM_POWERPC_INTERRUPT_H
>
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
>  /* BookE/4xx */
>  #define INTERRUPT_CRITICAL_INPUT  0x100
>
> @@ -39,9 +32,11 @@
>  /* BookE/BookS/4xx/8xx */
>  #define INTERRUPT_DATA_STORAGE0x300
>  #define INTERRUPT_INST_STORAGE0x400
> +#define INTERRUPT_EXTERNAL 0x500
>  #define INTERRUPT_ALIGNMENT   0x600
>  #define INTERRUPT_PROGRAM 0x700
>  #define INTERRUPT_SYSCALL 0xc00
> +#define INTERRUPT_TRACE0xd00

The INTERRUPT_TRACE macro is defined in BookS section.
In BookE, 0xd00 stands for debug interrupt, so I defined it as
INTERRUPT_DEBUG.  I understand they are similar things,
but the terminologies are different in reference manuals.

Regards,
Xiongwei

Re: linux-next: build failure after merge of the powerpc tree

2021-04-19 Thread Xiongwei Song 
Thank you so much Stephen. Sorry for my negligence.

Should I fix this myself on powerpc tree?

Regards,
Xiongwei

On Mon, Apr 19, 2021 at 5:14 PM Stephen Rothwell  wrote:
>
> Hi all,
>
> After merging the powerpc tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> arch/powerpc/kernel/fadump.c: In function 'crash_fadump':
> arch/powerpc/kernel/fadump.c:731:28: error: 'INTERRUPT_SYSTEM_RESET' 
> undeclared (first use in this function)
>   731 |  if (TRAP(&(fdh->regs)) == INTERRUPT_SYSTEM_RESET) {
>   |^~
> arch/powerpc/kernel/fadump.c:731:28: note: each undeclared identifier is 
> reported only once for each function it appears in
>
> Caused by commit
>
>   7153d4bf0b37 ("powerpc/traps: Enhance readability for trap types")
>
> I have applied the following patch for today.
>
> From: Stephen Rothwell 
> Date: Mon, 19 Apr 2021 19:05:05 +1000
> Subject: [PATCH] fix up for "powerpc/traps: Enhance readability for trap 
> types"
>
> Signed-off-by: Stephen Rothwell 
> ---
>  arch/powerpc/kernel/fadump.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index b55b4c23f3b6..000e3b7f3fca 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  /*
>   * The CPU who acquired the lock to trigger the fadump crash should
> --
> 2.30.2
>
> --
> Cheers,
> Stephen Rothwell

[PATCH v5] powerpc/traps: Enhance readability for trap types

2021-04-14 Thread Xiongwei Song 
From: Xiongwei Song 

Define macros to list ppc interrupt types in interttupt.h, replace the
reference of the trap hex values with these macros.

Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S, arch/powerpc/kernel/head_*.S,
arch/powerpc/kernel/head_booke.h and arch/powerpc/include/asm/kvm_asm.h.

v4-v5:
* Delete unnecessary #ifdef.
* Move INTERRUPT_* macros to interttupt.h, classify for different cpu
  types. Drop traps.h.
* Directly define INTERRUPT_MACHINE_CHECK with 0x200.

v3-v4:
Fix compile issue:
arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' 
undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
Reported-by: kernel test robot 

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/include/asm/interrupt.h  | 48 +--
 arch/powerpc/kernel/fadump.c  |  2 +-
 arch/powerpc/kernel/interrupt.c   |  2 +-
 arch/powerpc/kernel/process.c |  4 ++-
 arch/powerpc/kernel/traps.c   |  6 ++--
 arch/powerpc/kexec/crash.c|  3 +-
 arch/powerpc/mm/book3s64/hash_utils.c |  4 +--
 arch/powerpc/mm/fault.c   | 16 -
 arch/powerpc/perf/core-book3s.c   |  5 +--
 arch/powerpc/xmon/xmon.c  | 20 +++
 10 files changed, 81 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/interrupt.h 
b/arch/powerpc/include/asm/interrupt.h
index 05e7fc4ffb50..86daf1242088 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -9,6 +9,46 @@
 #include 
 #include 
 
+/* BookE/4xx */
+#define INTERRUPT_CRITICAL_INPUT  0x100
+
+/* BookE */
+#define INTERRUPT_DEBUG   0xd00
+#ifdef CONFIG_BOOKE
+#define INTERRUPT_PERFMON 0x260
+#define INTERRUPT_DOORBELL0x280
+#endif
+
+/* BookS/4xx/8xx */
+#define INTERRUPT_MACHINE_CHECK   0x200
+
+/* BookS/8xx */
+#define INTERRUPT_SYSTEM_RESET0x100
+
+/* BookS */
+#define INTERRUPT_DATA_SEGMENT0x380
+#define INTERRUPT_INST_SEGMENT0x480
+#define INTERRUPT_TRACE   0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#ifdef CONFIG_PPC_BOOK3S
+#define INTERRUPT_DOORBELL0xa00
+#define INTERRUPT_PERFMON 0xf00
+#endif
+
+/* BookE/BookS/4xx/8xx */
+#define INTERRUPT_DATA_STORAGE0x300
+#define INTERRUPT_INST_STORAGE0x400
+#define INTERRUPT_ALIGNMENT   0x600
+#define INTERRUPT_PROGRAM 0x700
+#define INTERRUPT_SYSCALL 0xc00
+
+/* BookE/BookS/44x */
+#define INTERRUPT_FP_UNAVAIL  0x800
+
+/* BookE/BookS/44x/8xx */
+#define INTERRUPT_DECREMENTER 0x900
+
 static inline void nap_adjust_return(struct pt_regs *regs)
 {
 #ifdef CONFIG_PPC_970_NAP
@@ -70,7 +110,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
*regs, struct interrup
 * CT_WARN_ON comes here via program_check_exception,
 * so avoid recursion.
 */
-   if (TRAP(regs) != 0x700)
+   if (TRAP(regs) != INTERRUPT_PROGRAM)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
 #endif
@@ -175,7 +215,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
pt_regs *regs, struct inte
/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
/* Allow DEC and PMI to be traced when they are soft-NMI */
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -204,7 +245,8 @@ static inline void interrupt_nmi_exit_prepare(struct 
pt_regs *regs, struct inter
 */
 
 #ifdef CONFIG_PPC64
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON)
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index eddf362caedc..b55b4c23f3b6 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -728,7 +728,7 @@ void crash_fadump(struct pt_regs *regs, const char *str)
 * If we came in via system reset, wait a while for the secondary
 * CPUs to enter.
 */
-   if (TRAP(&(fdh->regs)) == 0x100) {
+   if (TRAP(&(fdh->regs)) == INTERRUPT_SYSTEM_R

Re: [PATCH v4] powerpc/traps: Enhance readability for trap types

2021-04-10 Thread Xiongwei Song 


> On Apr 10, 2021, at 8:35 AM, Nicholas Piggin  wrote:
> 
> Thanks for working on this, I think it's a nice cleanup and helps
> non-powerpc people understand the code a bit better.
> 

My pleasure.

> Excerpts from Xiongwei Song's message of April 10, 2021 12:28 am:
>> From: Xiongwei Song 
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the references of the trap hex values with these
>> macros.
>> 
>> Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> 
>> Reported-by: kernel test robot 
> 
> It now looks like lkp asked for this whole cleanup patch. I would
> put [kernel test robot ] in your v3->4 changelog
> item.
> 

Agree. I just forgot to delete this line in the patch.

>> Signed-off-by: Xiongwei Song 
>> ---
>> 
>> v3-v4:
>> Fix compile issue:
>> arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' 
>> undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
>> I didn't add "Reported-by: kernel test robot " here,
>> because it's improper for this patch.
> 
> [...]
> 
>> diff --git a/arch/powerpc/include/asm/traps.h 
>> b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index ..2e64e10afcef
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAPS_H
>> +#define _ASM_PPC_TRAPS_H
> 
> These could go in interrupt.h.
> 
>> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
>> +#define INTERRUPT_MACHINE_CHECK   0x000
>> +#define INTERRUPT_CRITICAL_INPUT  0x100
>> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
>> +#define INTERRUPT_PERFMON 0x260
>> +#define INTERRUPT_DOORBELL0x280
>> +#define INTERRUPT_DEBUG   0xd00
>> +#else
>> +#define INTERRUPT_SYSTEM_RESET0x100
>> +#define INTERRUPT_MACHINE_CHECK   0x200
> 
> [...]
> 
>> @@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
>>  trap = TRAP(regs);
>>  if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
>>  pr_cont("CFAR: "REG" ", regs->orig_gpr3);
>> -if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
>> +if (trap == INTERRUPT_MACHINE_CHECK ||
>> +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);
>>  else
> 
> This is now a change in behaviour because previously BOOKE/4xx tested
> 0x200, but now it tests 0.

Yes. Previously BOOKE/4xx tested 0x200, but checked this line history, please 
see
the commit below:

commit c54006491dde7d1b8050c5542716b751be92ed80
Author: Anton Blanchard 
Date:   Fri Nov 15 15:41:19 2013 +1100

powerpc: Print DAR and DSISR on machine check oopses

Machine check exceptions set DAR and DSISR, so print them in our
oops output.

Signed-off-by: Anton Blanchard 
Signed-off-by: Benjamin Herrenschmidt 

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 75c2d1009985..37c4103a8cff 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -864,7 +864,7 @@ void show_regs(struct pt_regs * regs)
trap = TRAP(regs);
if ((regs->trap != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
printk("CFAR: "REG"\n", regs->orig_gpr3);
-   if (trap == 0x300 || trap == 0x600)
+   if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
 #else

0x200 aims to test Machine check, but for 64e, the regs->trap should be 0x000 
here,
under the commit comments, I changed the code behavior. Sorry I didn’t add 
comments. 

> 
> That looks wrong for 4xx. 64e does put 0x000 there but I wonder if it 
> should use 0x200 instead.

Ok. Thanks for pointing this out, let me learn about 4xx. 

> Bit difficult to test this stuff, I do have
> some MCE injection patches for QEMU for 64s, might be able to look at
> porting them to 64e although I have no idea about booke machine checks.
> 

Yes, would appreciate your sharing.

> Anyway I don't think this patch should change generated code at all.
> Either change the code first with smaller patches, or make sure you
> keep the tests the same.

Agree.

Regards,
Xiongwei

> 
> Thanks,
> Nick

Re: [PATCH v3] powerpc/traps: Enhance readability for trap types

2021-04-10 Thread Xiongwei Song 


> On Apr 10, 2021, at 8:04 AM, Michael Ellerman  wrote:
> 
> Christophe Leroy  writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song 
>>> 
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h 
>>> b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>> 
>>>  struct interrupt_state {
>>>  #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
>>> *regs, struct interrup
>>>  * CT_WARN_ON comes here via program_check_exception,
>>>  * so avoid recursion.
>>>  */
>>> -   if (TRAP(regs) != 0x700)
>>> +   if (TRAP(regs) != INTERRUPT_PROGRAM)
>>> CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>> }
>>>  #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
>>> pt_regs *regs, struct inte
>>> /* Don't do any per-CPU operations until interrupt state is fixed */
>>>  #endif
>>> /* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +   TRAP(regs) != INTERRUPT_PERFMON) {
>> 
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.
> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.
> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>> /* kernel has accessed a bad area */
>>> 
>>> switch (TRAP(regs)) {
>>> -   case 0x300:
>>> -   case 0x380:
>>> -   case 0xe00:
>>> +   case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +   case INTERRUPT_DATA_SEGMENT:
>>> +   case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>> 
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Ok. 

Regards,
Xiongwei

> 
> cheers

Re: [PATCH v3] powerpc/traps: Enhance readability for trap types

2021-04-10 Thread Xiongwei Song 


> On Apr 10, 2021, at 12:14 AM, Christophe Leroy  
> wrote:
> 
> 
> 
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
>> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> v2-v3:
>> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
>> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
>> and Nicholas Piggin.
>> v1-v2:
>> Define more trap macros to replace more trap hexs in code, not just for
>> the __show_regs function. This is suggested by Christophe Leroy.
>> Signed-off-by: Xiongwei Song 
>> ---
>>  arch/powerpc/include/asm/interrupt.h  |  9 +---
>>  arch/powerpc/include/asm/ptrace.h |  3 ++-
>>  arch/powerpc/include/asm/traps.h  | 32 +++
>>  arch/powerpc/kernel/interrupt.c   |  3 ++-
>>  arch/powerpc/kernel/process.c |  5 -
>>  arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>>  arch/powerpc/mm/fault.c   | 21 +++---
>>  arch/powerpc/perf/core-book3s.c   |  5 +++--
>>  arch/powerpc/xmon/xmon.c  | 16 +++---
>>  9 files changed, 78 insertions(+), 21 deletions(-)
>>  create mode 100644 arch/powerpc/include/asm/traps.h
>> diff --git a/arch/powerpc/include/asm/interrupt.h 
>> b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>struct interrupt_state {
>>  #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
>> *regs, struct interrup
>>   * CT_WARN_ON comes here via program_check_exception,
>>   * so avoid recursion.
>>   */
>> -if (TRAP(regs) != 0x700)
>> +if (TRAP(regs) != INTERRUPT_PROGRAM)
>>  CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>  }
>>  #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
>> pt_regs *regs, struct inte
>>  /* Don't do any per-CPU operations until interrupt state is fixed */
>>  #endif
>>  /* Allow DEC and PMI to be traced when they are soft-NMI */
>> -if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +TRAP(regs) != INTERRUPT_PERFMON) {
> 
> I think too long names hinder readability, see later for suggestions.
> 
>>  state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>>  this_cpu_set_ftrace_enabled(0);
>>  }
>> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct 
>> pt_regs *regs, struct inter
>>  nmi_exit();
>>#ifdef CONFIG_PPC64
>> -if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
>> +if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +TRAP(regs) != INTERRUPT_PERFMON)
>>  this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>>#ifdef CONFIG_PPC_BOOK3S_64
>> diff --git a/arch/powerpc/include/asm/ptrace.h 
>> b/arch/powerpc/include/asm/ptrace.h
>> index f10498e1b3f6..7a17e0365d43 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -21,6 +21,7 @@
>>#include 
>>  #include 
>> +#include 
>>#ifndef __ASSEMBLY__
>>  struct pt_regs
>> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct 
>> pt_regs *regs)
>>static inline bool trap_is_syscall(struct pt_regs *regs)
>>  {
>> -return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
>> +return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>>  }
>>static inline bool trap_norestart(struct pt_regs *regs)
>> diff --git a/arch/powerpc/include/asm/traps.h 
>> b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index ..cb416a17097c
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAP

[PATCH v4] powerpc/traps: Enhance readability for trap types

2021-04-09 Thread Xiongwei Song 
From: Xiongwei Song 

Create a new header named traps.h, define macros to list ppc interrupt
types in traps.h, replace the references of the trap hex values with these
macros.

Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S and
arch/powerpc/include/asm/kvm_asm.h.

Reported-by: kernel test robot 
Signed-off-by: Xiongwei Song 
---

v3-v4:
Fix compile issue:
arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' 
undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
I didn't add "Reported-by: kernel test robot " here,
because it's improper for this patch.

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

---
 arch/powerpc/include/asm/interrupt.h  |  9 +---
 arch/powerpc/include/asm/ptrace.h |  3 ++-
 arch/powerpc/include/asm/traps.h  | 32 +++
 arch/powerpc/kernel/interrupt.c   |  3 ++-
 arch/powerpc/kernel/process.c |  5 -
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c   | 21 +++---
 arch/powerpc/perf/core-book3s.c   |  5 +++--
 arch/powerpc/xmon/xmon.c  | 16 +++---
 9 files changed, 78 insertions(+), 21 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h 
b/arch/powerpc/include/asm/interrupt.h
index 05e7fc4ffb50..4fd904fb5d59 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline void nap_adjust_return(struct pt_regs *regs)
 {
@@ -70,7 +71,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
*regs, struct interrup
 * CT_WARN_ON comes here via program_check_exception,
 * so avoid recursion.
 */
-   if (TRAP(regs) != 0x700)
+   if (TRAP(regs) != INTERRUPT_PROGRAM)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
 #endif
@@ -175,7 +176,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
pt_regs *regs, struct inte
/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
/* Allow DEC and PMI to be traced when they are soft-NMI */
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -204,7 +206,8 @@ static inline void interrupt_nmi_exit_prepare(struct 
pt_regs *regs, struct inter
 */
 
 #ifdef CONFIG_PPC64
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON)
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h 
b/arch/powerpc/include/asm/ptrace.h
index 95600f3a6523..07ff8629e776 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs 
*regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-   return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+   return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index ..2e64e10afcef
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
+#define INTERRUPT_MACHINE_CHECK   0x000
+#define INTERRUPT_CRITICAL_INPUT  0x100
+#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
+#define INTERRUPT_PERFMON 0x260
+#define INTERRUPT_DOORBELL0x280
+#define INTERRUPT_DEBUG   0xd00
+#else
+#define INTERRUPT_SYSTEM_RESET0x100
+#define INTERRUPT_MACHINE_CHECK   0x200
+#define INTERRUPT_DATA_SEGMENT0x380
+#define INTERRUPT_INST_SEGMENT0x480
+#define INTERRUPT_DOORBELL0xa00
+#define INTERRUPT_TRACE   0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_PERFMON 0xf00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#endif
+
+#define INTERRUP

[PATCH v3] powerpc/traps: Enhance readability for trap types

2021-04-08 Thread Xiongwei Song 
From: Xiongwei Song 

Create a new header named traps.h, define macros to list ppc interrupt
types in traps.h, replace the reference of the trap hex values with these
macros.

Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S and
arch/powerpc/include/asm/kvm_asm.h.

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/include/asm/interrupt.h  |  9 +---
 arch/powerpc/include/asm/ptrace.h |  3 ++-
 arch/powerpc/include/asm/traps.h  | 32 +++
 arch/powerpc/kernel/interrupt.c   |  3 ++-
 arch/powerpc/kernel/process.c |  5 -
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c   | 21 +++---
 arch/powerpc/perf/core-book3s.c   |  5 +++--
 arch/powerpc/xmon/xmon.c  | 16 +++---
 9 files changed, 78 insertions(+), 21 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h 
b/arch/powerpc/include/asm/interrupt.h
index 7c633896d758..5ce9898bc9a6 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
@@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
*regs, struct interrup
 * CT_WARN_ON comes here via program_check_exception,
 * so avoid recursion.
 */
-   if (TRAP(regs) != 0x700)
+   if (TRAP(regs) != INTERRUPT_PROGRAM)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
 #endif
@@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
pt_regs *regs, struct inte
/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
/* Allow DEC and PMI to be traced when they are soft-NMI */
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct 
pt_regs *regs, struct inter
nmi_exit();
 
 #ifdef CONFIG_PPC64
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+   if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+   TRAP(regs) != INTERRUPT_PERFMON)
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h 
b/arch/powerpc/include/asm/ptrace.h
index f10498e1b3f6..7a17e0365d43 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs 
*regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-   return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+   return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index ..cb416a17097c
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
+#define INTERRUPT_MACHINE_CHECK   0x000
+#define INTERRUPT_CRITICAL_INPUT  0x100
+#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
+#define INTERRUPT_PERFMON 0x260
+#define INTERRUPT_DOORBELL0x280
+#define INTERRUPT_DEBUG   0xd00
+#elif defined(CONFIG_PPC_BOOK3S)
+#define INTERRUPT_SYSTEM_RESET0x100
+#define INTERRUPT_MACHINE_CHECK   0x200
+#define INTERRUPT_DATA_SEGMENT0x380
+#define INTERRUPT_INST_SEGMENT0x480
+#define INTERRUPT_DOORBELL0xa00
+#define INTERRUPT_TRACE   0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_PERFMON 0xf00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#endif
+
+#define INTERRUPT_DATA_STORAGE0x300
+#define INTERRUPT_INST_STORAGE0x400
+#define INTERRUPT_ALIGNMENT   0x600
+#define INTERRUPT_PROGRAM 0x700
+#define INTERRUPT_FP_UNAVAIL  0x800
+#define INTERRUPT_DECREMENTER 0x900
+#define INTERRUPT_SYSCALL 0xc00
+
+#endif /

Re: [PATCH v2] powerpc/traps: Enhance readability for trap types

2021-04-05 Thread Xiongwei Song 


Regards,
Xiongwei




> On Apr 2, 2021, at 8:36 AM, Nicholas Piggin  wrote:
> 
> Excerpts from Segher Boessenkool's message of April 2, 2021 2:11 am:
>> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>>> Segher Boessenkool  于2021年4月1日周四 上午6:15写道：
>>> 
>>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>>>> So perhaps:
>>>>> 
>>>>>  EXC_SYSTEM_RESET
>>>>>  EXC_MACHINE_CHECK
>>>>>  EXC_DATA_STORAGE
>>>>>  EXC_DATA_SEGMENT
>>>>>  EXC_INST_STORAGE
>>>>>  EXC_INST_SEGMENT
>>>>>  EXC_EXTERNAL_INTERRUPT
>>>>>  EXC_ALIGNMENT
>>>>>  EXC_PROGRAM_CHECK
>>>>>  EXC_FP_UNAVAILABLE
>>>>>  EXC_DECREMENTER
>>>>>  EXC_HV_DECREMENTER
>>>>>  EXC_SYSTEM_CALL
>>>>>  EXC_HV_DATA_STORAGE
>>>>>  EXC_PERF_MONITOR
>>>> 
>>>> These are interrupt (vectors), not exceptions.  It doesn't matter all
>>>> that much, but confusing things more isn't useful either!  There can be
>>>> multiple exceptions that all can trigger the same interrupt.
>>>> 
>>>> When looking at the reference manual of e500 and e600 from NXP
>>> official, they call them as interrupts.While looking at the "The
>>> Programming Environments"
>>> that is also from NXP, they call them exceptions. Looks like there is
>>> no explicit distinction between interrupts and exceptions.
>> 
>> The architecture documents have always called it interrupts.  The PEM
>> says it calls them exceptions instead, but they are called interrupts in
>> the architecture (and the PEM says that, too).
>> 
>>> Here is the "The Programming Environments" link:
>>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
>> 
>> That document is 24 years old.  The architecture is still published,
>> new versions regularly.
>> 
>>> As far as I know, the values of interrupts or exceptions above are defined
>>> explicitly in reference manual or the programming environments.
>> 
>> They are defined in the architecture.
>> 
>>> Could
>>> you please provide more details about multiple exceptions with the same
>>> interrupts?
>> 
>> The simplest example is 700, program interrupt.  There are many causes
>> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
>> VX is actually divided into nine separate cases itself.  There also are
>> the various causes of privileged instruction type program interrupts,
>> and  the trap type program interrupt, but the FEX ones are most obvious
>> here.
> 
> Also:
> 
> * Some interrupts have no corresponding exception (system call and 
> system call vectored). This is not just semantics or a bug in the ISA
> because it is different from other synchronous interrupts: instructions 
> which cause exceptions (e.g., a page fault) do not complete before 
> taking the interrupt whereas sc does.
> 
> * It's quite usual for an exception to not cause an interrupt 
> immediately (MSR[EE]=0, HMEER) or never cause one and be cleared by 
> other means (msgclr, mtDEC, mtHMER, etc).
> 
> * It's possible for an exception to cause different interrupts!
> A decrementer exception usually causes a decrementer interrupt, but it
> can cause a system reset interrupt if the processor was in a power
> saving mode. A data storage exception can cause a DSI or HDSI interrupt
> depending on LPCR settings, and many other examples.
> 
> So I agree with Segher on this. We should use interrupt for interrupts, 
> reduce exception except where we really mean it, and move away from vec 
> and trap (I've got this wrong in the past too I admit). We don't have to 
> do it all immediately, but new code should go in this direction.

Appreciate these details about exceptions and interrupts. Looks like interrupt
is the correct term here. I’m glad to create the v3 patch with INTERRUPT_
prefix.  

Regards,
Xiongwei

> 
> Thanks,
> Nick

Re: [PATCH v2] powerpc/traps: Enhance readability for trap types

2021-04-05 Thread Xiongwei Song 


Regards,
Xiongwei




> On Apr 1, 2021, at 4:01 PM, Nicholas Piggin  wrote:
> 
> Excerpts from Michael Ellerman's message of April 1, 2021 12:39 pm:
>> Segher Boessenkool  writes:
>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
 So perhaps:
 
  EXC_SYSTEM_RESET
  EXC_MACHINE_CHECK
  EXC_DATA_STORAGE
  EXC_DATA_SEGMENT
  EXC_INST_STORAGE
  EXC_INST_SEGMENT
  EXC_EXTERNAL_INTERRUPT
  EXC_ALIGNMENT
  EXC_PROGRAM_CHECK
  EXC_FP_UNAVAILABLE
  EXC_DECREMENTER
  EXC_HV_DECREMENTER
  EXC_SYSTEM_CALL
  EXC_HV_DATA_STORAGE
  EXC_PERF_MONITOR
>>> 
>>> These are interrupt (vectors), not exceptions.  It doesn't matter all
>>> that much, but confusing things more isn't useful either!  There can be
>>> multiple exceptions that all can trigger the same interrupt.
>> 
>> Yeah I know, but I think that ship has already sailed as far as the
>> naming we have in the kernel.
> 
> It has, but there are also several other ships also sailing in different 
> directions. It could be worse though, at least they are not sideways in 
> the Suez.
> 
>> We have over 250 uses of "exc", and several files called "exception"
>> something.
>> 
>> Using "interrupt" can also be confusing because Linux uses that to mean
>> "external interrupt".
>> 
>> But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
> 
> We actually already have defines that follow Segher's suggestion, it's 
> just that they're hidden away in a KVM header.
> 
> #define BOOK3S_INTERRUPT_SYSTEM_RESET   0x100
> #define BOOK3S_INTERRUPT_MACHINE_CHECK  0x200
> #define BOOK3S_INTERRUPT_DATA_STORAGE   0x300
> #define BOOK3S_INTERRUPT_DATA_SEGMENT   0x380
> #define BOOK3S_INTERRUPT_INST_STORAGE   0x400
> #define BOOK3S_INTERRUPT_INST_SEGMENT   0x480
> #define BOOK3S_INTERRUPT_EXTERNAL   0x500
> #define BOOK3S_INTERRUPT_EXTERNAL_HV0x502
> #define BOOK3S_INTERRUPT_ALIGNMENT  0x600
> 
> It would take just a small amount of work to move these to general 
> powerpc header, add #ifdefs for Book E/S where the numbers differ,
> and remove the BOOK3S_ prefix.
> 
> I don't mind INTERRUPT_ but INT_ would be okay too. VEC_ actually
> doesn't match what Book E does (which is some weirdness to map some
> of them to match Book S but not all, arguably we should clean that
> up too and just use vector numbers consistently, but the INTERRUPT_
> prefix would still be valid if we did that).
> 
> BookE KVM entry will still continue to use a different convention
> there so I would leave all those KVM defines in place for now, we
> might do another pass on them later.

Thanks for the comments. 

> 
> Thanks,
> Nick

Re: [PATCH v2] powerpc/traps: Enhance readability for trap types

2021-04-05 Thread Xiongwei Song 


> On Apr 2, 2021, at 12:11 AM, Segher Boessenkool  
> wrote:
> 
> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>> Segher Boessenkool  于2021年4月1日周四 上午6:15写道：
>> 
>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>>> So perhaps:
>>>> 
>>>>  EXC_SYSTEM_RESET
>>>>  EXC_MACHINE_CHECK
>>>>  EXC_DATA_STORAGE
>>>>  EXC_DATA_SEGMENT
>>>>  EXC_INST_STORAGE
>>>>  EXC_INST_SEGMENT
>>>>  EXC_EXTERNAL_INTERRUPT
>>>>  EXC_ALIGNMENT
>>>>  EXC_PROGRAM_CHECK
>>>>  EXC_FP_UNAVAILABLE
>>>>  EXC_DECREMENTER
>>>>  EXC_HV_DECREMENTER
>>>>  EXC_SYSTEM_CALL
>>>>  EXC_HV_DATA_STORAGE
>>>>  EXC_PERF_MONITOR
>>> 
>>> These are interrupt (vectors), not exceptions.  It doesn't matter all
>>> that much, but confusing things more isn't useful either!  There can be
>>> multiple exceptions that all can trigger the same interrupt.
>>> 
>>> When looking at the reference manual of e500 and e600 from NXP
>> official, they call them as interrupts.While looking at the "The
>> Programming Environments"
>> that is also from NXP, they call them exceptions. Looks like there is
>> no explicit distinction between interrupts and exceptions.
> 
> The architecture documents have always called it interrupts.  The PEM
> says it calls them exceptions instead, but they are called interrupts in
> the architecture (and the PEM says that, too).
> 
>> Here is the "The Programming Environments" link:
>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
> 
> That document is 24 years old.  The architecture is still published,
> new versions regularly.
> 
>> As far as I know, the values of interrupts or exceptions above are defined
>> explicitly in reference manual or the programming environments.
> 
> They are defined in the architecture.
> 
>> Could
>> you please provide more details about multiple exceptions with the same
>> interrupts?
> 
> The simplest example is 700, program interrupt.  There are many causes
> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
> VX is actually divided into nine separate cases itself.  There also are
> the various causes of privileged instruction type program interrupts,
> and  the trap type program interrupt, but the FEX ones are most obvious
> here.

Thanks for the explanation.

Regards,
Xiongwei

> 
> 
> Segher

Re: [PATCH v2] powerpc/traps: Enhance readability for trap types

2021-03-31 Thread Xiongwei Song 
Segher Boessenkool  于2021年4月1日周四 上午6:15写道：

> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
> > So perhaps:
> >
> >   EXC_SYSTEM_RESET
> >   EXC_MACHINE_CHECK
> >   EXC_DATA_STORAGE
> >   EXC_DATA_SEGMENT
> >   EXC_INST_STORAGE
> >   EXC_INST_SEGMENT
> >   EXC_EXTERNAL_INTERRUPT
> >   EXC_ALIGNMENT
> >   EXC_PROGRAM_CHECK
> >   EXC_FP_UNAVAILABLE
> >   EXC_DECREMENTER
> >   EXC_HV_DECREMENTER
> >   EXC_SYSTEM_CALL
> >   EXC_HV_DATA_STORAGE
> >   EXC_PERF_MONITOR
>
> These are interrupt (vectors), not exceptions.  It doesn't matter all
> that much, but confusing things more isn't useful either!  There can be
> multiple exceptions that all can trigger the same interrupt.
>
>  When looking at the reference manual of e500 and e600 from NXP
 official, they call them as interrupts.While looking at the "The
Programming Environments"
 that is also from NXP, they call them exceptions. Looks like there is
 no explicit distinction between interrupts and exceptions.

Here are the links for the reference manual of e600 and e500:
https://www.nxp.com.cn/docs/en/reference-manual/E600CORERM.pdf
https://www.nxp.com/files-static/32bit/doc/ref_manual/EREF_RM.pdf

Here is the "The Programming Environments" link:
https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf

As far as I know, the values of interrupts or exceptions above are defined
explicitly in reference manual or the programming environments. Could
you please provide more details about multiple exceptions with the same
interrupts?

Xiongwei

Re: [PATCH v2] powerpc/traps: Enhance readability for trap types

2021-03-31 Thread Xiongwei Song 
Michael Ellerman  于2021年3月31日周三 下午5:58写道：

> Xiongwei Song  writes:
> > From: Xiongwei Song 
> >
> > Create a new header named traps.h, define macros to list ppc exception
> > types in traps.h, replace the reference of the real trap values with
> > these macros.
>
> Personally I find the hex values easier to recognise, but I realise
> that's probably not true of other people :)
>
> I'm one of the "other people".

> ...
> > diff --git a/arch/powerpc/include/asm/traps.h
> b/arch/powerpc/include/asm/traps.h
> > new file mode 100644
> > index ..a31b6122de23
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/traps.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_PPC_TRAPS_H
> > +#define _ASM_PPC_TRAPS_H
> > +
> > +#define TRAP_RESET   0x100 /* System reset */
> > +#define TRAP_MCE 0x200 /* Machine check */
> > +#define TRAP_DSI 0x300 /* Data storage */
> > +#define TRAP_DSEGI   0x380 /* Data segment */
> > +#define TRAP_ISI 0x400 /* Instruction storage */
> > +#define TRAP_ISEGI   0x480 /* Instruction segment */
> > +#define TRAP_ALIGN   0x600 /* Alignment */
> > +#define TRAP_PROG0x700 /* Program */
> > +#define TRAP_DEC 0x900 /* Decrementer */
> > +#define TRAP_SYSCALL 0xc00 /* System call */
> > +#define TRAP_TRACEI  0xd00 /* Trace */
> > +#define TRAP_FPA 0xe00 /* Floating-point Assist */
> > +#define TRAP_PMI 0xf00 /* Performance monitor */
>
> I know the macro is called TRAP and the field in pt_regs is called trap,
> but the terminology in the architecture is "exception", and we already
> have many uses of that. In particular we have a lot of uses of "exc" as
> an abbreviation for "exception". So I think I'd rather we use that than
> "TRAP".
>
Ok.

>
> I think we should probably use the names from the ISA, unless they are
> really over long.
>
> Which are:
>
>   0x100   System Reset
>   0x200   Machine Check
>   0x300   Data Storage
>   0x380   Data Segment
>   0x400   Instruction Storage
>   0x480   Instruction Segment
>   0x500   External
>   0x600   Alignment
>   0x700   Program
>   0x800   Floating-Point Unavailable
>   0x900   Decrementer
>   0x980   Hypervisor Decrementer
>   0xA00   Directed Privileged Doorbell
>   0xC00   System Call
>   0xD00   Trace
>   0xE00   Hypervisor Data Storage
>   0xE20   Hypervisor Instruction Storage
>   0xE40   Hypervisor Emulation Assistance
>   0xE60   Hypervisor Maintenance
>   0xE80   Directed Hypervisor Doorbell
>   0xEA0   Hypervisor Virtualization
>   0xF00   Performance Monitor
>   0xF20   Vector Unavailable
>   0xF40   VSX Unavailable
>   0xF60   Facility Unavailable
>   0xF80   Hypervisor Facility Unavailable
>   0xFA0   Directed Ultravisor Doorbell
>
>
> So perhaps:
>
>   EXC_SYSTEM_RESET
>   EXC_MACHINE_CHECK
>   EXC_DATA_STORAGE
>   EXC_DATA_SEGMENT
>   EXC_INST_STORAGE
>   EXC_INST_SEGMENT
>   EXC_EXTERNAL_INTERRUPT
>   EXC_ALIGNMENT
>   EXC_PROGRAM_CHECK
>   EXC_FP_UNAVAILABLE
>   EXC_DECREMENTER
>   EXC_HV_DECREMENTER
>   EXC_SYSTEM_CALL
>   EXC_HV_DATA_STORAGE
>   EXC_PERF_MONITOR
>
> Thanks for the suggestions. I'm ok with the prefix. Let me change.

[PATCH v2] powerpc/traps: Enhance readability for trap types

2021-03-30 Thread Xiongwei Song 
From: Xiongwei Song 

Create a new header named traps.h, define macros to list ppc exception
types in traps.h, replace the reference of the real trap values with
these macros.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/include/asm/interrupt.h  |  7 ---
 arch/powerpc/include/asm/ptrace.h |  3 ++-
 arch/powerpc/include/asm/traps.h  | 19 +++
 arch/powerpc/kernel/interrupt.c   |  2 +-
 arch/powerpc/kernel/process.c |  3 ++-
 arch/powerpc/kernel/traps.c   |  5 +++--
 arch/powerpc/kexec/crash.c|  3 ++-
 arch/powerpc/kvm/book3s_hv_builtin.c  |  5 +++--
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c   | 17 +
 arch/powerpc/perf/core-book3s.c   |  5 +++--
 arch/powerpc/xmon/xmon.c  | 21 +++--
 12 files changed, 62 insertions(+), 33 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h 
b/arch/powerpc/include/asm/interrupt.h
index 7c633896d758..d4c935ba8e16 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
@@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs 
*regs, struct interrup
 * CT_WARN_ON comes here via program_check_exception,
 * so avoid recursion.
 */
-   if (TRAP(regs) != 0x700)
+   if (TRAP(regs) != TRAP_PROG)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
 #endif
@@ -156,7 +157,7 @@ static inline void interrupt_nmi_enter_prepare(struct 
pt_regs *regs, struct inte
/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
/* Allow DEC and PMI to be traced when they are soft-NMI */
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+   if (TRAP(regs) != TRAP_DEC && TRAP(regs) != TRAP_PMI && TRAP(regs) != 
0x260) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -180,7 +181,7 @@ static inline void interrupt_nmi_exit_prepare(struct 
pt_regs *regs, struct inter
nmi_exit();
 
 #ifdef CONFIG_PPC64
-   if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+   if (TRAP(regs) != TRAP_DEC && TRAP(regs) != TRAP_PMI && TRAP(regs) != 
0x260)
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h 
b/arch/powerpc/include/asm/ptrace.h
index f10498e1b3f6..04726fb43a9d 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs 
*regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-   return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+   return (trap_is_scv(regs) || TRAP(regs) == TRAP_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index ..a31b6122de23
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#define TRAP_RESET   0x100 /* System reset */
+#define TRAP_MCE 0x200 /* Machine check */
+#define TRAP_DSI 0x300 /* Data storage */
+#define TRAP_DSEGI   0x380 /* Data segment */
+#define TRAP_ISI 0x400 /* Instruction storage */
+#define TRAP_ISEGI   0x480 /* Instruction segment */
+#define TRAP_ALIGN   0x600 /* Alignment */
+#define TRAP_PROG0x700 /* Program */
+#define TRAP_DEC 0x900 /* Decrementer */
+#define TRAP_SYSCALL 0xc00 /* System call */
+#define TRAP_TRACEI  0xd00 /* Trace */
+#define TRAP_FPA 0xe00 /* Floating-point Assist */
+#define TRAP_PMI 0xf00 /* Performance monitor */
+
+#endif /* _ASM_PPC_TRAPS_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..fc9a40cbbcae 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -456,7 +456,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct 
pt_regs *regs, unsign
 * CT_WARN_ON comes here via program_check_exception,
 * so avoid recursion.
 */
-   if (TRAP(regs) != 0x700)
+   if (TRAP(regs) != TRAP_PROG)
CT_WARN_ON(ct_state() == CONTEXT_USER);
 
kuap = kuap_get_and_assert_locked();
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b966c8e0cead..0d416744136

Re: [PATCH] powerpc/process: Enhance readability for trap types.

2021-03-28 Thread Xiongwei Song 


> On Mar 28, 2021, at 11:21 PM, Christophe Leroy  
> wrote:
> 
> 
> 
> Le 28/03/2021 à 16:35, Xiongwei Song a écrit :
>> From: Xiongwei Song 
>> Define macros to enhance the code readability on ppc trap types.
> 
> Good idea
> 
>> Signed-off-by: Xiongwei Song 
>> ---
>>  arch/powerpc/kernel/process.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 3231c2df9e26..3bbd3cf353a7 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1451,6 +1451,10 @@ static void print_msr_bits(unsigned long val)
>>  #define LAST_VOLATILE   12
>>  #endif
>>  +#define TRAP_MC  0x200 /* Machine Check */
> 
> I think usually we use MCE, so TRAP_MCE would be better

Ok.
> 
>> +#define TRAP_DSI 0x300 /* DSI exception */
>> +#define TRAP_AM  0x600 /* Alignment exception */
> 
> Don't know what AM means. TRAP_ALIGN would be more explicit.

No Problem.
> 
>> +
> 
> The defines should go in a header file, for instance asm/ptrace.h in order to 
> be re-used in other files.

Agree.
> 
> You should do more. You can find other places to improve with:
> 
> git grep "trap ==" arch/powerpc/
> git grep "TRAP(regs) ==" arch/powerpc/

Just ran “git grep”, looks like the work is much bigger than what I imagined.
> 
>>  static void __show_regs(struct pt_regs *regs)
>>  {
>>  int i, trap;
>> @@ -1465,7 +1469,7 @@ static void __show_regs(struct pt_regs *regs)
>>  trap = TRAP(regs);
>>  if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
>>  pr_cont("CFAR: "REG" ", regs->orig_gpr3);
>> -if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
>> +if (trap == TRAP_MC || trap == TRAP_DSI || trap == TRAP_AM) {
>>  if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
>>  pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, 
>> regs->dsisr);
>>  else

Thanks for the response. I will send v2.

 Regards,
Xiongwei

[PATCH] powerpc/process: Enhance readability for trap types.

2021-03-28 Thread Xiongwei Song 
From: Xiongwei Song 

Define macros to enhance the code readability on ppc trap types.

Signed-off-by: Xiongwei Song 
---
 arch/powerpc/kernel/process.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 3231c2df9e26..3bbd3cf353a7 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1451,6 +1451,10 @@ static void print_msr_bits(unsigned long val)
 #define LAST_VOLATILE  12
 #endif
 
+#define TRAP_MC  0x200 /* Machine Check */
+#define TRAP_DSI 0x300 /* DSI exception */
+#define TRAP_AM  0x600 /* Alignment exception */
+
 static void __show_regs(struct pt_regs *regs)
 {
int i, trap;
@@ -1465,7 +1469,7 @@ static void __show_regs(struct pt_regs *regs)
trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-   if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
+   if (trap == TRAP_MC || trap == TRAP_DSI || trap == TRAP_AM) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, 
regs->dsisr);
else
-- 
2.17.1