Re: [PATCH v3 4/9] powerpc: Explicitly disable math features when copying thread

[Balbir Singh <bsingharora@gmail.com>]

On Wed, Jan 27, 2016 at 10:50 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
> On Mon, 25 Jan 2016 11:04:23 +1100
> Balbir Singh <bsingharora@gmail.com> wrote:
>
>> On Thu, 21 Jan 2016 11:55:44 +1100
>> Cyril Bur <cyrilbur@gmail.com> wrote:
>>
>> > Currently when threads get scheduled off they always giveup the FPU,
>> > Altivec (VMX) and Vector (VSX) units if they were using them. When they are
>> > scheduled back on a fault is then taken to enable each facility and load
>> > registers. As a result explicitly disabling FPU/VMX/VSX has not been
>> > necessary.
>> >
>> > Future changes and optimisations remove this mandatory giveup and fault
>> > which could cause calls such as clone() and fork() to copy threads and run
>> > them later with FPU/VMX/VSX enabled but no registers loaded.
>> >
>> > This patch starts the process of having MSR_{FP,VEC,VSX} mean that a
>> > threads registers are hot while not having MSR_{FP,VEC,VSX} means that the
>> > registers must be loaded. This allows for a smarter return to userspace.
>> >
>> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
>> > ---
>> >  arch/powerpc/kernel/process.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> > index dccc87e..e0c3d2d 100644
>> > --- a/arch/powerpc/kernel/process.c
>> > +++ b/arch/powerpc/kernel/process.c
>> > @@ -1307,6 +1307,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
>> >
>> >             f = ret_from_fork;
>> >     }
>> > +   childregs->msr &= ~(MSR_FP|MSR_VEC|MSR_VSX);
>>
>
> Hi Balbir,
>
> Perhaps I'm missing something, are you saying
>
>> Ideally you want to use __msr_check_and_clear()
>>
>
> instead of childregs->msr &= ~(MSR_FP|MSR_VEC|MSR_VSX); ? I don't see how that
> can work...
>
> __msr_check_and_clear() operates on the currently active MSR, that is, the msr
> for the current kernel context. childregs->msr is the value that will be used
> for that userspace context when the kernel returns. Here we must ensure that
> that children are created with the bit disabled.
>

Yes, my bad! I thought the routine took generic bits, hoping to reuse
the CONFIG_VSX bits. I don't think it helps much, what you have is
correct.

>> Basically we start with these bits off and then take an exception on use?
>>
>
> Currently yes, this is what I'm trying to change. This patch hasn't been
> necessary until now as any thread which saves its FPU/VMX/VSX data ALSO
> disables those bits in regs->msr and so theres no way a clone() or fork() can
> create a child with MSR_FP or MSR_VEC or MSR_VSX set. I add a meaning to
> 'having a regs->msr FP,VEC,VSX bit set' to mean that 'the regs are hot' in a
> subsequent patch which means this assumption no longer holds so now we must
> explicitly disable (so as to signal that the FPU/VMX/VSX regs are not hot) for
> children thread.
>
> Sounds like I still haven't got that commit message quite right yet.

I think the older series had more data to help understand the patch.
It would help to move some of them to the current series

Balbir