LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Xiongwei Song <sxwjean@gmail.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Xiongwei Song <sxwjean@me.com>,
	PowerPC <linuxppc-dev@lists.ozlabs.org>,
	oleg@redhat.com, Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	ravi.bangoria@linux.ibm.com, npiggin@gmail.com,
	aneesh.kumar@linux.ibm.com, sandipan@linux.ibm.com,
	efremov@linux.com, peterx@redhat.com, akpm@linux-foundation.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register
Date: Fri, 6 Aug 2021 11:16:04 +0800	[thread overview]
Message-ID: <CAEVVKH-rht+xpwTUL=ny6qENe2Fb0n=3e7DEmc5qzpSq2_1gTA@mail.gmail.com> (raw)
In-Reply-To: <5e2336aa-90bf-b820-b5e4-e9a63088930f@csgroup.eu>

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");
> >

  reply	other threads:[~2021-08-06  3:16 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 ` [RFC PATCH 3/4] powerpc: Optimize register usage for dear register sxwjean
2021-08-05 10:09   ` Christophe Leroy
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-08-05 10:06 ` [RFC PATCH 1/4] powerpc: Optimize register usage for esr register Christophe Leroy
2021-08-06  3:16   ` Xiongwei Song [this message]
2021-08-06  7:32     ` Christophe Leroy
2021-08-06 13:22       ` Xiongwei Song
2021-08-06  6:53 ` Michael Ellerman
2021-08-06 13:14   ` Xiongwei Song
2021-08-06 14:26   ` Segher Boessenkool

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEVVKH-rht+xpwTUL=ny6qENe2Fb0n=3e7DEmc5qzpSq2_1gTA@mail.gmail.com' \
    --to=sxwjean@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=benh@kernel.crashing.org \
    --cc=christophe.leroy@csgroup.eu \
    --cc=efremov@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=npiggin@gmail.com \
    --cc=oleg@redhat.com \
    --cc=paulus@samba.org \
    --cc=peterx@redhat.com \
    --cc=ravi.bangoria@linux.ibm.com \
    --cc=sandipan@linux.ibm.com \
    --cc=sxwjean@me.com \
    --subject='Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).