From: Timothy Pearson <tpearson@raptorengineering.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Jens Axboe <axboe@kernel.dk>,
regressions <regressions@lists.linux.dev>,
npiggin <npiggin@gmail.com>,
christophe leroy <christophe.leroy@csgroup.eu>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
Date: Thu, 23 Nov 2023 18:01:59 -0600 (CST) [thread overview]
Message-ID: <1340817182.49635143.1700784119445.JavaMail.zimbra@raptorengineeringinc.com> (raw)
In-Reply-To: <87leaqjs8x.fsf@mail.lhotse>
----- Original Message -----
> From: "Michael Ellerman" <mpe@ellerman.id.au>
> To: "Timothy Pearson" <tpearson@raptorengineering.com>
> Cc: "Jens Axboe" <axboe@kernel.dk>, "regressions" <regressions@lists.linux.dev>, "npiggin" <npiggin@gmail.com>,
> "christophe leroy" <christophe.leroy@csgroup.eu>, "linuxppc-dev" <linuxppc-dev@lists.ozlabs.org>
> Sent: Tuesday, November 21, 2023 11:01:50 PM
> Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
> Timothy Pearson <tpearson@raptorengineering.com> writes:
>>
> ...
>>
>> So a little more detail on this, just to put it to rest properly vs.
>> assuming hand analysis caught every possible pathway. :)
>>
>> The debugging that generates this stack trace also verifies the following in
>> __giveup_fpu():
>>
>> 1.) tsk->thread.fp_state.fpr doesn't contain the FPSCR contents prior to calling
>> save_fpu()
>> 2.) tsk->thread.fp_state.fpr contains the FPSCR contents directly after calling
>> save_fpu()
>> 3.) MSR_FP is set both in the task struct and in the live MSR.
>>
>> Only if all three conditions are met will it generate the trace. This
>> is a generalization of the hack I used to find the problem in the
>> first place.
>>
>> If the state will subsequently be reloaded from the thread struct,
>> that means we're reloading the registers from the thread struct that
>> we just verified was corrupted by the earlier save_fpu() call. There
>> are only two ways I can see for that to be true -- one is if the
>> registers were already clobbered when giveup_all() was entered, and
>> the other is if save_fpu() went ahead and clobbered them right here
>> inside giveup_all().
>>
>> To see which scenario we were dealing with, I added a bit more
>> instrumentation to dump the current register state if MSR_FP bit was
>> already set in registers (i.e. not dumping data from task struct, but
>> using the live FPU registers instead), and sure enough the registers
>> are corrupt on entry, so something else has already called save_fpu()
>> before we even hit giveup_all() in this call chain.
>
> Can you share the debug patch you're using?
>
> cheers
Sure, here you go. Note that with my FPU patch there is no WARN_ON hit, at least in my testing, so it isn't userspace purposefully loading the fr0/vs0 register with the FPSCR.
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 392404688cec..bde57dc3262a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -154,7 +154,49 @@ static void __giveup_fpu(struct task_struct *tsk)
{
unsigned long msr;
+ // DEBUGGING
+ uint64_t prev_fpr0 = *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0);
+ uint64_t prev_fpr1 = *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1);
+ struct thread_fp_state debug_fp_state;
+ unsigned long currentmsr = mfmsr();
+
+ if (currentmsr & MSR_FP) {
+ store_fp_state(&debug_fp_state);
+ load_fp_state(&debug_fp_state);
+ }
+
save_fpu(tsk);
+
+ // DEBUGGING
+ if (tsk->thread.regs->msr & MSR_FP) {
+ if (((*(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0) == 0x82004000) && (prev_fpr0 != 0x82004000))
+ || ((*(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1) == 0x82004000) && (prev_fpr1 != 0x82004000)))
+ {
+ WARN_ON(1);
+
+ printk("[TS %lld] In __giveup_fpu() for process [comm: '%s' pid %d tid %d], before save current "
+ "fp0: 0x%016llx/%016llx fp1: 0x%016llx/%016llx fp8: 0x%016llx/%016llx fp9: 0x%016llx/%016llx"
+ " msr: 0x%016lx (FP %d VSX %d EE %d) on core %d\n",
+ ktime_get_boottime_ns(), current->comm, current->pid, current->tgid,
+ *(((uint64_t*)(&debug_fp_state.fpr[0]))+0), *(((uint64_t*)(&debug_fp_state.fpr[0]))+1),
+ *(((uint64_t*)(&debug_fp_state.fpr[1]))+0), *(((uint64_t*)(&debug_fp_state.fpr[1]))+1),
+ *(((uint64_t*)(&debug_fp_state.fpr[8]))+0), *(((uint64_t*)(&debug_fp_state.fpr[8]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+1),
+ currentmsr, !!(currentmsr & MSR_FP), !!(currentmsr & MSR_VSX), !!(currentmsr & MSR_EE), raw_smp_processor_id());
+
+ printk("[TS %lld] In __giveup_fpu() for process [comm: '%s' pid %d tid %d], after save saved "
+ "fp0: 0x%016llx/%016llx fp1: 0x%016llx/%016llx fp8: 0x%016llx/%016llx fp9: 0x%016llx/%016llx"
+ " msr: 0x%016lx (FP %d VSX %d EE %d) on core %d\n",
+ ktime_get_boottime_ns(), current->comm, current->pid, current->tgid,
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[1]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[1]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[8]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[8]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+1),
+ tsk->thread.regs->msr, !!(tsk->thread.regs->msr & MSR_FP), !!(tsk->thread.regs->msr & MSR_VSX), !!(tsk->thread.regs->msr & MSR_EE), raw_smp_processor_id());
+ }
+ }
+
+
msr = tsk->thread.regs->msr;
msr &= ~(MSR_FP|MSR_FE0|MSR_FE1);
if (cpu_has_feature(CPU_FTR_VSX))
WARNING: multiple messages have this Message-ID (diff)
From: Timothy Pearson <tpearson@raptorengineering.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Jens Axboe <axboe@kernel.dk>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
regressions <regressions@lists.linux.dev>,
npiggin <npiggin@gmail.com>
Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
Date: Thu, 23 Nov 2023 18:01:59 -0600 (CST) [thread overview]
Message-ID: <1340817182.49635143.1700784119445.JavaMail.zimbra@raptorengineeringinc.com> (raw)
In-Reply-To: <87leaqjs8x.fsf@mail.lhotse>
----- Original Message -----
> From: "Michael Ellerman" <mpe@ellerman.id.au>
> To: "Timothy Pearson" <tpearson@raptorengineering.com>
> Cc: "Jens Axboe" <axboe@kernel.dk>, "regressions" <regressions@lists.linux.dev>, "npiggin" <npiggin@gmail.com>,
> "christophe leroy" <christophe.leroy@csgroup.eu>, "linuxppc-dev" <linuxppc-dev@lists.ozlabs.org>
> Sent: Tuesday, November 21, 2023 11:01:50 PM
> Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
> Timothy Pearson <tpearson@raptorengineering.com> writes:
>>
> ...
>>
>> So a little more detail on this, just to put it to rest properly vs.
>> assuming hand analysis caught every possible pathway. :)
>>
>> The debugging that generates this stack trace also verifies the following in
>> __giveup_fpu():
>>
>> 1.) tsk->thread.fp_state.fpr doesn't contain the FPSCR contents prior to calling
>> save_fpu()
>> 2.) tsk->thread.fp_state.fpr contains the FPSCR contents directly after calling
>> save_fpu()
>> 3.) MSR_FP is set both in the task struct and in the live MSR.
>>
>> Only if all three conditions are met will it generate the trace. This
>> is a generalization of the hack I used to find the problem in the
>> first place.
>>
>> If the state will subsequently be reloaded from the thread struct,
>> that means we're reloading the registers from the thread struct that
>> we just verified was corrupted by the earlier save_fpu() call. There
>> are only two ways I can see for that to be true -- one is if the
>> registers were already clobbered when giveup_all() was entered, and
>> the other is if save_fpu() went ahead and clobbered them right here
>> inside giveup_all().
>>
>> To see which scenario we were dealing with, I added a bit more
>> instrumentation to dump the current register state if MSR_FP bit was
>> already set in registers (i.e. not dumping data from task struct, but
>> using the live FPU registers instead), and sure enough the registers
>> are corrupt on entry, so something else has already called save_fpu()
>> before we even hit giveup_all() in this call chain.
>
> Can you share the debug patch you're using?
>
> cheers
Sure, here you go. Note that with my FPU patch there is no WARN_ON hit, at least in my testing, so it isn't userspace purposefully loading the fr0/vs0 register with the FPSCR.
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 392404688cec..bde57dc3262a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -154,7 +154,49 @@ static void __giveup_fpu(struct task_struct *tsk)
{
unsigned long msr;
+ // DEBUGGING
+ uint64_t prev_fpr0 = *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0);
+ uint64_t prev_fpr1 = *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1);
+ struct thread_fp_state debug_fp_state;
+ unsigned long currentmsr = mfmsr();
+
+ if (currentmsr & MSR_FP) {
+ store_fp_state(&debug_fp_state);
+ load_fp_state(&debug_fp_state);
+ }
+
save_fpu(tsk);
+
+ // DEBUGGING
+ if (tsk->thread.regs->msr & MSR_FP) {
+ if (((*(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0) == 0x82004000) && (prev_fpr0 != 0x82004000))
+ || ((*(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1) == 0x82004000) && (prev_fpr1 != 0x82004000)))
+ {
+ WARN_ON(1);
+
+ printk("[TS %lld] In __giveup_fpu() for process [comm: '%s' pid %d tid %d], before save current "
+ "fp0: 0x%016llx/%016llx fp1: 0x%016llx/%016llx fp8: 0x%016llx/%016llx fp9: 0x%016llx/%016llx"
+ " msr: 0x%016lx (FP %d VSX %d EE %d) on core %d\n",
+ ktime_get_boottime_ns(), current->comm, current->pid, current->tgid,
+ *(((uint64_t*)(&debug_fp_state.fpr[0]))+0), *(((uint64_t*)(&debug_fp_state.fpr[0]))+1),
+ *(((uint64_t*)(&debug_fp_state.fpr[1]))+0), *(((uint64_t*)(&debug_fp_state.fpr[1]))+1),
+ *(((uint64_t*)(&debug_fp_state.fpr[8]))+0), *(((uint64_t*)(&debug_fp_state.fpr[8]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+1),
+ currentmsr, !!(currentmsr & MSR_FP), !!(currentmsr & MSR_VSX), !!(currentmsr & MSR_EE), raw_smp_processor_id());
+
+ printk("[TS %lld] In __giveup_fpu() for process [comm: '%s' pid %d tid %d], after save saved "
+ "fp0: 0x%016llx/%016llx fp1: 0x%016llx/%016llx fp8: 0x%016llx/%016llx fp9: 0x%016llx/%016llx"
+ " msr: 0x%016lx (FP %d VSX %d EE %d) on core %d\n",
+ ktime_get_boottime_ns(), current->comm, current->pid, current->tgid,
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[0]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[1]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[1]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[8]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[8]))+1),
+ *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+0), *(((uint64_t*)(&tsk->thread.fp_state.fpr[9]))+1),
+ tsk->thread.regs->msr, !!(tsk->thread.regs->msr & MSR_FP), !!(tsk->thread.regs->msr & MSR_VSX), !!(tsk->thread.regs->msr & MSR_EE), raw_smp_processor_id());
+ }
+ }
+
+
msr = tsk->thread.regs->msr;
msr &= ~(MSR_FP|MSR_FE0|MSR_FE1);
if (cpu_has_feature(CPU_FTR_VSX))
next prev parent reply other threads:[~2023-11-24 0:02 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-19 15:18 [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save Timothy Pearson
2023-11-20 7:10 ` Michael Ellerman
2023-11-20 14:32 ` Nicholas Piggin
2023-11-20 16:45 ` Timothy Pearson
2023-11-20 16:45 ` Timothy Pearson
2023-11-20 23:39 ` Michael Ellerman
2023-11-20 23:39 ` Michael Ellerman
2023-11-21 0:27 ` Nicholas Piggin
2023-11-21 0:27 ` Nicholas Piggin
2023-11-21 1:23 ` Timothy Pearson
2023-11-21 1:23 ` Timothy Pearson
2023-11-21 7:56 ` Nicholas Piggin
2023-11-21 7:56 ` Nicholas Piggin
2023-11-21 4:10 ` Timothy Pearson
2023-11-21 4:10 ` Timothy Pearson
2023-11-21 4:26 ` Timothy Pearson
2023-11-21 4:26 ` Timothy Pearson
2023-11-21 7:54 ` Nicholas Piggin
2023-11-21 7:54 ` Nicholas Piggin
2023-11-22 5:01 ` Michael Ellerman
2023-11-22 5:01 ` Michael Ellerman
2023-11-24 0:01 ` Timothy Pearson [this message]
2023-11-24 0:01 ` Timothy Pearson
2023-11-27 18:39 ` Timothy Pearson
2023-11-27 18:39 ` Timothy Pearson
2023-11-27 19:58 ` Christophe Leroy
2023-11-27 19:58 ` Christophe Leroy
2023-11-28 0:59 ` Michael Ellerman
2023-11-28 0:59 ` Michael Ellerman
2023-11-28 1:40 ` Nicholas Piggin
2023-11-28 1:40 ` Nicholas Piggin
2023-11-27 22:53 ` Michael Ellerman
2023-11-27 22:53 ` Michael Ellerman
2023-11-28 12:57 ` Michael Ellerman
2023-11-28 12:57 ` Michael Ellerman
2023-11-30 16:29 ` Timothy Pearson
2023-11-30 16:29 ` Timothy Pearson
2023-11-21 0:18 ` Nicholas Piggin
2023-11-21 0:18 ` Nicholas Piggin
2023-12-02 23:00 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1340817182.49635143.1700784119445.JavaMail.zimbra@raptorengineeringinc.com \
--to=tpearson@raptorengineering.com \
--cc=axboe@kernel.dk \
--cc=christophe.leroy@csgroup.eu \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
--cc=regressions@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.