[PATCH] pseries/le: Fix endiannes issue in RTAS call from xmon

[PATCH] pseries/le: Fix endiannes issue in RTAS call from xmon

[Laurent Dufour]

On pseries system (LPAR) xmon failed to enter when running in LE mode, system
is hunging. Inititating xmon will lead to such an output on the console:

SysRq : Entering xmon
cpu 0x15: Vector: 0  at [c0000003f39ffb10]
    pc: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
    lr: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
    sp: c0000003f39ffc70
   msr: 8000000000009033
  current = 0xc0000003fafa7180
  paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
    pid   = 14617, comm = bash
Bad kernel stack pointer fafb4b0 at eca7cc4
cpu 0x15: Vector: 300 (Data Access) at [c000000007f07d40]
    pc: 000000000eca7cc4
    lr: 000000000eca7c44
    sp: fafb4b0
   msr: 8000000000001000
   dar: 10000000
 dsisr: 42000000
  current = 0xc0000003fafa7180
  paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
    pid   = 14617, comm = bash
cpu 0x15: Exception 300 (Data Access) in xmon, returning to main loop
xmon: WARNING: bad recursive fault on cpu 0x15

The root cause is that xmon is calling RTAS to turn off the surveillance
when entering xmon, and RTAS is requiring big endian parameters.

This patch is byte swapping the RTAS arguments when running in LE mode.

Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index b988b5addf86..c8efbb37d6e0 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -293,10 +293,10 @@ static inline void disable_surveillance(void)
 	args.token = rtas_token("set-indicator");
 	if (args.token == RTAS_UNKNOWN_SERVICE)
 		return;
-	args.nargs = 3;
-	args.nret = 1;
+	args.nargs = cpu_to_be32(3);
+	args.nret = cpu_to_be32(1);
 	args.rets = &args.args[3];
-	args.args[0] = SURVEILLANCE_TOKEN;
+	args.args[0] = cpu_to_be32(SURVEILLANCE_TOKEN);
 	args.args[1] = 0;
 	args.args[2] = 0;
 	enter_rtas(__pa(&args));
-- 
1.9.1

Re: pseries/le: Fix endiannes issue in RTAS call from xmon

[Michael Ellerman]

On Mon, 2014-24-11 at 14:07:53 UTC, Laurent Dufour wrote:
> On pseries system (LPAR) xmon failed to enter when running in LE mode, system
> is hunging. Inititating xmon will lead to such an output on the console:

OK. You say "LPAR", by which you mean "under phyp" I think. I haven't seen this
under KVM, and it looks like KVM doesn't implement "set-indicator" so that
would explain that.

I'll take this as a bug fix and CC it to stable.

cheers

Re: pseries/le: Fix endiannes issue in RTAS call from xmon

[Laurent Dufour]

On 26/11/2014 04:31, Michael Ellerman wrote:
> OK. You say "LPAR", by which you mean "under phyp" I think. I haven't seen this
> under KVM, and it looks like KVM doesn't implement "set-indicator" so that
> would explain that.

Yes LPAR implies phyp, and KVM don't implement "set-indicator" so this
doesn't happen in that case.

> I'll take this as a bug fix and CC it to stable.

That's a good point.

Thanks,
Laurent.

Re: [PATCH] pseries/le: Fix endiannes issue in RTAS call from xmon

[Michael Ellerman]

On Mon, 2014-11-24 at 15:07 +0100, Laurent Dufour wrote:
> On pseries system (LPAR) xmon failed to enter when running in LE mode, system
> is hunging. Inititating xmon will lead to such an output on the console:
> 
> SysRq : Entering xmon
> cpu 0x15: Vector: 0  at [c0000003f39ffb10]
>     pc: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
>     lr: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
>     sp: c0000003f39ffc70
>    msr: 8000000000009033
>   current = 0xc0000003fafa7180
>   paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
>     pid   = 14617, comm = bash
> Bad kernel stack pointer fafb4b0 at eca7cc4
> cpu 0x15: Vector: 300 (Data Access) at [c000000007f07d40]
>     pc: 000000000eca7cc4
>     lr: 000000000eca7c44
>     sp: fafb4b0
>    msr: 8000000000001000
>    dar: 10000000
>  dsisr: 42000000
>   current = 0xc0000003fafa7180
>   paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
>     pid   = 14617, comm = bash
> cpu 0x15: Exception 300 (Data Access) in xmon, returning to main loop
> xmon: WARNING: bad recursive fault on cpu 0x15
> 
> The root cause is that xmon is calling RTAS to turn off the surveillance
> when entering xmon, and RTAS is requiring big endian parameters.
> 
> This patch is byte swapping the RTAS arguments when running in LE mode.
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
>  arch/powerpc/xmon/xmon.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index b988b5addf86..c8efbb37d6e0 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -293,10 +293,10 @@ static inline void disable_surveillance(void)
>  	args.token = rtas_token("set-indicator");
>  	if (args.token == RTAS_UNKNOWN_SERVICE)
>  		return;

