On Fri, 2020-03-27 at 07:48 +0100, Christophe Leroy wrote: > > Le 26/03/2020 à 07:15, Balamuruhan S a écrit : > > Data Cache Block Invalidate (dcbi) instruction was implemented back in > > PowerPC > > architecture version 2.03. It is obsolete and attempt to use of this > > illegal > > instruction results in a hypervisor emulation assistance interrupt. So, > > ifdef > > it out the option `i` in xmon for 64bit Book3S. > > I don't understand. You say two contradictory things: > 1/ You say it _was_ added back. > 2/ You say it _is_ obsolete. > > How can it be obsolete if it was added back ?
I actually learnt it from P8 and P9 User Manual, The POWER8/POWER9 core does not provide support for the following optional or obsolete instructions (attempted use of these results in a hypervisor emulation assistance interrupt): • tlbia - TLB invalidate all • tlbiex - TLB invalidate entry by index (obsolete) • slbiex - SLB invalidate entry by index (obsolete) • dcba - Data cache block allocate (Book II; obsolete) • dcbi - Data cache block invalidate (obsolete) • rfi - Return from interrupt (32-bit; obsolete) > > [...] > > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > > index 0ec9640335bb..bfd5a97689cd 100644 > > --- a/arch/powerpc/xmon/xmon.c > > +++ b/arch/powerpc/xmon/xmon.c > > @@ -335,10 +335,12 @@ static inline void cflush(void *p) > > asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p)); > > } > > > > +#ifndef CONFIG_PPC_BOOK3S_64 > > You don't need that #ifndef. Keeping it should be harmless. okay. > > > static inline void cinval(void *p) > > { > > asm volatile ("dcbi 0,%0; icbi 0,%0" : : "r" (p)); > > } > > +#endif > > > > /** > > * write_ciabr() - write the CIABR SPR > > @@ -1791,8 +1793,9 @@ static void prregs(struct pt_regs *fp) > > > > static void cacheflush(void) > > { > > - int cmd; > > unsigned long nflush; > > +#ifndef CONFIG_PPC_BOOK3S_64 > > Don't make it so complex, see below > > > + int cmd; > > > > cmd = inchar(); > > if (cmd != 'i') > > @@ -1800,13 +1803,14 @@ static void cacheflush(void) > > scanhex((void *)&adrs); > > if (termch != '\n') > > termch = 0; > > +#endif > > nflush = 1; > > scanhex(&nflush); > > nflush = (nflush + L1_CACHE_BYTES - 1) / L1_CACHE_BYTES; > > if (setjmp(bus_error_jmp) == 0) { > > catch_memory_errors = 1; > > sync(); > > - > > +#ifndef CONFIG_PPC_BOOK3S_64 > > You don't need that ifndef, just ensure below that regardless of cmd, > book3s/64 calls cflush and not cinval. > > > if (cmd != 'i') { > > The only thing you have to do is to replace the above test by: > > if (cmd != 'i' || IS_ENABLED(CONFIG_PPC_BOOK3S_64)) { yes, this is the better way. > > > for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES) > > cflush((void *) adrs); > > @@ -1814,6 +1818,10 @@ static void cacheflush(void) > > for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES) > > cinval((void *) adrs); > > } > > +#else > > Don't need that at all, it's a duplication of the above. sure :+1: Thanks for reviewing. -- Bala > > > + for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES) > > + cflush((void *)adrs); > > +#endif > > sync(); > > /* wait a little while to see if we get a machine check */ > > __delay(200); > > > > base-commit: a87b93bdf800a4d7a42d95683624a4516e516b4f > > > > Christophe