| Message ID | 2df879d982809c05b0dfade57942fe03dbe9e7de.1672767868.git.geoff@infradead.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5705c6d97efc4aa9478fe2887fd911f60ddf17e5 |
| Headers | show |
| Series | [v2,1/2] powerpc/ps3: Change updateboltedpp panic to info | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
Le 03/01/2023 à 18:51, Geoff Levand a écrit : > Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate > CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() > to be called. Change the panic statment in ps3_hpte_updateboltedpp() > to a pr_info statement so that bootup can continue. But if I understand correctly, it means that CONFIG_STRICT_KERNEL_RWX won't work then. So, shouldn't we keep the panic and forbid CONFIG_STRICT_KERNEL_RWX on PS3 ? > > Signed-off-by: Geoff Levand <geoff@infradead.org> > --- > arch/powerpc/platforms/ps3/htab.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/ps3/htab.c b/arch/powerpc/platforms/ps3/htab.c > index c27e6cf85272..9de62bd52650 100644 > --- a/arch/powerpc/platforms/ps3/htab.c > +++ b/arch/powerpc/platforms/ps3/htab.c > @@ -146,7 +146,7 @@ static long ps3_hpte_updatepp(unsigned long slot, unsigned long newpp, > static void ps3_hpte_updateboltedpp(unsigned long newpp, unsigned long ea, > int psize, int ssize) > { > - panic("ps3_hpte_updateboltedpp() not implemented"); > + pr_info("ps3_hpte_updateboltedpp() not implemented"); > } > > static void ps3_hpte_invalidate(unsigned long slot, unsigned long vpn,
Hi Christophe, On 1/9/23 09:41, Christophe Leroy wrote: > > > Le 03/01/2023 à 18:51, Geoff Levand a écrit : >> Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate >> CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() >> to be called. Change the panic statment in ps3_hpte_updateboltedpp() >> to a pr_info statement so that bootup can continue. > > But if I understand correctly, it means that CONFIG_STRICT_KERNEL_RWX > won't work then. > > So, shouldn't we keep the panic and forbid CONFIG_STRICT_KERNEL_RWX on PS3 ? mmu_hash_ops.updateboltedpp returns void, so I can't return an error code to indicate the feature is not supported. I could do something like this in arch/powerpc/Kconfig: - select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION + select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !PPC_PS3 && !HIBERNATION But then the ppc64_defconfig would be built without STRICT_KERNEL_RWX. I could do this in ps3_defconfig: +# CONFIG_STRICT_KERNEL_RWX is not set +# CONFIG_STRICT_MODULE_RWX is not set But I don't like that way because it seems too easy for users to not add those into a custom kernel config, and then they need to figure out what to do after their kernel panics on startup. What other 'clean' way is there? -Geoff
Geoff Levand <geoff@infradead.org> writes: > On 1/9/23 09:41, Christophe Leroy wrote: >> >> >> Le 03/01/2023 à 18:51, Geoff Levand a écrit : >>> Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate >>> CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() >>> to be called. Change the panic statment in ps3_hpte_updateboltedpp() >>> to a pr_info statement so that bootup can continue. >> >> But if I understand correctly, it means that CONFIG_STRICT_KERNEL_RWX >> won't work then. >> >> So, shouldn't we keep the panic and forbid CONFIG_STRICT_KERNEL_RWX on PS3 ? > > mmu_hash_ops.updateboltedpp returns void, so I can't return an error code to > indicate the feature is not supported. We could change that in the medium term. > I could do something like this in arch/powerpc/Kconfig: > > - select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION > + select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !PPC_PS3 && !HIBERNATION > > But then the ppc64_defconfig would be built without STRICT_KERNEL_RWX. Yeah that would be a pity. We could do the above and disable PS3 in ppc64_defconfig, allowing ppc64_defconfig to still have STRICT_KERNEL_RWX. I assume actual PS3 users would use a ps3_defconfig anyway right? Relatedly are there any actual PS3 users left? ;) > I could do this in ps3_defconfig: > > +# CONFIG_STRICT_KERNEL_RWX is not set > +# CONFIG_STRICT_MODULE_RWX is not set > > But I don't like that way because it seems too easy for users to not add those > into a custom kernel config, and then they need to figure out what to do after > their kernel panics on startup. Yep agreed. > What other 'clean' way is there? If we want to have a multi-platform kernel image that can boot on PS3 and other platforms, and have strict kernel RWX, then we need some runtime logic to deal with that. I'd rather not do that though, because it adds complexity to deal with a pretty obscure corner case, and I suspect no one really boots a ppc64_defconfig on actual PS3 hardware these days. So my preference is we disable PS3 in ppc64_defconfig, and make PS3 incompatible with STRICT_KERNEL_RWX. cheers
Hi, On 1/15/23 16:06, Michael Ellerman wrote: > Geoff Levand <geoff@infradead.org> writes: >> On 1/9/23 09:41, Christophe Leroy wrote: >>> >>> >>> Le 03/01/2023 à 18:51, Geoff Levand a écrit : >>>> Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate >>>> CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() >>>> to be called. Change the panic statment in ps3_hpte_updateboltedpp() >>>> to a pr_info statement so that bootup can continue. >>> >>> But if I understand correctly, it means that CONFIG_STRICT_KERNEL_RWX >>> won't work then. >>> >>> So, shouldn't we keep the panic and forbid CONFIG_STRICT_KERNEL_RWX on PS3 ? >> >> mmu_hash_ops.updateboltedpp returns void, so I can't return an error code to >> indicate the feature is not supported. > > We could change that in the medium term. > >> I could do something like this in arch/powerpc/Kconfig: >> >> - select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION >> + select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !PPC_PS3 && !HIBERNATION >> >> But then the ppc64_defconfig would be built without STRICT_KERNEL_RWX. > > Yeah that would be a pity. > > We could do the above and disable PS3 in ppc64_defconfig, allowing > ppc64_defconfig to still have STRICT_KERNEL_RWX. I really want to keep PS3 included in ppc64_defconfig. Not that I expect anyone to boot a ppc64_defconfig kernel on PS3, but that is one of the 'standard' configs that is built by some automated builders, and generally by anyone doing changes to the powerpc arch, and I want to keep getting those build tests for PS3. > I assume actual PS3 users would use a ps3_defconfig anyway right? Yeah, a derivative of it. They are most likely are using 'Jailbreak' firmware that allows them to run Linux in the gameos partition. > Relatedly are there any actual PS3 users left? ;) It seems there are more users now than a few years ago. I think they buy PS5s to play the latest games, and use their old console to mess around with Linux. I generally get a private inquiry every 3 or 4 weeks. Usually asking how to update their kernel, or how to install a modern distro. >> What other 'clean' way is there? > > If we want to have a multi-platform kernel image that can boot on PS3 > and other platforms, and have strict kernel RWX, then we need some > runtime logic to deal with that. > > I'd rather not do that though, because it adds complexity to deal with a > pretty obscure corner case, and I suspect no one really boots a > ppc64_defconfig on actual PS3 hardware these days. > > So my preference is we disable PS3 in ppc64_defconfig, and make PS3 > incompatible with STRICT_KERNEL_RWX. As mentioned, I'd really like to keep PS3 included in ppc64_defconfig. My original patch that basically just ignores the call to mmu_hash_ops.updateboltedpp allows that, and I haven't experienced any problems with it yet. My preference would be to keep PS3 in ppc64_defconfig, and either apply my original patch, or I keep that patch in my ps3-linux repo on kernel.org. Then, if we end up adding runtime support for RWX I then fixup PS3 to use that. -Geoff
Le 16/01/2023 à 21:08, Geoff Levand a écrit : > Hi, > > On 1/15/23 16:06, Michael Ellerman wrote: >> Geoff Levand <geoff@infradead.org> writes: >>> On 1/9/23 09:41, Christophe Leroy wrote: >>>> >>>> >>>> Le 03/01/2023 à 18:51, Geoff Levand a écrit : >>>>> Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate >>>>> CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() >>>>> to be called. Change the panic statment in ps3_hpte_updateboltedpp() >>>>> to a pr_info statement so that bootup can continue. >>>> >>>> But if I understand correctly, it means that CONFIG_STRICT_KERNEL_RWX >>>> won't work then. >>>> >>>> So, shouldn't we keep the panic and forbid CONFIG_STRICT_KERNEL_RWX on PS3 ? >>> >>> mmu_hash_ops.updateboltedpp returns void, so I can't return an error code to >>> indicate the feature is not supported. >> >> We could change that in the medium term. >> >>> I could do something like this in arch/powerpc/Kconfig: >>> >>> - select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION >>> + select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !PPC_PS3 && !HIBERNATION >>> >>> But then the ppc64_defconfig would be built without STRICT_KERNEL_RWX. >> >> Yeah that would be a pity. >> >> We could do the above and disable PS3 in ppc64_defconfig, allowing >> ppc64_defconfig to still have STRICT_KERNEL_RWX. > > I really want to keep PS3 included in ppc64_defconfig. Not that I expect > anyone to boot a ppc64_defconfig kernel on PS3, but that is one of the > 'standard' configs that is built by some automated builders, and generally by > anyone doing changes to the powerpc arch, and I want to keep getting those > build tests for PS3. > >> I assume actual PS3 users would use a ps3_defconfig anyway right? > > Yeah, a derivative of it. They are most likely are using 'Jailbreak' firmware > that allows them to run Linux in the gameos partition. > >> Relatedly are there any actual PS3 users left? ;) > > It seems there are more users now than a few years ago. I think they buy PS5s > to play the latest games, and use their old console to mess around with Linux. > I generally get a private inquiry every 3 or 4 weeks. Usually asking how to > update their kernel, or how to install a modern distro. > >>> What other 'clean' way is there? >> >> If we want to have a multi-platform kernel image that can boot on PS3 >> and other platforms, and have strict kernel RWX, then we need some >> runtime logic to deal with that. >> >> I'd rather not do that though, because it adds complexity to deal with a >> pretty obscure corner case, and I suspect no one really boots a >> ppc64_defconfig on actual PS3 hardware these days. >> >> So my preference is we disable PS3 in ppc64_defconfig, and make PS3 >> incompatible with STRICT_KERNEL_RWX. > > As mentioned, I'd really like to keep PS3 included in ppc64_defconfig. My > original patch that basically just ignores the call to > mmu_hash_ops.updateboltedpp allows that, and I haven't experienced any problems > with it yet. When you say you have not experienced any problems with it, do you mean that STRICT_RWX works for you ? When you select CONFIG_DEBUG_RODATA_TEST it tells you that it works ? Otherwise it looks incorrect to enable something that doesn't work. > > My preference would be to keep PS3 in ppc64_defconfig, and either apply my > original patch, or I keep that patch in my ps3-linux repo on kernel.org. Then, > if we end up adding runtime support for RWX I then fixup PS3 to use that. > In that case I see two solutions: 1/ Implement updateboltedpp for PS3. 2/ Implement arch_parse_debug_rodata() to always set rodata_enabled = false on PS3, and update free_initmem() to only call mark_initmem_nx() when strict_kernel_rwx_enabled() returns true. Christophe
On 1/16/23 23:26, Christophe Leroy wrote: > Le 16/01/2023 à 21:08, Geoff Levand a écrit : >> >> As mentioned, I'd really like to keep PS3 included in ppc64_defconfig. My >> original patch that basically just ignores the call to >> mmu_hash_ops.updateboltedpp allows that, and I haven't experienced any problems >> with it yet. > > When you say you have not experienced any problems with it, do you mean > that STRICT_RWX works for you ? When you select CONFIG_DEBUG_RODATA_TEST > it tells you that it works ? Otherwise it looks incorrect to enable > something that doesn't work. What I mean is that the system boots up, and works as expected. I have not tried with CONFIG_DEBUG_RODATA_TEST set. >> My preference would be to keep PS3 in ppc64_defconfig, and either apply my >> original patch, or I keep that patch in my ps3-linux repo on kernel.org. Then, >> if we end up adding runtime support for RWX I then fixup PS3 to use that. >> > > In that case I see two solutions: > 1/ Implement updateboltedpp for PS3. I'm now looking into if this is possible. > 2/ Implement arch_parse_debug_rodata() to always set rodata_enabled = > false on PS3, and update free_initmem() to only call mark_initmem_nx() > when strict_kernel_rwx_enabled() returns true. OK, I'll consider this if I cannot get updateboltedp working. -Geoff
Geoff Levand <geoff@infradead.org> writes: > On 1/16/23 23:26, Christophe Leroy wrote: >> Le 16/01/2023 à 21:08, Geoff Levand a écrit : >>> >>> As mentioned, I'd really like to keep PS3 included in ppc64_defconfig. My >>> original patch that basically just ignores the call to >>> mmu_hash_ops.updateboltedpp allows that, and I haven't experienced any problems >>> with it yet. >> >> When you say you have not experienced any problems with it, do you mean >> that STRICT_RWX works for you ? When you select CONFIG_DEBUG_RODATA_TEST >> it tells you that it works ? Otherwise it looks incorrect to enable >> something that doesn't work. > > What I mean is that the system boots up, and works as expected. > I have not tried with CONFIG_DEBUG_RODATA_TEST set. > >>> My preference would be to keep PS3 in ppc64_defconfig, and either apply my >>> original patch, or I keep that patch in my ps3-linux repo on kernel.org. Then, >>> if we end up adding runtime support for RWX I then fixup PS3 to use that. >>> >> >> In that case I see two solutions: >> 1/ Implement updateboltedpp for PS3. > > I'm now looking into if this is possible. > >> 2/ Implement arch_parse_debug_rodata() to always set rodata_enabled = >> false on PS3, and update free_initmem() to only call mark_initmem_nx() >> when strict_kernel_rwx_enabled() returns true. > > OK, I'll consider this if I cannot get updateboltedp working. I'll take this series as-is for 6.3, there's no point panicking. Hopefully one of the above options can be implemented in future. cheers
diff --git a/arch/powerpc/platforms/ps3/htab.c b/arch/powerpc/platforms/ps3/htab.c index c27e6cf85272..9de62bd52650 100644 --- a/arch/powerpc/platforms/ps3/htab.c +++ b/arch/powerpc/platforms/ps3/htab.c @@ -146,7 +146,7 @@ static long ps3_hpte_updatepp(unsigned long slot, unsigned long newpp, static void ps3_hpte_updateboltedpp(unsigned long newpp, unsigned long ea, int psize, int ssize) { - panic("ps3_hpte_updateboltedpp() not implemented"); + pr_info("ps3_hpte_updateboltedpp() not implemented"); } static void ps3_hpte_invalidate(unsigned long slot, unsigned long vpn,
Commit fdacae8a84024474afff234bdd1dbe19ad597a10 (powerpc: Activate CONFIG_STRICT_KERNEL_RWX by default) causes ps3_hpte_updateboltedpp() to be called. Change the panic statment in ps3_hpte_updateboltedpp() to a pr_info statement so that bootup can continue. Signed-off-by: Geoff Levand <geoff@infradead.org> --- arch/powerpc/platforms/ps3/htab.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)