| Message ID | 20200826013103.27651-2-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Bionic/Focal] powerpc/64s: Don't init FSCR_DSCR in __init_FSCR() | expand |
On 26.08.20 03:31, Thadeu Lima de Souza Cascardo wrote: > From: Michael Ellerman <mpe@ellerman.id.au> > > BugLink: https://bugs.launchpad.net/bugs/1888332 > > __init_FSCR() was added originally in commit 2468dcf641e4 ("powerpc: > Add support for context switching the TAR register") (Feb 2013), and > only set FSCR_TAR. > > At that point FSCR (Facility Status and Control Register) was not > context switched, so the setting was permanent after boot. > > Later we added initialisation of FSCR_DSCR to __init_FSCR(), in commit > 54c9b2253d34 ("powerpc: Set DSCR bit in FSCR setup") (Mar 2013), again > that was permanent after boot. > > Then commit 2517617e0de6 ("powerpc: Fix context switch DSCR on > POWER8") (Aug 2013) added a limited context switch of FSCR, just the > FSCR_DSCR bit was context switched based on thread.dscr_inherit. That > commit said "This clears the H/FSCR DSCR bit initially", but it > didn't, it left the initialisation of FSCR_DSCR in __init_FSCR(). > However the initial context switch from init_task to pid 1 would clear > FSCR_DSCR because thread.dscr_inherit was 0. > > That commit also introduced the requirement that FSCR_DSCR be clear > for user processes, so that we can take the facility unavailable > interrupt in order to manage dscr_inherit. > > Then in commit 152d523e6307 ("powerpc: Create context switch helpers > save_sprs() and restore_sprs()") (Dec 2015) FSCR was added to > thread_struct. However it still wasn't fully context switched, we just > took the existing value and set FSCR_DSCR if the new thread had > dscr_inherit set. FSCR was still initialised at boot to FSCR_DSCR | > FSCR_TAR, but that value was not propagated into the thread_struct, so > the initial context switch set FSCR_DSCR back to 0. > > Finally commit b57bd2de8c6c ("powerpc: Improve FSCR init and context > switching") (Jun 2016) added a full context switch of the FSCR, and > added an initialisation of init_task.thread.fscr to FSCR_TAR | > FSCR_EBB, but omitted FSCR_DSCR. > > The end result is that swapper runs with FSCR_DSCR set because of the > initialisation in __init_FSCR(), but no other processes do, they use > the value from init_task.thread.fscr. > > Having FSCR_DSCR set for swapper allows it to access SPR 3 from > userspace, but swapper never runs userspace, so it has no useful > effect. It's also confusing to have the value initialised in two > places to two different values. > > So remove FSCR_DSCR from __init_FSCR(), this at least gets us to the > point where there's a single value of FSCR, even if it's still set in > two places. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Tested-by: Alistair Popple <alistair@popple.id.au> > Link: https://lore.kernel.org/r/20200527145843.2761782-1-mpe@ellerman.id.au > (cherry picked from commit 0828137e8f16721842468e33df0460044a0c588b) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > arch/powerpc/kernel/cpu_setup_power.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S > index a460298c7ddb..f91ecb10d0ae 100644 > --- a/arch/powerpc/kernel/cpu_setup_power.S > +++ b/arch/powerpc/kernel/cpu_setup_power.S > @@ -184,7 +184,7 @@ __init_LPCR_ISA300: > > __init_FSCR: > mfspr r3,SPRN_FSCR > - ori r3,r3,FSCR_TAR|FSCR_DSCR|FSCR_EBB > + ori r3,r3,FSCR_TAR|FSCR_EBB > mtspr SPRN_FSCR,r3 > blr > >
On 26.08.20 03:31, Thadeu Lima de Souza Cascardo wrote: > From: Michael Ellerman <mpe@ellerman.id.au> > > BugLink: https://bugs.launchpad.net/bugs/1888332 > > __init_FSCR() was added originally in commit 2468dcf641e4 ("powerpc: > Add support for context switching the TAR register") (Feb 2013), and > only set FSCR_TAR. > > At that point FSCR (Facility Status and Control Register) was not > context switched, so the setting was permanent after boot. > > Later we added initialisation of FSCR_DSCR to __init_FSCR(), in commit > 54c9b2253d34 ("powerpc: Set DSCR bit in FSCR setup") (Mar 2013), again > that was permanent after boot. > > Then commit 2517617e0de6 ("powerpc: Fix context switch DSCR on > POWER8") (Aug 2013) added a limited context switch of FSCR, just the > FSCR_DSCR bit was context switched based on thread.dscr_inherit. That > commit said "This clears the H/FSCR DSCR bit initially", but it > didn't, it left the initialisation of FSCR_DSCR in __init_FSCR(). > However the initial context switch from init_task to pid 1 would clear > FSCR_DSCR because thread.dscr_inherit was 0. > > That commit also introduced the requirement that FSCR_DSCR be clear > for user processes, so that we can take the facility unavailable > interrupt in order to manage dscr_inherit. > > Then in commit 152d523e6307 ("powerpc: Create context switch helpers > save_sprs() and restore_sprs()") (Dec 2015) FSCR was added to > thread_struct. However it still wasn't fully context switched, we just > took the existing value and set FSCR_DSCR if the new thread had > dscr_inherit set. FSCR was still initialised at boot to FSCR_DSCR | > FSCR_TAR, but that value was not propagated into the thread_struct, so > the initial context switch set FSCR_DSCR back to 0. > > Finally commit b57bd2de8c6c ("powerpc: Improve FSCR init and context > switching") (Jun 2016) added a full context switch of the FSCR, and > added an initialisation of init_task.thread.fscr to FSCR_TAR | > FSCR_EBB, but omitted FSCR_DSCR. > > The end result is that swapper runs with FSCR_DSCR set because of the > initialisation in __init_FSCR(), but no other processes do, they use > the value from init_task.thread.fscr. > > Having FSCR_DSCR set for swapper allows it to access SPR 3 from > userspace, but swapper never runs userspace, so it has no useful > effect. It's also confusing to have the value initialised in two > places to two different values. > > So remove FSCR_DSCR from __init_FSCR(), this at least gets us to the > point where there's a single value of FSCR, even if it's still set in > two places. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Tested-by: Alistair Popple <alistair@popple.id.au> > Link: https://lore.kernel.org/r/20200527145843.2761782-1-mpe@ellerman.id.au > (cherry picked from commit 0828137e8f16721842468e33df0460044a0c588b) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > arch/powerpc/kernel/cpu_setup_power.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S > index a460298c7ddb..f91ecb10d0ae 100644 > --- a/arch/powerpc/kernel/cpu_setup_power.S > +++ b/arch/powerpc/kernel/cpu_setup_power.S > @@ -184,7 +184,7 @@ __init_LPCR_ISA300: > > __init_FSCR: > mfspr r3,SPRN_FSCR > - ori r3,r3,FSCR_TAR|FSCR_DSCR|FSCR_EBB > + ori r3,r3,FSCR_TAR|FSCR_EBB > mtspr SPRN_FSCR,r3 > blr > >
On 26.08.20 03:31, Thadeu Lima de Souza Cascardo wrote: > From: Michael Ellerman <mpe@ellerman.id.au> > > BugLink: https://bugs.launchpad.net/bugs/1888332 > > __init_FSCR() was added originally in commit 2468dcf641e4 ("powerpc: > Add support for context switching the TAR register") (Feb 2013), and > only set FSCR_TAR. > > At that point FSCR (Facility Status and Control Register) was not > context switched, so the setting was permanent after boot. > > Later we added initialisation of FSCR_DSCR to __init_FSCR(), in commit > 54c9b2253d34 ("powerpc: Set DSCR bit in FSCR setup") (Mar 2013), again > that was permanent after boot. > > Then commit 2517617e0de6 ("powerpc: Fix context switch DSCR on > POWER8") (Aug 2013) added a limited context switch of FSCR, just the > FSCR_DSCR bit was context switched based on thread.dscr_inherit. That > commit said "This clears the H/FSCR DSCR bit initially", but it > didn't, it left the initialisation of FSCR_DSCR in __init_FSCR(). > However the initial context switch from init_task to pid 1 would clear > FSCR_DSCR because thread.dscr_inherit was 0. > > That commit also introduced the requirement that FSCR_DSCR be clear > for user processes, so that we can take the facility unavailable > interrupt in order to manage dscr_inherit. > > Then in commit 152d523e6307 ("powerpc: Create context switch helpers > save_sprs() and restore_sprs()") (Dec 2015) FSCR was added to > thread_struct. However it still wasn't fully context switched, we just > took the existing value and set FSCR_DSCR if the new thread had > dscr_inherit set. FSCR was still initialised at boot to FSCR_DSCR | > FSCR_TAR, but that value was not propagated into the thread_struct, so > the initial context switch set FSCR_DSCR back to 0. > > Finally commit b57bd2de8c6c ("powerpc: Improve FSCR init and context > switching") (Jun 2016) added a full context switch of the FSCR, and > added an initialisation of init_task.thread.fscr to FSCR_TAR | > FSCR_EBB, but omitted FSCR_DSCR. > > The end result is that swapper runs with FSCR_DSCR set because of the > initialisation in __init_FSCR(), but no other processes do, they use > the value from init_task.thread.fscr. > > Having FSCR_DSCR set for swapper allows it to access SPR 3 from > userspace, but swapper never runs userspace, so it has no useful > effect. It's also confusing to have the value initialised in two > places to two different values. > > So remove FSCR_DSCR from __init_FSCR(), this at least gets us to the > point where there's a single value of FSCR, even if it's still set in > two places. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Tested-by: Alistair Popple <alistair@popple.id.au> > Link: https://lore.kernel.org/r/20200527145843.2761782-1-mpe@ellerman.id.au > (cherry picked from commit 0828137e8f16721842468e33df0460044a0c588b) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- Applied to focal/respin-6. And will get applied to bionic/respin-6 soon. Thanks. -Stefan > arch/powerpc/kernel/cpu_setup_power.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S > index a460298c7ddb..f91ecb10d0ae 100644 > --- a/arch/powerpc/kernel/cpu_setup_power.S > +++ b/arch/powerpc/kernel/cpu_setup_power.S > @@ -184,7 +184,7 @@ __init_LPCR_ISA300: > > __init_FSCR: > mfspr r3,SPRN_FSCR > - ori r3,r3,FSCR_TAR|FSCR_DSCR|FSCR_EBB > + ori r3,r3,FSCR_TAR|FSCR_EBB > mtspr SPRN_FSCR,r3 > blr > >
diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S index a460298c7ddb..f91ecb10d0ae 100644 --- a/arch/powerpc/kernel/cpu_setup_power.S +++ b/arch/powerpc/kernel/cpu_setup_power.S @@ -184,7 +184,7 @@ __init_LPCR_ISA300: __init_FSCR: mfspr r3,SPRN_FSCR - ori r3,r3,FSCR_TAR|FSCR_DSCR|FSCR_EBB + ori r3,r3,FSCR_TAR|FSCR_EBB mtspr SPRN_FSCR,r3 blr