| Message ID | a40565807874c9ca82d60c118225ee65fe668fcd.1697614386.git.andrea.porta@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Make Aarch32 compatibility enablement optional at boot | expand |
On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote: > Major aspect of Aarch32 emulation is the ability to load 32bit > processes. > That's currently decided (among others) by compat_elf_check_arch(). > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is > enabled before loading a 32bit process. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> Why can't you make system_supports_32bit_el0() take the option into account instead? Mark. > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 657ea273c0f9..96832f1ec3ee 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -601,7 +601,7 @@ unsigned long arch_align_stack(unsigned long sp) > #ifdef CONFIG_COMPAT > int compat_elf_check_arch(const struct elf32_hdr *hdr) > { > - if (!system_supports_32bit_el0()) > + if (!system_supports_32bit_el0() || !aarch32_enabled()) > return false; > > if ((hdr)->e_machine != EM_ARM) > -- > 2.35.3 >
On 13:52 Wed 18 Oct , Mark Rutland wrote: > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote: > > Major aspect of Aarch32 emulation is the ability to load 32bit > > processes. > > That's currently decided (among others) by compat_elf_check_arch(). > > > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is > > enabled before loading a 32bit process. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > Why can't you make system_supports_32bit_el0() take the option into account > instead? > I may be wrong here, but it seems to me that system_supports_32bit_el0() answers teh question "can this system supports compat execution?" rather than "do I want this system to run any compat execution?". That's the point of aarch32_enabled(), to state whether we want teh system to run A32 code or not, regardless of the system supporting it (of course, if the system does not support A32 in EL0, this is a no-no, but that's another story). Andrea
On Thu, Oct 19, 2023 at 02:38:32PM +0200, Andrea della Porta wrote: > On 13:52 Wed 18 Oct , Mark Rutland wrote: > > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote: > > > Major aspect of Aarch32 emulation is the ability to load 32bit > > > processes. > > > That's currently decided (among others) by compat_elf_check_arch(). > > > > > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is > > > enabled before loading a 32bit process. > > > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > Why can't you make system_supports_32bit_el0() take the option into account > > instead? > > > > I may be wrong here, but it seems to me that system_supports_32bit_el0() > answers teh question "can this system supports compat execution?" rather than > "do I want this system to run any compat execution?". That's the point of > aarch32_enabled(), to state whether we want teh system to run A32 code or not, > regardless of the system supporting it (of course, if the system does not > support A32 in EL0, this is a no-no, but that's another story). That's what the implementation does today, but we're really using it as a "do we intend for 32-bit EL0 to work?" predicate, and generally the system_supports_${FEATURE}() helpers are affected by the combination of actual HW support, kernel config options, *and* kernel command line options. For example, system_supports_sve() is affected by both CONFIG_ARM64_SVE and the "arm64.nosve" command line option. Thanks, Mark.
On 15:27 Thu 19 Oct , Mark Rutland wrote: > On Thu, Oct 19, 2023 at 02:38:32PM +0200, Andrea della Porta wrote: > > On 13:52 Wed 18 Oct , Mark Rutland wrote: > > > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote: > > > > Major aspect of Aarch32 emulation is the ability to load 32bit > > > > processes. > > > > That's currently decided (among others) by compat_elf_check_arch(). > > > > > > > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is > > > > enabled before loading a 32bit process. > > > > > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > > > Why can't you make system_supports_32bit_el0() take the option into account > > > instead? > > > > > > > I may be wrong here, but it seems to me that system_supports_32bit_el0() > > answers teh question "can this system supports compat execution?" rather than > > "do I want this system to run any compat execution?". That's the point of > > aarch32_enabled(), to state whether we want teh system to run A32 code or not, > > regardless of the system supporting it (of course, if the system does not > > support A32 in EL0, this is a no-no, but that's another story). > > That's what the implementation does today, but we're really using it as a "do > we intend for 32-bit EL0 to work?" predicate, and generally the > system_supports_${FEATURE}() helpers are affected by the combination of actual > HW support, kernel config options, *and* kernel command line options. For > example, system_supports_sve() is affected by both CONFIG_ARM64_SVE and the > "arm64.nosve" command line option. > > Thanks, > Mark. Many thanks for the explanation, then inserting aach32_enabled() in system_supports_32bit_el0() is the way to go. Andrea
On Thu, Oct 19, 2023 at 04:32:27PM +0200, Andrea della Porta wrote: > On 15:27 Thu 19 Oct , Mark Rutland wrote: > > On Thu, Oct 19, 2023 at 02:38:32PM +0200, Andrea della Porta wrote: > > > On 13:52 Wed 18 Oct , Mark Rutland wrote: > > > > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote: > > > > > Major aspect of Aarch32 emulation is the ability to load 32bit > > > > > processes. > > > > > That's currently decided (among others) by compat_elf_check_arch(). > > > > > > > > > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is > > > > > enabled before loading a 32bit process. > > > > > > > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > > > > > Why can't you make system_supports_32bit_el0() take the option into account > > > > instead? > > > > > > > > > > I may be wrong here, but it seems to me that system_supports_32bit_el0() > > > answers teh question "can this system supports compat execution?" rather than > > > "do I want this system to run any compat execution?". That's the point of > > > aarch32_enabled(), to state whether we want teh system to run A32 code or not, > > > regardless of the system supporting it (of course, if the system does not > > > support A32 in EL0, this is a no-no, but that's another story). > > > > That's what the implementation does today, but we're really using it as a "do > > we intend for 32-bit EL0 to work?" predicate, and generally the > > system_supports_${FEATURE}() helpers are affected by the combination of actual > > HW support, kernel config options, *and* kernel command line options. For > > example, system_supports_sve() is affected by both CONFIG_ARM64_SVE and the > > "arm64.nosve" command line option. > > > > Thanks, > > Mark. > > Many thanks for the explanation, then inserting aach32_enabled() in > system_supports_32bit_el0() is the way to go. I think what we should do here is clean up the way we implement system_supports_32bit_el0() such that it can be a cpucap, and have the conditions that would affect aarch32_enabled() feed into that. That way, system_supports_32bit_el0() will compile down to a single branch/nop (or elided entirely when known to be false at compile-time), and with that I think can reasonably fold the existing UNHANDLED() logic into the entry-common.c exception handlers as a simplification. The only obviously painful part is that enable_mismatched_32bit_el0() allows (mismatched) AArch32 support to be enabled after we finalize system cpucaps, as part of a late hotplug. I suspect that was implemented that way for expedience rather than because we wanted to enable mismatched AArch32 after finalizing cpucaps. Will, do you remember why we used a cpuhp callback for enabling mismatched 32-bit support? I couldn't see anything explicit in the commit message for: 2122a833316f2f3f ("arm64: Allow mismatched 32-bit EL0 support") ... and I suspect it was just easier to write that way, rather than adding more code around setup_system_capabilities() ? Mark.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 657ea273c0f9..96832f1ec3ee 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -601,7 +601,7 @@ unsigned long arch_align_stack(unsigned long sp) #ifdef CONFIG_COMPAT int compat_elf_check_arch(const struct elf32_hdr *hdr) { - if (!system_supports_32bit_el0()) + if (!system_supports_32bit_el0() || !aarch32_enabled()) return false; if ((hdr)->e_machine != EM_ARM)
Major aspect of Aarch32 emulation is the ability to load 32bit processes. That's currently decided (among others) by compat_elf_check_arch(). Make the macro use aarch32_enabled() to decide if Aarch32 compat is enabled before loading a 32bit process. Signed-off-by: Andrea della Porta <andrea.porta@suse.com> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)