Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
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
[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 [c3be74a0]
pc: c0102030: cacheflush+0x180/0x1a0
lr: c0101f3c: cacheflush+0x8c/0x1a0
sp: c3be7730
msr: 80081033
current = 0xc35e5c00
paca= 0xc191 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
[c3be7c50] c084abb0 __handle_sysrq+0xf0/0x2a0
[c3be7d00] c084b3c0 write_sysrq_trigger+0xb0/0xe0
[c3be7d30] c04d1edc proc_reg_write+0x8c/0x130
[c3be7d60] c040dc7c __vfs_write+0x3c/0x70
[c3be7d80] c0410e70 vfs_write+0xd0/0x210
[c3be7dd0] c041126c ksys_write+0xdc/0x130
[c3be7e20] c000b9d0 system_call+0x5c/0x68
--- Exception: c01 (System Call) at 7fffa345e420
SP (70b08ab0) is in userspace
Signed-off-by: Balamuruhan S
---
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
