diff mbox series

[v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S

Message ID 20200326061522.33123-1-bala24@linux.ibm.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (a87b93bdf800a4d7a42d95683624a4516e516b4f)
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/checkpatch warning total: 1 errors, 1 warnings, 0 checks, 47 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Balamuruhan S March 26, 2020, 6:15 a.m. UTC 
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.

0:mon> fi
cpu 0x0: Vector: 700 (Program Check) at [c000000003be74a0]
    pc: c000000000102030: cacheflush+0x180/0x1a0
    lr: c000000000101f3c: cacheflush+0x8c/0x1a0
    sp: c000000003be7730
   msr: 8000000000081033
  current = 0xc0000000035e5c00
  paca    = 0xc000000001910000   irqmask: 0x03   irq_happened: 0x01
    pid   = 1025, comm = bash
Linux version 5.6.0-rc5-g5aa19adac (root@ltc-wspoon6) (gcc version 7.4.0
(Ubuntu 7.4.0-1ubuntu1~18.04.1)) #1 SMP Tue Mar 10 04:38:41 CDT 2020
cpu 0x0: Exception 700 (Program Check) in xmon, returning to main loop
[c000000003be7c50] c00000000084abb0 __handle_sysrq+0xf0/0x2a0
[c000000003be7d00] c00000000084b3c0 write_sysrq_trigger+0xb0/0xe0
[c000000003be7d30] c0000000004d1edc proc_reg_write+0x8c/0x130
[c000000003be7d60] c00000000040dc7c __vfs_write+0x3c/0x70
[c000000003be7d80] c000000000410e70 vfs_write+0xd0/0x210
[c000000003be7dd0] c00000000041126c ksys_write+0xdc/0x130
[c000000003be7e20] c00000000000b9d0 system_call+0x5c/0x68
--- Exception: c01 (System Call) at 00007fffa345e420
SP (7ffff0b08ab0) is in userspace

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
---
changes in v2:
-------------
Fix review comments from Segher and Michael,
	* change incorrect architecture version 2.01 to 2.03 in commit
	  message.
	* ifdef it out the option `i` for PPC_BOOK3S_64 instead to drop it
	  and change the commit message accordingly.


base-commit: a87b93bdf800a4d7a42d95683624a4516e516b4f

Comments

Christophe Leroy March 27, 2020, 6:48 a.m. UTC | #1 
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 ?

[...]

> 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.

>   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)) {

>   			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.

> +		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
Balamuruhan S March 27, 2020, 9:03 a.m. UTC | #2 
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
Christophe Leroy March 27, 2020, 3:12 p.m. UTC | #3 
Le 27/03/2020 à 10:03, Balamuruhan S a écrit :
> 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)
> 

Then that's exactly what you have to say in the coming log.

Maybe you could also change invalidate_dcache_range():

	for (i = 0; i < size >> shift; i++, addr += bytes) {
		if (IS_ENABLED(CONFIG_PPC_BOOK3S_64))
			dcbf(addr);
		else
			dcbi(addr);
	}




Christophe
Segher Boessenkool March 27, 2020, 6:19 p.m. UTC | #4 
On Fri, Mar 27, 2020 at 04:12:13PM +0100, Christophe Leroy wrote:
> Maybe you could also change invalidate_dcache_range():
> 
> 	for (i = 0; i < size >> shift; i++, addr += bytes) {
> 		if (IS_ENABLED(CONFIG_PPC_BOOK3S_64))
> 			dcbf(addr);
> 		else
> 			dcbi(addr);
> 	}

But please note that flushing is pretty much the opposite from
invalidating (a flush (dcbf) makes sure that what is in the cache now
ends up in memory, while an invalidate (dcbi) makes sure it will *not*
end up in memory).  (Both will remove the addressed cache line from the
data caches).

So you cannot blindly replace them; in all cases you need to look and
see if it does what you need here.

(dcbi is much harder to use correctly -- it can race very easily -- so
in practice you will be fine most of the time; but be careful around
startup code and the like).


Segher
Christophe Leroy March 27, 2020, 7:40 p.m. UTC | #5 
Le 27/03/2020 à 19:19, Segher Boessenkool a écrit :
> On Fri, Mar 27, 2020 at 04:12:13PM +0100, Christophe Leroy wrote:
>> Maybe you could also change invalidate_dcache_range():
>>
>> 	for (i = 0; i < size >> shift; i++, addr += bytes) {
>> 		if (IS_ENABLED(CONFIG_PPC_BOOK3S_64))
>> 			dcbf(addr);
>> 		else
>> 			dcbi(addr);
>> 	}
> 
> But please note that flushing is pretty much the opposite from
> invalidating (a flush (dcbf) makes sure that what is in the cache now
> ends up in memory, while an invalidate (dcbi) makes sure it will *not*
> end up in memory).  (Both will remove the addressed cache line from the
> data caches).
> 
> So you cannot blindly replace them; in all cases you need to look and
> see if it does what you need here.
> 
> (dcbi is much harder to use correctly -- it can race very easily -- so
> in practice you will be fine most of the time; but be careful around
> startup code and the like).
> 

At the time being, invalidate_dcache_range() is used in only one place, 
and that's a place for PPC32 only. So I was just suggesting that just in 
case. Maybe there is no point in bothering with that at the time being.

Christophe
Balamuruhan S March 28, 2020, 8:03 a.m. UTC | #6 
On Fri, 2020-03-27 at 16:12 +0100, Christophe Leroy wrote:
> 
> Le 27/03/2020 à 10:03, Balamuruhan S a écrit :
> > 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)
> > 
> 
> Then that's exactly what you have to say in the coming log.

Sure, I will change the commit log in next version along with your suggested
way to ifdef.

> 
> Maybe you could also change invalidate_dcache_range():
> 
> 	for (i = 0; i < size >> shift; i++, addr += bytes) {
> 		if (IS_ENABLED(CONFIG_PPC_BOOK3S_64))
> 			dcbf(addr);
> 		else
> 			dcbi(addr);
> 	}

I will leave this as is based on the discussion.

Thank you Christophe and Segher.

-- Bala
> 
> 
> 
> 
> Christophe
diff mbox series

Patch

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
 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
+	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
 		if (cmd != 'i') {
 			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
+		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);