Re: [PATCH] powerpc/pseries: fix endian problems with LE migration

[Cyril Bur <cyrilbur@gmail.com>]

On Wed, 2015-01-21 at 14:33 +1100, Michael Ellerman wrote:
> On Wed, 2015-01-21 at 13:32 +1100, Cyril Bur wrote:
> > The need to handle ibm,suspend_me specially from within ppc_rtas has left an
> > endian bug exposed as rtas_ibm_suspend_me actually performs HCALLs and should
> > have its params in CPU endian.
> 
> That needs a much better explanation.
> 
Agreed

> Key points:
>  - ppc_rtas() is a syscall, which takes arguments in BE
>  - ibm,suspend-me is not a real RTAS call and is handled specially in there
>  - ibm,suspend-me is actually implemented by an hcall
>  - there is currently a bug on LE, because rtas_ibm_suspend_me() takes the
>    ppc_rtas() args and feeds them directly to the hcall
> 
I've tried to write that out neatly and orderly. Here's how that went:


RTAS events require arguments be passed in big endian while hypercalls
have their arguments passed in registers and the values should therefore
be in CPU endian.

The ibm,suspend_me 'RTAS' call makes a sequence of hypercalls to setup
one true RTAS call. This means that ibm,suspend_me is handled specially
in the ppc_rtas syscall.

The ppc_rtas syscall has its arguments in big endian and can therefore
pass these arguments directly to the rtas call. ibm,suspend_me is
handled specially from within ppc_rtas (by calling rtas_ibm_suspend_me)
which has left an endian bug on little endian systems due to the
requirement of hypercalls. The return value from rtas_ibm_suspend me
gets returned in cpu endian, and is left unconverted, also a bug on
little endian systems.

rtas_ibm_suspend_me does not actually make use of the rtas_args that it
is passed. This patch removes the convoluted use of the rtas_args struct
to pass params to rtas_ibm_suspend_me in favour of passing what it needs
as actual arguments. This patch also ensures the two callers of
rtas_ibm_suspend_me pass function parameters in cpu endian and in the
case of ppc_rtas, converts the return value.

migrate_store (the other caller of rtas_ibm_suspend_me) is from a sysfs
file which deals with everything in cpu endian so this function only
underwent cleanup.

> > Have ppc_rtas send the params correctly and also interpret the result
> > correctly.
> 
> That's a second bug which you should also mention above.
> 
> > Removed the convoluted use of the rtas_args struct to pass params to
> > rtas_ibm_suspend_me in favour of passing what it needs directly.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> > This patch has been tested with KVM both LE and BE and on PowerVM both LE and
> > BE. Under QEMU/KVM the migration happens without touching the these code
> > pathes.
> > For PowerVM there is no obvious regression on BE and the LE code path now
> > provides the correct parameters to the hypervisor
> 
> Fold that into the changelog, it's worth remembering.
> 
> cheers
> 
>