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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <26814448-c30a-1de1-bad4-79e2bffc3054@csgroup.eu>
Date:   Fri, 6 Aug 2021 09:32:43 +0200
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Xiongwei Song <sxwjean@...il.com>
Cc:     Xiongwei Song <sxwjean@...com>,
        PowerPC <linuxppc-dev@...ts.ozlabs.org>, oleg@...hat.com,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>, ravi.bangoria@...ux.ibm.com,
        npiggin@...il.com, aneesh.kumar@...ux.ibm.com,
        sandipan@...ux.ibm.com, efremov@...ux.com, peterx@...hat.com,
        akpm@...ux-foundation.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register



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