LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Xiongwei Song <sxwjean@gmail.com>
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 09:32:43 +0200	[thread overview]
Message-ID: <26814448-c30a-1de1-bad4-79e2bffc3054@csgroup.eu> (raw)
In-Reply-To: <CAEVVKH-rht+xpwTUL=ny6qENe2Fb0n=3e7DEmc5qzpSq2_1gTA@mail.gmail.com>



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

  reply	other threads:[~2021-08-06  7:32 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
2021-08-06  7:32     ` Christophe Leroy [this message]
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=26814448-c30a-1de1-bad4-79e2bffc3054@csgroup.eu \
    --to=christophe.leroy@csgroup.eu \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=benh@kernel.crashing.org \
    --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@gmail.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).