I just noticed we're not handling the token correctly here. It is be32 also.

cheers

Re: [PATCH] pseries/le: Fix endiannes issue in RTAS call from xmon

[Laurent Dufour]

On 15/01/2015 05:25, Michael Ellerman wrote:
> On Mon, 2014-11-24 at 15:07 +0100, Laurent Dufour wrote:
>> On pseries system (LPAR) xmon failed to enter when running in LE mode, system
>> is hunging. Inititating xmon will lead to such an output on the console:
>>
>> SysRq : Entering xmon
>> cpu 0x15: Vector: 0  at [c0000003f39ffb10]
>>     pc: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
>>     lr: c00000000007ed7c: sysrq_handle_xmon+0x5c/0x70
>>     sp: c0000003f39ffc70
>>    msr: 8000000000009033
>>   current = 0xc0000003fafa7180
>>   paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
>>     pid   = 14617, comm = bash
>> Bad kernel stack pointer fafb4b0 at eca7cc4
>> cpu 0x15: Vector: 300 (Data Access) at [c000000007f07d40]
>>     pc: 000000000eca7cc4
>>     lr: 000000000eca7c44
>>     sp: fafb4b0
>>    msr: 8000000000001000
>>    dar: 10000000
>>  dsisr: 42000000
>>   current = 0xc0000003fafa7180
>>   paca    = 0xc000000007d75e80	 softe: 0	 irq_happened: 0x01
>>     pid   = 14617, comm = bash
>> cpu 0x15: Exception 300 (Data Access) in xmon, returning to main loop
>> xmon: WARNING: bad recursive fault on cpu 0x15
>>
>> The root cause is that xmon is calling RTAS to turn off the surveillance
>> when entering xmon, and RTAS is requiring big endian parameters.
>>
>> This patch is byte swapping the RTAS arguments when running in LE mode.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/xmon/xmon.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index b988b5addf86..c8efbb37d6e0 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -293,10 +293,10 @@ static inline void disable_surveillance(void)
>>  	args.token = rtas_token("set-indicator");
>>  	if (args.token == RTAS_UNKNOWN_SERVICE)
>>  		return;
> 
> I just noticed we're not handling the token correctly here. It is be32 also.

