Re: [PATCH] PowerPC: Clear MSR_VSX for getcontext

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/17/2013 01:06 PM, Adhemerval Zanella wrote:
> On 17-10-2013 12:38, 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,
>> 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
>>
> Indeed my fix is incomplete and fixing kernel side seems an easier and robust way
> to do so. I'll talk with kernel guys, thanks.
> 

Jakub,

Thanks for the review!

Adhemerval,

I appreciate your help fixing this issue for POWER7, it's going to help
make a POWER7 runtime feature complete. There are quite a few applications
that rely on *context to work correctly and without this fix it doesn't.

Cheers,
Carlos.