Message ID | 8900.1385072483@ale.ozlabs.ibm.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
On 11/21/2013 05:21 PM, Michael Neuling wrote: >>> What about the 64-bit code? I don't know the code but it appears at a glance to >>> have the same bug. >> >> It doesn't happen with 64-bit code because there the context contains >> a sigcontext which on ppc64 has vmx_reserve to store the entire VMX >> state. Therefore 64-bit ppc always has space to store the VMX registers >> in a userspace context switch. It is only the 32-bit ppc ABI that lacks >> the space. > > VMX? I don't understand this at all. We extended the ucontext to > handle the extra VSX state, so older code may still be using the small > ucontext and we already have a bunch of checks in the 64bit case for > this. > > I agree with Michael, we should add this to the 64 bit case. If we > can't put VSX state in, then clear MSR VSX. Sorry, typo, VSX not VMX. I had not gone through the historical implementation of the 64-bit code, I assumed it started with a sufficiently sized context structure, but on closer review it didn't. The addition of the *context functions in glibc for 64-bit power happened in 2003 by glibc commit 609b4783, with the mcontext_t being expanded to include I see that the 64-bit userspace context was extended in 2008 by your kernel commit ce48b210. Thus you're right the check is needed in the 64-bit case. However, at present the issue doesn't seem to trigger in the 64-bit userspace. Which is odd now that I review the code and see that the 64-bit userspace context is smaller than the kernel context (lacks the `+32' to the vmx_reserve space). It could just be that the compiler finds no chance to use VSX and therefore the existing test cases don't trigger the bug. I don't plan to investigate this further given that we're going to fix the 64-bit case also. > So how about we make it that simple and put it independent of the other > if statement? > > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > index 749778e..f4a7fd4 100644 > --- a/arch/powerpc/kernel/signal_32.c > +++ b/arch/powerpc/kernel/signal_32.c > @@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct mcon > return 1; > msr |= MSR_VSX; > } > + /* > + * With a small context structure we can't hold the VSX registers, > + * hence clear the MSR value to indicate the state was not saved. > + */ > + if (!ctx_has_vsx_region) > + msr &= ~MSR_VSX; > + > + > #endif /* CONFIG_VSX */ > #ifdef CONFIG_SPE > /* save spe registers */ > Looks good to me, along with a similar fix for signal_64.c. Cheers, Carlos.
On 11/21/2013 07:53 PM, Carlos O'Donell wrote: > The addition of the *context functions in glibc for 64-bit power > happened in 2003 by glibc commit 609b4783, with the mcontext_t > being expanded to include ... support for VMX state via `long vmx_reserve[NVRREG+NVRREG+1];'. Cheers, Carlos.
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c index 749778e..f4a7fd4 100644 --- a/arch/powerpc/kernel/signal_32.c +++ b/arch/powerpc/kernel/signal_32.c @@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct mcon return 1; msr |= MSR_VSX; } + /* + * With a small context structure we can't hold the VSX registers, + * hence clear the MSR value to indicate the state was not saved. + */ + if (!ctx_has_vsx_region) + msr &= ~MSR_VSX; + + #endif /* CONFIG_VSX */ #ifdef CONFIG_SPE /* save spe registers */