Ouch, my mistake :(

I will drop a new patch to complete this one.

Cheers.

> cheers
> 
> 

[PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Laurent Dufour]

The commit 3b8a3c010969 ("powerpc/pseries: Fix endiannes issue in RTAS
call from xmon") was fixing an endianness issue in the call made from
xmon to RTAS.

However, as Michael Ellerman noticed, this fix was not complete, the
token value was not byte swapped. This lead to call an unexpected and
most of the time unexisting RTAS function, which is silently ignored
by RTAS.

This fix addresses this hole.

Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: stable@vger.kernel.org
Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 5b150f0c5df9..13c6e200b24e 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -337,6 +337,7 @@ static inline void disable_surveillance(void)
 	args.token = rtas_token("set-indicator");
 	if (args.token == RTAS_UNKNOWN_SERVICE)
 		return;
+	args.token = cpu_to_be32(args.token);
 	args.nargs = cpu_to_be32(3);
 	args.nret = cpu_to_be32(1);
 	args.rets = &args.args[3];
-- 
1.9.1

Re: [PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Tyrel Datwyler]

On 01/15/2015 09:23 AM, Laurent Dufour wrote:
> The commit 3b8a3c010969 ("powerpc/pseries: Fix endiannes issue in RTAS
> call from xmon") was fixing an endianness issue in the call made from
> xmon to RTAS.
> 
> However, as Michael Ellerman noticed, this fix was not complete, the
> token value was not byte swapped. This lead to call an unexpected and
> most of the time unexisting RTAS function, which is silently ignored
> by RTAS.

Nit. Not so much that is silently ignored by RTAS as much as
disable_surveillance silently doesn't check the return status of the
RTAS call. Maybe a check is warranted and reporting of non-success.

-Tyrel

> 
> This fix addresses this hole.
> 
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: stable@vger.kernel.org
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
>  arch/powerpc/xmon/xmon.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 5b150f0c5df9..13c6e200b24e 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -337,6 +337,7 @@ static inline void disable_surveillance(void)
>  	args.token = rtas_token("set-indicator");
>  	if (args.token == RTAS_UNKNOWN_SERVICE)
>  		return;
> +	args.token = cpu_to_be32(args.token);
>  	args.nargs = cpu_to_be32(3);
>  	args.nret = cpu_to_be32(1);
>  	args.rets = &args.args[3];
> 

Re: [PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Michael Ellerman]

On Thu, 2015-01-15 at 11:44 -0800, Tyrel Datwyler wrote:
> On 01/15/2015 09:23 AM, Laurent Dufour wrote:
> > The commit 3b8a3c010969 ("powerpc/pseries: Fix endiannes issue in RTAS
> > call from xmon") was fixing an endianness issue in the call made from
> > xmon to RTAS.
> > 
> > However, as Michael Ellerman noticed, this fix was not complete, the
> > token value was not byte swapped. This lead to call an unexpected and
> > most of the time unexisting RTAS function, which is silently ignored
> > by RTAS.
> 
> Nit. Not so much that is silently ignored by RTAS as much as
> disable_surveillance silently doesn't check the return status of the
> RTAS call. Maybe a check is warranted and reporting of non-success.

Yeah you're right, I added a printf of the result and got -3, which is also
wrong as far as I can tell, but I didn't have the energy to chase it any
further.

Because this is in xmon we want to be extra careful about what we do, but an
xmon_printf() should be safe. I'll do that as a cleanup after this.

cheers

Re: [PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Tyrel Datwyler]

On 01/15/2015 02:19 PM, Michael Ellerman wrote:
> On Thu, 2015-01-15 at 11:44 -0800, Tyrel Datwyler wrote:
>> On 01/15/2015 09:23 AM, Laurent Dufour wrote:
>>> The commit 3b8a3c010969 ("powerpc/pseries: Fix endiannes issue in RTAS
>>> call from xmon") was fixing an endianness issue in the call made from
>>> xmon to RTAS.
>>>
>>> However, as Michael Ellerman noticed, this fix was not complete, the
>>> token value was not byte swapped. This lead to call an unexpected and
>>> most of the time unexisting RTAS function, which is silently ignored
>>> by RTAS.
>>
>> Nit. Not so much that is silently ignored by RTAS as much as
>> disable_surveillance silently doesn't check the return status of the
>> RTAS call. Maybe a check is warranted and reporting of non-success.
> 
> Yeah you're right, I added a printf of the result and got -3, which is also
> wrong as far as I can tell, but I didn't have the energy to chase it any
> further.

If this was on a powerkvm guest set-indicator should be present for
hotplug (DLPAR) support. However, the surveillance indicator would not
be implemented. I know sometimes I forget if I'm on a powervm or
powerkvm guest. Just a thought.

-Tyrel

> 
> Because this is in xmon we want to be extra careful about what we do, but an
> xmon_printf() should be safe. I'll do that as a cleanup after this.
> 
> cheers
> 
> 

Re: [PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Michael Ellerman]

On Thu, 2015-01-15 at 15:41 -0800, Tyrel Datwyler wrote:
> On 01/15/2015 02:19 PM, Michael Ellerman wrote:
> > On Thu, 2015-01-15 at 11:44 -0800, Tyrel Datwyler wrote:
> >> On 01/15/2015 09:23 AM, Laurent Dufour wrote:
> >>> The commit 3b8a3c010969 ("powerpc/pseries: Fix endiannes issue in RTAS
> >>> call from xmon") was fixing an endianness issue in the call made from
> >>> xmon to RTAS.
> >>>
> >>> However, as Michael Ellerman noticed, this fix was not complete, the
> >>> token value was not byte swapped. This lead to call an unexpected and
> >>> most of the time unexisting RTAS function, which is silently ignored
> >>> by RTAS.
> >>
> >> Nit. Not so much that is silently ignored by RTAS as much as
> >> disable_surveillance silently doesn't check the return status of the
> >> RTAS call. Maybe a check is warranted and reporting of non-success.
> > 
> > Yeah you're right, I added a printf of the result and got -3, which is also
> > wrong as far as I can tell, but I didn't have the energy to chase it any
> > further.
> 
> If this was on a powerkvm guest set-indicator should be present for
> hotplug (DLPAR) support. However, the surveillance indicator would not
> be implemented. I know sometimes I forget if I'm on a powervm or
> powerkvm guest. Just a thought.

Right that does explain it. I went looking for the KVM kernel/qemu code that
implements set-indicator but couldn't find it. Presumably it's in some branch
other than the one I was looking at, or I was grepping for the wrong thing.

So I guess a printf there is probably not helpful, because it will fire always
on PowerKVM (at least at the moment).

cheers

Re: [PATCH] pseries/le: Fix another endiannes issue in RTAS call from xmon

[Laurent Dufour]

On 16/01/2015 04:02, Michael Ellerman wrote:
> On Thu, 2015-01-15 at 15:41 -0800, Tyrel Datwyler wrote:
>> On 01/15/2015 02:19 PM, Michael Ellerman wrote:
>> If this was on a powerkvm guest set-indicator should be present for
>> hotplug (DLPAR) support. However, the surveillance indicator would not
>> be implemented. I know sometimes I forget if I'm on a powervm or
>> powerkvm guest. Just a thought.
> 
> Right that does explain it. I went looking for the KVM kernel/qemu code that
> implements set-indicator but couldn't find it. Presumably it's in some branch
> other than the one I was looking at, or I was grepping for the wrong thing.
> 
> So I guess a printf there is probably not helpful, because it will fire always
> on PowerKVM (at least at the moment).

FWIW, the PAPR mentions that the surveillance indicator may not be
implemented. In that case -3 is returned.? I double checked that on my LPAR.

One option would be to warn only if the returned value is different from
0 and -3.

Cheers