Messages in this thread |  | | From | Xiongwei Song <> | Date | Fri, 6 Aug 2021 21:22:52 +0800 | Subject | Re: [RFC PATCH 1/4] powerpc: Optimize register usage for esr register |
| |
On Fri, Aug 6, 2021 at 3:32 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 06/08/2021 à 05:16, Xiongwei Song a écrit : > > On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> > >> > >> Le 26/07/2021 à 16:30, sxwjean@me.com a écrit : > >>> From: Xiongwei Song <sxwjean@gmail.com> > >>> > >>> Create an anonymous union for dsisr and esr regsiters, we can reference > >>> esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y. > >>> Otherwise, reference dsisr. This makes code more clear. > >> > >> I'm not sure it is worth doing that. > > Why don't we use "esr" as reference manauls mentioned? > > > >> > >> What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly > >> the same: > >> > >> > - err = ___do_page_fault(regs, regs->dar, regs->dsisr); > >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE)) > >> > + err = ___do_page_fault(regs, regs->dar, regs->esr); > >> > + else > >> > + err = ___do_page_fault(regs, regs->dar, regs->dsisr); > >> > + > > Yes, we can drop this. But it's a bit vague. > > > >> Or even > >> > >> > - int is_write = page_fault_is_write(regs->dsisr); > >> > + unsigned long err_reg; > >> > + int is_write; > >> > + > >> > + if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE)) > >> > + err_reg = regs->esr; > >> > + else > >> > + err_reg = regs->dsisr; > >> > + > >> > + is_write = page_fault_is_write(err_reg); > >> > >> > >> Artificially growing the code for that makes no sense to me. > > > > We can drop this too. > >> > >> > >> To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so > >> that we know it represents the status register, which is DSISR or ESR depending on the platform. > > > > If so, this would make other people more confused. My consideration is > > to follow what the reference > > manuals represent. > > Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear
I still prefer my method.
> > That would be more explicit for everyone. > > The UAPI header however should remain as is because anonymous unions are not supported by old > compilers as mentioned by Michael.
Sure. Will update in v2.
> > But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have > in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1 > register into regs->dsisr.
Can I think my method has better expansibility here;-)? Let me finish esr and dear first. Thank you for the reminder.
Regards, Xiongwei > > Christophe
|  |