Re: [PATCH] PowerPC: Clear MSR_VSX for getcontext

[Steven Munroe <munroesj at linux dot vnet dot ibm dot com>]

On Thu, 2013-10-17 at 17:38 +0200, Jakub Jelinek wrote:
> On Thu, Oct 17, 2013 at 08:56:21AM -0300, Adhemerval Zanella wrote:
> > This patch makes getcontext clear the MSR_VSX (indicating VSX usage)
> > from gregs[PT_MSR] so a following setcontext/swapcontext/makecontext
> > will not fail with invalid argument.
> > 
> > This patch fixes the testcases:
> > 
> > * stdlib/tst-setcontext,
> > * stdlib/tst-makecontext3, and
> > * tst-setcontext-fpscr
> > 
> > When building for PPC32 with --with-cpu=power7.
> 
> This is not sufficient, while what getcontext will return will be fine,
> what swapcontext returns (well, fills in *oucp) will often not.  So, if you
> say use some VSX instruction somewhere (thus modify VSX state), do
> getcontext (&uctx1); makecontext (&uctx1, fn, 0); swapcontext (&uctx2, &uctx1);
> this will all succeed, but uctx2 might have MSR_VSX bit set in it.  Thus
> if you say in fn call setcontext (&uctx2); or swapcontext (&uctx1, &uctx2);
> etc., those will fail.  And, for swapcontext there is no (easy) way to run
> code to actually clear the bit in the new context.
> 
> Which is why I'm afraid this is much more easily fixable in the kernel,

I agree. This was reported some time ago to kernel folks but seems to
have fallen into to bit bucket. I will try to raise the temperature on
the kernel side.

> say through something like:
> --- signal_32.c 2013-01-16 11:26:18.125461647 +0100
> +++ signal_32.c    2013-10-11 11:41:40.288026929 +0200
> @@ -449,7 +449,8 @@ static int save_user_regs(struct pt_regs
>                 if (copy_vsx_to_user(&frame->mc_vsregs, current))
>                         return 1;
>                 msr |= MSR_VSX;
> -       }
> +       } else if (!ctx_has_vsx_region)
> +               msr &= ~MSR_VSX;
>  #endif /* CONFIG_VSX */
>  #ifdef CONFIG_SPE
>         /* save spe registers */
> (complete untested) - the point, if somebody asks with getcontext
> or swapcontext for context being filled and it is small enough that
> the VSX state can't be stored into it, just clear the MSR_VSX bit
> so that you can actually successfully swapcontext to it.
> 
> Or the other option is to clear the MSR_VSX bit not in userland
> getcontext (where it is possible) and swapcontext (where it is not
> possible), but instead in setcontext and swapcontext in the structure
> with the new context.  Except that the argument points to const ucontext_t,
> and it might be undesirable or impossible to modify it, so it would need
> to check if the MSR_VSX bit is set, and if it is, copy to a temporary
> ucontext_t on the stack, clear the MSR_VSX bit there, and use address of the
> copy instead of the original as argument to the swapcontext syscall.
> This could be perhaps improved by also clearing the MSR_VSX bit in
> getcontext, so if you never use swapcontext function call, no copying would be
> ever needed.
> 
> 	Jakub
>