* [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
@ 2020-03-26 6:15 Balamuruhan S
2020-03-27 6:48 ` Christophe Leroy
0 siblings, 1 reply; 7+ messages in thread
From: Balamuruhan S @ 2020-03-26 6:15 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, jniethe5, Balamuruhan S, paulus, sandipan,
naveen.n.rao, linuxppc-dev
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.
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-26 6:15 [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S Balamuruhan S
@ 2020-03-27 6:48 ` Christophe Leroy
2020-03-27 9:03 ` Balamuruhan S
0 siblings, 1 reply; 7+ messages in thread
From: Christophe Leroy @ 2020-03-27 6:48 UTC (permalink / raw)
To: Balamuruhan S, mpe
Cc: ravi.bangoria, jniethe5, paulus, sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-27 6:48 ` Christophe Leroy
@ 2020-03-27 9:03 ` Balamuruhan S
2020-03-27 15:12 ` Christophe Leroy
0 siblings, 1 reply; 7+ messages in thread
From: Balamuruhan S @ 2020-03-27 9:03 UTC (permalink / raw)
To: Christophe Leroy, mpe
Cc: ravi.bangoria, jniethe5, paulus, sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-27 9:03 ` Balamuruhan S
@ 2020-03-27 15:12 ` Christophe Leroy
2020-03-27 18:19 ` Segher Boessenkool
2020-03-28 8:03 ` Balamuruhan S
0 siblings, 2 replies; 7+ messages in thread
From: Christophe Leroy @ 2020-03-27 15:12 UTC (permalink / raw)
To: Balamuruhan S, mpe
Cc: ravi.bangoria, jniethe5, paulus, sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-27 15:12 ` Christophe Leroy
@ 2020-03-27 18:19 ` Segher Boessenkool
2020-03-27 19:40 ` Christophe Leroy
2020-03-28 8:03 ` Balamuruhan S
1 sibling, 1 reply; 7+ messages in thread
From: Segher Boessenkool @ 2020-03-27 18:19 UTC (permalink / raw)
To: Christophe Leroy
Cc: ravi.bangoria, Balamuruhan S, paulus, sandipan, jniethe5,
naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-27 18:19 ` Segher Boessenkool
@ 2020-03-27 19:40 ` Christophe Leroy
0 siblings, 0 replies; 7+ messages in thread
From: Christophe Leroy @ 2020-03-27 19:40 UTC (permalink / raw)
To: Segher Boessenkool
Cc: ravi.bangoria, Balamuruhan S, paulus, sandipan, jniethe5,
naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
2020-03-27 15:12 ` Christophe Leroy
2020-03-27 18:19 ` Segher Boessenkool
@ 2020-03-28 8:03 ` Balamuruhan S
1 sibling, 0 replies; 7+ messages in thread
From: Balamuruhan S @ 2020-03-28 8:03 UTC (permalink / raw)
To: Christophe Leroy, mpe
Cc: ravi.bangoria, jniethe5, paulus, sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-28 8:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 6:15 [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S Balamuruhan S
2020-03-27 6:48 ` Christophe Leroy
2020-03-27 9:03 ` Balamuruhan S
2020-03-27 15:12 ` Christophe Leroy
2020-03-27 18:19 ` Segher Boessenkool
2020-03-27 19:40 ` Christophe Leroy
2020-03-28 8:03 ` Balamuruhan S
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.