| Message ID | 1421342627-7214-1-git-send-email-ldufour@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Michael Ellerman |
| Headers | show |
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]; >
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
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 > >
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
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
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];
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(+)