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

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

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

base-commit: a87b93bdf800a4d7a42d95683624a4516e516b4f
-- 
2.24.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 ?

[...]

You don't need that #ifndef. Keeping it should be harmless.

Don't make it so complex, see below

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

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

On Fri, 2020-03-27 at 07:48 +0100, Christophe Leroy wrote:

Le 26/03/2020 à 07:15, Balamuruhan S a écrit :

quoted

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)

[...]

quoted

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.

quoted

  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

quoted

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

quoted

  		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.

quoted

  			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

quoted

+		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

Le 27/03/2020 à 10:03, Balamuruhan S a écrit :

On Fri, 2020-03-27 at 07:48 +0100, Christophe Leroy wrote:

quoted

Le 26/03/2020 à 07:15, Balamuruhan S a écrit :

quoted

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

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

Le 27/03/2020 à 19:19, Segher Boessenkool a écrit :

On Fri, Mar 27, 2020 at 04:12:13PM +0100, Christophe Leroy wrote:

quoted

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

On Fri, 2020-03-27 at 16:12 +0100, Christophe Leroy wrote:

Le 27/03/2020 à 10:03, Balamuruhan S a écrit :

quoted

On Fri, 2020-03-27 at 07:48 +0100, Christophe Leroy wrote:

quoted

Le 26/03/2020 à 07:15, Balamuruhan S a écrit :

quoted

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