| Message ID | 20150120095735.A55671A5E86@localhost.localdomain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
From Christophe Leroy > Having a macro will help keep clear code. It might remove an #if but it doesn't really help. All it means is that anyone reading the code has to hunt for the definition before proceeding. Some comment about what (and why) the extra code is needed might help. ... > + > +#ifdef CONFIG_8xx_CPU15 > +#define DO_8xx_CPU15(tmp, addr) \ > + addi tmp, addr, PAGE_SIZE; \ > + tlbie tmp; \ > + addi tmp, addr, PAGE_SIZE; \ You've even transcribed this incorrectly. Clearly not tested :-) David > + tlbie tmp > +#else > +#define DO_8xx_CPU15(tmp, addr) > +#endif > + > InstructionTLBMiss: > #ifdef CONFIG_8xx_CPU6 > mtspr SPRN_DAR, r3 > @@ -304,12 +315,7 @@ InstructionTLBMiss: > EXCEPTION_PROLOG_0 > mtspr SPRN_SPRG_SCRATCH2, r10 > mfspr r10, SPRN_SRR0 /* Get effective address of fault */ > -#ifdef CONFIG_8xx_CPU15 > - addi r11, r10, PAGE_SIZE > - tlbie r11 > - addi r11, r10, -PAGE_SIZE > - tlbie r11 > -#endif > + DO_8xx_CPU15(r11, r10) > > /* If we are faulting a kernel address, we have to use the > * kernel page tables. > -- > 2.1.0 > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
Le 20/01/2015 12:09, David Laight a écrit : > From Christophe Leroy >> Having a macro will help keep clear code. > It might remove an #if but it doesn't really help. > All it means is that anyone reading the code has to hunt for > the definition before proceeding. > > Some comment about what (and why) the extra code is needed > might help. The main reason is because of patch 09/11 where we have to duplicate this code. I prefer to just duplicate one line rather than duplicate the whole code (especially because in v1 of the PATCHset, it was duplicated twice): - DO_8xx_CPU15(r11, r10) [...] #ifdef CONFIG_MODULES [...] + DO_8xx_CPU15(r10, r11) [...] +#else + mfspr r10, SPRN_SRR0 /* Get effective address of fault */ + DO_8xx_CPU15(r11, r10) Is this approach wrong ? > > ... >> + >> +#ifdef CONFIG_8xx_CPU15 >> +#define DO_8xx_CPU15(tmp, addr) \ >> + addi tmp, addr, PAGE_SIZE; \ >> + tlbie tmp; \ >> + addi tmp, addr, PAGE_SIZE; \ > You've even transcribed this incorrectly. Oops > > Clearly not tested :-) Indeed it's been tested, but tests can only show that the code is not worth than before. This code is there to fix a chip errata which (almost?) never happens. In my production version, I have not activated this errata, and he have never seen the problem on any of the more than 200 boards that have run for at least 4 years. Christophe > > David > >> + tlbie tmp >> +#else >> +#define DO_8xx_CPU15(tmp, addr) >> +#endif >> + >> InstructionTLBMiss: >> #ifdef CONFIG_8xx_CPU6 >> mtspr SPRN_DAR, r3 >> @@ -304,12 +315,7 @@ InstructionTLBMiss: >> EXCEPTION_PROLOG_0 >> mtspr SPRN_SPRG_SCRATCH2, r10 >> mfspr r10, SPRN_SRR0 /* Get effective address of fault */ >> -#ifdef CONFIG_8xx_CPU15 >> - addi r11, r10, PAGE_SIZE >> - tlbie r11 >> - addi r11, r10, -PAGE_SIZE >> - tlbie r11 >> -#endif >> + DO_8xx_CPU15(r11, r10) >> >> /* If we are faulting a kernel address, we have to use the >> * kernel page tables. >> -- >> 2.1.0 >> >> _______________________________________________ >> Linuxppc-dev mailing list >> Linuxppc-dev@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/linuxppc-dev
From: leroy > Le 20/01/2015 12:09, David Laight a écrit : > > From Christophe Leroy > >> Having a macro will help keep clear code. > > It might remove an #if but it doesn't really help. > > All it means is that anyone reading the code has to hunt for > > the definition before proceeding. > > > > Some comment about what (and why) the extra code is needed > > might help. > The main reason is because of patch 09/11 where we have to duplicate > this code. I prefer to just duplicate one line rather than duplicate the > whole code (especially because in v1 of the PATCHset, it was duplicated > twice): > > - DO_8xx_CPU15(r11, r10) > [...] > #ifdef CONFIG_MODULES > [...] > + DO_8xx_CPU15(r10, r11) > [...] > +#else > + mfspr r10, SPRN_SRR0 /* Get effective address of fault */ > + DO_8xx_CPU15(r11, r10) > > Is this approach wrong ? I'd call it something that infers 'invalidate adjacent pages' and then mention that this is needed due to a cpu errata. David
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index a1571b3..065896f 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -297,6 +297,17 @@ SystemCall: * We have to use the MD_xxx registers for the tablewalk because the * equivalent MI_xxx registers only perform the attribute functions. */ + +#ifdef CONFIG_8xx_CPU15 +#define DO_8xx_CPU15(tmp, addr) \ + addi tmp, addr, PAGE_SIZE; \ + tlbie tmp; \ + addi tmp, addr, PAGE_SIZE; \ + tlbie tmp +#else +#define DO_8xx_CPU15(tmp, addr) +#endif + InstructionTLBMiss: #ifdef CONFIG_8xx_CPU6 mtspr SPRN_DAR, r3 @@ -304,12 +315,7 @@ InstructionTLBMiss: EXCEPTION_PROLOG_0 mtspr SPRN_SPRG_SCRATCH2, r10 mfspr r10, SPRN_SRR0 /* Get effective address of fault */ -#ifdef CONFIG_8xx_CPU15 - addi r11, r10, PAGE_SIZE - tlbie r11 - addi r11, r10, -PAGE_SIZE - tlbie r11 -#endif + DO_8xx_CPU15(r11, r10) /* If we are faulting a kernel address, we have to use the * kernel page tables.
Having a macro will help keep clear code. Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- v2: no change arch/powerpc/kernel/head_8xx.S | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)