* [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems @ 2020-10-21 10:46 Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 1/4] arm64: kvm: Handle " Qais Yousef ` (4 more replies) 0 siblings, 5 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 10:46 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel) Cc: Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch, Qais Yousef This series adds basic support for Asymmetric AArch32 systems. Full rationale is in v1's cover letter. https://lore.kernel.org/linux-arch/20201008181641.32767-1-qais.yousef@arm.com/ Changes in v2: * We now reset vcpu->arch.target to force re-initialized for KVM patch. (Marc) * Fix a bug where this_cpu_has_cap() must be called with preemption disabled in check_aarch32_cpumask(). * Add new sysctl.enable_asym_32bit. (Catalin) * Export id_aar64fpr0 register in sysfs which allows user space to discover which cpus support 32bit@EL0. The sysctl must be enabled for the user space to discover the asymmetry. (Will/Catalin) * Fixing up affinity in the kernel approach was dropped. The support assumes the user space that wants to enable this support knows how to guarantee correct affinities for 32bit apps by using cpusets. Open questions: * Should there be any extra handling at execve() time? At the moment we allow the app to start and only SIGKILL it after it has moved to the 'wrong' cpu. We could be stricter and do the check earlier when the elf is loaded. Thanks -- Qais Yousef Qais Yousef (4): arm64: kvm: Handle Asymmetric AArch32 systems arm64: Add support for asymmetric AArch32 EL0 configurations arm64: export emulate_sys_reg() arm64: Export id_aar64fpr0 via sysfs Documentation/arm64/cpu-feature-registers.rst | 2 +- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/cpucaps.h | 3 +- arch/arm64/include/asm/cpufeature.h | 22 +++- arch/arm64/include/asm/thread_info.h | 5 +- arch/arm64/kernel/cpufeature.c | 72 ++++++----- arch/arm64/kernel/cpuinfo.c | 116 +++++++++++++----- arch/arm64/kernel/process.c | 17 +++ arch/arm64/kernel/signal.c | 24 ++++ arch/arm64/kvm/arm.c | 14 +++ arch/arm64/kvm/guest.c | 2 +- arch/arm64/kvm/sys_regs.c | 8 +- kernel/sysctl.c | 11 ++ 13 files changed, 226 insertions(+), 71 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef @ 2020-10-21 10:46 ` Qais Yousef 2020-10-21 12:02 ` Marc Zyngier 2020-10-21 10:46 ` [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations Qais Yousef ` (3 subsequent siblings) 4 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 10:46 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel) Cc: Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch, Qais Yousef On a system without uniform support for AArch32 at EL0, it is possible for the guest to force run AArch32 at EL0 and potentially cause an illegal exception if running on the wrong core. Add an extra check to catch if the guest ever does that and prevent it from running again by resetting vcpu->arch.target and return ARM_EXCEPTION_IL. We try to catch this misbehavior as early as possible and not rely on PSTATE.IL to occur. Tested on Juno by instrumenting the host to: * Fake asym aarch32. * Instrument KVM to make the asymmetry visible to the guest. Any attempt to run 32bit app in the guest will produce such error on qemu: # ./test error: kvm run failed Invalid argument PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 PSTATE=60000005 -ZC- EL1h qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT Aborted Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- arch/arm64/kvm/arm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index b588c3b5c2f0..c2fa57f56a94 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -804,6 +804,19 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) preempt_enable(); + /* + * The ARMv8 architecture doesn't give the hypervisor + * a mechanism to prevent a guest from dropping to AArch32 EL0 + * if implemented by the CPU. If we spot the guest in such + * state and that we decided it wasn't supposed to do so (like + * with the asymmetric AArch32 case), return to userspace with + * a fatal error. + */ + if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) { + vcpu->arch.target = -1; + ret = ARM_EXCEPTION_IL; + } + ret = handle_exit(vcpu, ret); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 10:46 ` [RFC PATCH v2 1/4] arm64: kvm: Handle " Qais Yousef @ 2020-10-21 12:02 ` Marc Zyngier 2020-10-21 13:35 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Marc Zyngier @ 2020-10-21 12:02 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 2020-10-21 11:46, Qais Yousef wrote: > On a system without uniform support for AArch32 at EL0, it is possible > for the guest to force run AArch32 at EL0 and potentially cause an > illegal exception if running on the wrong core. s/the wrong core/a core without AArch32/ > > Add an extra check to catch if the guest ever does that and prevent it Not "if the guest ever does that". Rather "let's hope we are lucky enough to catch the guest doing that". > from running again by resetting vcpu->arch.target and return > ARM_EXCEPTION_IL. > > We try to catch this misbehavior as early as possible and not rely on > PSTATE.IL to occur. > > Tested on Juno by instrumenting the host to: > > * Fake asym aarch32. > * Instrument KVM to make the asymmetry visible to the guest. > > Any attempt to run 32bit app in the guest will produce such error on > qemu: Not *any* attempt. Only the ones where the guest exits whilst in AArch32 EL0. It is perfectly possible for the guest to use AArch32 undetected for quite a while. > > # ./test > error: kvm run failed Invalid argument > PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 > X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 > X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 > X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 > X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 > X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 > X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 > X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 > X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 > X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 > X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 > PSTATE=60000005 -ZC- EL1h > qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT It'd be worth working out: - why does this show an AArch64 mode it we caught the vcpu in AArch32? - why does QEMU shout about the timer register? > Aborted > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > arch/arm64/kvm/arm.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index b588c3b5c2f0..c2fa57f56a94 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -804,6 +804,19 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > > preempt_enable(); > > + /* > + * The ARMv8 architecture doesn't give the hypervisor > + * a mechanism to prevent a guest from dropping to AArch32 EL0 > + * if implemented by the CPU. If we spot the guest in such > + * state and that we decided it wasn't supposed to do so (like > + * with the asymmetric AArch32 case), return to userspace with > + * a fatal error. > + */ Please add a comment explaining the effects of setting target to -1. Something like: "As we have caught the guest red-handed, decide that it isn't fit for purpose anymore by making the4 vcpu invalid. The VMM can try and fix it by issuing a KVM_ARM_VCPU_INIT if it really wants to." > + if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) { > + vcpu->arch.target = -1; > + ret = ARM_EXCEPTION_IL; > + } > + > ret = handle_exit(vcpu, ret); > } M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 12:02 ` Marc Zyngier @ 2020-10-21 13:35 ` Qais Yousef 2020-10-21 13:51 ` Marc Zyngier 0 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:35 UTC (permalink / raw) To: Marc Zyngier Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 13:02, Marc Zyngier wrote: > On 2020-10-21 11:46, Qais Yousef wrote: > > On a system without uniform support for AArch32 at EL0, it is possible > > for the guest to force run AArch32 at EL0 and potentially cause an > > illegal exception if running on the wrong core. > > s/the wrong core/a core without AArch32/ > > > > > Add an extra check to catch if the guest ever does that and prevent it > > Not "if the guest ever does that". Rather "let's hope we are lucky enough > to catch the guest doing that". > > > from running again by resetting vcpu->arch.target and return > > ARM_EXCEPTION_IL. > > > > We try to catch this misbehavior as early as possible and not rely on > > PSTATE.IL to occur. > > > > Tested on Juno by instrumenting the host to: > > > > * Fake asym aarch32. > > * Instrument KVM to make the asymmetry visible to the guest. > > > > Any attempt to run 32bit app in the guest will produce such error on > > qemu: > > Not *any* attempt. Only the ones where the guest exits whilst in > AArch32 EL0. It is perfectly possible for the guest to use AArch32 > undetected for quite a while. Thanks Marc! I'll change them all. > > > > # ./test > > error: kvm run failed Invalid argument > > PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 > > X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 > > X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 > > X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 > > X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 > > X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 > > X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 > > X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 > > X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 > > X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 > > X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 > > PSTATE=60000005 -ZC- EL1h > > qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT > > It'd be worth working out: > - why does this show an AArch64 mode it we caught the vcpu in AArch32? > - why does QEMU shout about the timer register? /me puts a monocular on Which bit is the AArch64? It did surprise me that it is shouting about the timer. My guess was that a timer interrupt at the guest between exit/reentry caused the state change and the failure to read the timer register? Since the target is no longer valid it falls over, hopefully as expected. I could have been naive of course. That explanation made sense to my mind so I didn't dig further. > > Aborted > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > --- > > arch/arm64/kvm/arm.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index b588c3b5c2f0..c2fa57f56a94 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -804,6 +804,19 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > > > > preempt_enable(); > > > > + /* > > + * The ARMv8 architecture doesn't give the hypervisor > > + * a mechanism to prevent a guest from dropping to AArch32 EL0 > > + * if implemented by the CPU. If we spot the guest in such > > + * state and that we decided it wasn't supposed to do so (like > > + * with the asymmetric AArch32 case), return to userspace with > > + * a fatal error. > > + */ > > Please add a comment explaining the effects of setting target to -1. > Something > like: > > "As we have caught the guest red-handed, decide that it isn't fit for > purpose > anymore by making the4 vcpu invalid. The VMM can try and fix it by issuing > a KVM_ARM_VCPU_INIT if it really wants to." Will do. Thanks -- Qais Yousef > > > + if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) { > > + vcpu->arch.target = -1; > > + ret = ARM_EXCEPTION_IL; > > + } > > + > > ret = handle_exit(vcpu, ret); > > } > > M. > -- > Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 13:35 ` Qais Yousef @ 2020-10-21 13:51 ` Marc Zyngier 2020-10-21 14:38 ` Qais Yousef 2020-11-02 17:58 ` Qais Yousef 0 siblings, 2 replies; 57+ messages in thread From: Marc Zyngier @ 2020-10-21 13:51 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 2020-10-21 14:35, Qais Yousef wrote: > On 10/21/20 13:02, Marc Zyngier wrote: >> On 2020-10-21 11:46, Qais Yousef wrote: >> > On a system without uniform support for AArch32 at EL0, it is possible >> > for the guest to force run AArch32 at EL0 and potentially cause an >> > illegal exception if running on the wrong core. >> >> s/the wrong core/a core without AArch32/ >> >> > >> > Add an extra check to catch if the guest ever does that and prevent it >> >> Not "if the guest ever does that". Rather "let's hope we are lucky >> enough >> to catch the guest doing that". >> >> > from running again by resetting vcpu->arch.target and return >> > ARM_EXCEPTION_IL. >> > >> > We try to catch this misbehavior as early as possible and not rely on >> > PSTATE.IL to occur. >> > >> > Tested on Juno by instrumenting the host to: >> > >> > * Fake asym aarch32. >> > * Instrument KVM to make the asymmetry visible to the guest. >> > >> > Any attempt to run 32bit app in the guest will produce such error on >> > qemu: >> >> Not *any* attempt. Only the ones where the guest exits whilst in >> AArch32 EL0. It is perfectly possible for the guest to use AArch32 >> undetected for quite a while. > > Thanks Marc! I'll change them all. > >> > >> > # ./test >> > error: kvm run failed Invalid argument >> > PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 >> > X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 >> > X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 >> > X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 >> > X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 >> > X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 >> > X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 >> > X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 >> > X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 >> > X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 >> > X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 >> > PSTATE=60000005 -ZC- EL1h >> > qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT >> >> It'd be worth working out: >> - why does this show an AArch64 mode it we caught the vcpu in AArch32? >> - why does QEMU shout about the timer register? > > /me puts a monocular on > > Which bit is the AArch64? It clearly spits out "EL1h", and PSTATE.M is 5, also consistent with EL1h. > It did surprise me that it is shouting about the timer. My guess was > that > a timer interrupt at the guest between exit/reentry caused the state > change and > the failure to read the timer register? Since the target is no longer > valid it > falls over, hopefully as expected. I could have been naive of course. > That > explanation made sense to my mind so I didn't dig further. Userspace is never involved with the timer interrupt, unless you've elected to have the interrupt controller in userspace, which I seriously doubt. As we are introducing a change to the userspace ABI, it'd be interesting to find out what is happening here. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 13:51 ` Marc Zyngier @ 2020-10-21 14:38 ` Qais Yousef 2020-11-02 17:58 ` Qais Yousef 1 sibling, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 14:38 UTC (permalink / raw) To: Marc Zyngier Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 14:51, Marc Zyngier wrote: > On 2020-10-21 14:35, Qais Yousef wrote: > > On 10/21/20 13:02, Marc Zyngier wrote: > > > On 2020-10-21 11:46, Qais Yousef wrote: > > > > On a system without uniform support for AArch32 at EL0, it is possible > > > > for the guest to force run AArch32 at EL0 and potentially cause an > > > > illegal exception if running on the wrong core. > > > > > > s/the wrong core/a core without AArch32/ > > > > > > > > > > > Add an extra check to catch if the guest ever does that and prevent it > > > > > > Not "if the guest ever does that". Rather "let's hope we are lucky > > > enough > > > to catch the guest doing that". > > > > > > > from running again by resetting vcpu->arch.target and return > > > > ARM_EXCEPTION_IL. > > > > > > > > We try to catch this misbehavior as early as possible and not rely on > > > > PSTATE.IL to occur. > > > > > > > > Tested on Juno by instrumenting the host to: > > > > > > > > * Fake asym aarch32. > > > > * Instrument KVM to make the asymmetry visible to the guest. > > > > > > > > Any attempt to run 32bit app in the guest will produce such error on > > > > qemu: > > > > > > Not *any* attempt. Only the ones where the guest exits whilst in > > > AArch32 EL0. It is perfectly possible for the guest to use AArch32 > > > undetected for quite a while. > > > > Thanks Marc! I'll change them all. > > > > > > > > > > # ./test > > > > error: kvm run failed Invalid argument > > > > PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 > > > > X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 > > > > X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 > > > > X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 > > > > X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 > > > > X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 > > > > X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 > > > > X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 > > > > X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 > > > > X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 > > > > X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 > > > > PSTATE=60000005 -ZC- EL1h > > > > qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT > > > > > > It'd be worth working out: > > > - why does this show an AArch64 mode it we caught the vcpu in AArch32? > > > - why does QEMU shout about the timer register? > > > > /me puts a monocular on > > > > Which bit is the AArch64? > > It clearly spits out "EL1h", and PSTATE.M is 5, also consistent with EL1h. > > > It did surprise me that it is shouting about the timer. My guess was > > that > > a timer interrupt at the guest between exit/reentry caused the state > > change and > > the failure to read the timer register? Since the target is no longer > > valid it > > falls over, hopefully as expected. I could have been naive of course. > > That > > explanation made sense to my mind so I didn't dig further. > > Userspace is never involved with the timer interrupt, unless you've elected > to have the interrupt controller in userspace, which I seriously doubt. > > As we are introducing a change to the userspace ABI, it'd be interesting > to find out what is happening here. Sure. Let me educate myself more about this and find a way to interrogate qemu and KVM. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 1/4] arm64: kvm: Handle Asymmetric AArch32 systems 2020-10-21 13:51 ` Marc Zyngier 2020-10-21 14:38 ` Qais Yousef @ 2020-11-02 17:58 ` Qais Yousef 1 sibling, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-11-02 17:58 UTC (permalink / raw) To: Marc Zyngier Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch Hi Marc On 10/21/20 14:51, Marc Zyngier wrote: > > > > > > > > # ./test > > > > error: kvm run failed Invalid argument > > > > PC=ffff800010945080 X00=ffff800016a45014 X01=ffff800010945058 > > > > X02=ffff800016917190 X03=0000000000000000 X04=0000000000000000 > > > > X05=00000000fffffffb X06=0000000000000000 X07=ffff80001000bab0 > > > > X08=0000000000000000 X09=0000000092ec5193 X10=0000000000000000 > > > > X11=ffff80001608ff40 X12=ffff000075fcde86 X13=ffff000075fcde88 > > > > X14=ffffffffffffffff X15=ffff00007b2105a8 X16=ffff00007b006d50 > > > > X17=0000000000000000 X18=0000000000000000 X19=ffff00007a82b000 > > > > X20=0000000000000000 X21=ffff800015ccd158 X22=ffff00007a82b040 > > > > X23=ffff00007a82b008 X24=0000000000000000 X25=ffff800015d169b0 > > > > X26=ffff8000126d05bc X27=0000000000000000 X28=0000000000000000 > > > > X29=ffff80001000ba90 X30=ffff80001093f3dc SP=ffff80001000ba90 > > > > PSTATE=60000005 -ZC- EL1h > > > > qemu-system-aarch64: Failed to get KVM_REG_ARM_TIMER_CNT > > > > > > It'd be worth working out: > > > - why does this show an AArch64 mode it we caught the vcpu in AArch32? > > > - why does QEMU shout about the timer register? > > > > /me puts a monocular on > > > > Which bit is the AArch64? > > It clearly spits out "EL1h", and PSTATE.M is 5, also consistent with EL1h. Apologies for the delay to look at the reason on failing to read the timer register. Digging into the qemu 5.0.0 code, the error message is printed from kvm_arm_get_virtual_time() which in turn is called from kvm_arm_vm_state_change(). The latter is a callback function that is called when a vm starts/stop. So the sequence of events is: VM runs 32bit apps host resets vcpu->arch.target to -1 qemu::kvm_cpu_exec() hits -EINVAL error (somewhere I didn't trace) kvm_cpu_exec()::cpu_dump_state() kvm_cpu_exec()::vm_stop() .. kvm_arm_vm_state_change() kvm_arm_get_virtual_time() host return -ENOEXEC above error message is printed abort() I admit I didn't trace qemu to see what's going inside it. It was only statically analysing the code. To verify the theory I applied the following hack to hide the timer register error diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 5d2a1caf55a0..1c8fdf6566ea 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1113,7 +1113,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp, case KVM_GET_ONE_REG: { struct kvm_one_reg reg; - r = -ENOEXEC; + r = 0; if (unlikely(!kvm_vcpu_initialized(vcpu))) break; With that I see the following error from qemu after which it seems to have 'hanged'. I can terminate qemu with the usual <ctrl-a>x. So it's not dead, just the vm has exited I suppose and qemu went into monitor mode or something. error: kvm run failed Invalid argument PC=ffff8000109ca100 X00=ffff800016ff5014 X01=ffff8000109ca0d8 X02=ffff800016daae80 X03=0000000000000000 X04=0000000000000003 X05=0000000000000000 X06=0000000000000000 X07=ffff800016e2bae0 X08=00000000ffffffff X09=ffff8000109c4410 X10=0000000000000000 X11=ffff8000164fb9c8 X12=ffff0000458ad186 X13=ffff0000458ad188 X14=ffffffffffffffff X15=ffff000040268560 X16=0000000000000000 X17=0000000000000001 X18=0000000000000000 X19=ffff0000458c0000 X20=0000000000000000 X21=ffff0000458c0048 X22=ffff0000458c0008 X23=ffff800016103a38 X24=0000000000000000 X25=ffff800016150a38 X26=ffff800012a510d8 X27=ffff8000129504e0 X28=0000000000000000 X29=ffff800016e2bac0 X30=ffff8000109c4410 SP=ffff000040268000 PSTATE=834853a0 N--- EL0t Which hopefully is what you expected to see in the first place. Note that qemu v4.1.0 code didn't have this kvm_arm_get_virtual_time() function. It seems to be a relatively new addition. Also note that kvm_cpu_exec() in qemu completely ignores ARM_EXCEPTION_IL; the kvm_arch_handle_exit() for arm only catches KVM_EXIT_DEBUG and returns 0 for everything else. So kvm_cpu_exec() will jump back the loop to reenter the guest. I haven't traced it but it seems to fail before calling: run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0); where return -ENOEXEC for invalid kvm_vcpu_initialized() in this path not -EINVAL. So all in all, a lot of qemu specific handling and not sure if there's any guarantee how things will fail for different virtualization software. But I think there's a guarantee that they will fail. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 1/4] arm64: kvm: Handle " Qais Yousef @ 2020-10-21 10:46 ` Qais Yousef 2020-10-21 15:39 ` Will Deacon 2020-10-21 10:46 ` [RFC PATCH v2 3/4] arm64: export emulate_sys_reg() Qais Yousef ` (2 subsequent siblings) 4 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 10:46 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel) Cc: Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch, Qais Yousef When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. The kernel will now return true for system_supports_32bit_el0() and checks 32-bit tasks are affined to AArch32 capable CPUs only in do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, the tasks will get SIGKILLed. If the last CPU supporting 32-bit is offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the alternative is to prevent offlining through a new .cpu_disable feature entry). In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, this patch factors out the 32-bit cpuinfo and features setting into separate functions: __cpuinfo_store_cpu_32bit(), init_cpu_32bit_features(). The cpuinfo of the booting CPU (boot_cpu_data) is now updated on the first 32-bit capable CPU even if it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support is enabled. The compat_elf_hwcaps are only verified for the AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. Make sure that KVM never sees the asymmetric 32bit system. Guest can still ignore ID registers and force run 32bit at EL0. Co-developed-by: Qais Yousef <qais.yousef@arm.com> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/cpucaps.h | 3 +- arch/arm64/include/asm/cpufeature.h | 19 +++++++- arch/arm64/include/asm/thread_info.h | 5 ++- arch/arm64/kernel/cpufeature.c | 66 ++++++++++++++++------------ arch/arm64/kernel/cpuinfo.c | 58 +++++++++++++----------- arch/arm64/kernel/process.c | 17 +++++++ arch/arm64/kernel/signal.c | 24 ++++++++++ arch/arm64/kvm/arm.c | 3 +- arch/arm64/kvm/guest.c | 2 +- arch/arm64/kvm/sys_regs.c | 4 +- kernel/sysctl.c | 11 +++++ 12 files changed, 153 insertions(+), 60 deletions(-) diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h index 7faae6ff3ab4..3c5c1633a565 100644 --- a/arch/arm64/include/asm/cpu.h +++ b/arch/arm64/include/asm/cpu.h @@ -65,6 +65,7 @@ void cpuinfo_store_cpu(void); void __init cpuinfo_store_boot_cpu(void); void __init init_cpu_features(struct cpuinfo_arm64 *info); +void init_cpu_32bit_features(struct cpuinfo_arm64 *info); void update_cpu_features(int cpu, struct cpuinfo_arm64 *info, struct cpuinfo_arm64 *boot); diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index 07b643a70710..d3b6a5dce456 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -64,7 +64,8 @@ #define ARM64_BTI 54 #define ARM64_HAS_ARMv8_4_TTL 55 #define ARM64_HAS_TLB_RANGE 56 +#define ARM64_HAS_ASYM_32BIT_EL0 57 -#define ARM64_NCAPS 57 +#define ARM64_NCAPS 58 #endif /* __ASM_CPUCAPS_H */ diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 89b4f0142c28..338637907e6a 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -17,6 +17,7 @@ #ifndef __ASSEMBLY__ #include <linux/bug.h> +#include <linux/cpumask.h> #include <linux/jump_label.h> #include <linux/kernel.h> @@ -79,6 +80,8 @@ struct arm64_ftr_reg { extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; +extern unsigned int __read_mostly sysctl_enable_asym_32bit; + /* * CPU capabilities: * @@ -582,9 +585,23 @@ static inline bool cpu_supports_mixed_endian_el0(void) return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1)); } -static inline bool system_supports_32bit_el0(void) +static inline bool system_supports_sym_32bit_el0(void) { return cpus_have_const_cap(ARM64_HAS_32BIT_EL0); + +} + +static inline bool system_supports_asym_32bit_el0(void) +{ + return sysctl_enable_asym_32bit && + !cpus_have_const_cap(ARM64_HAS_32BIT_EL0) && + cpus_have_const_cap(ARM64_HAS_ASYM_32BIT_EL0); +} + +static inline bool system_supports_32bit_el0(void) +{ + return system_supports_sym_32bit_el0() || + system_supports_asym_32bit_el0(); } static inline bool system_supports_4kb_granule(void) diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index 5e784e16ee89..312974ab2c85 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ #define TIF_SYSCALL_TRACE 8 /* syscall trace active */ #define TIF_SYSCALL_AUDIT 9 /* syscall auditing */ #define TIF_SYSCALL_TRACEPOINT 10 /* syscall tracepoint for ftrace */ @@ -95,11 +96,13 @@ void arch_release_task_struct(struct task_struct *tsk); #define _TIF_FSCHECK (1 << TIF_FSCHECK) #define _TIF_SINGLESTEP (1 << TIF_SINGLESTEP) #define _TIF_32BIT (1 << TIF_32BIT) +#define _TIF_CHECK_32BIT_AFFINITY (1 << TIF_CHECK_32BIT_AFFINITY) #define _TIF_SVE (1 << TIF_SVE) #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ - _TIF_UPROBE | _TIF_FSCHECK) + _TIF_UPROBE | _TIF_FSCHECK | \ + _TIF_CHECK_32BIT_AFFINITY) #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 6424584be01e..1bbdfa9e911d 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -63,7 +63,6 @@ #define pr_fmt(fmt) "CPU features: " fmt #include <linux/bsearch.h> -#include <linux/cpumask.h> #include <linux/crash_dump.h> #include <linux/sort.h> #include <linux/stop_machine.h> @@ -93,6 +92,8 @@ unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT; unsigned int compat_elf_hwcap2 __read_mostly; #endif +unsigned int __read_mostly sysctl_enable_asym_32bit; + DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS); EXPORT_SYMBOL(cpu_hwcaps); static struct arm64_cpu_capabilities const __ro_after_init *cpu_hwcaps_ptrs[ARM64_NCAPS]; @@ -753,7 +754,7 @@ static void __init sort_ftr_regs(void) * Any bits that are not covered by an arm64_ftr_bits entry are considered * RES0 for the system-wide value, and must strictly match. */ -static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) +static void init_cpu_ftr_reg(u32 sys_reg, u64 new) { u64 val = 0; u64 strict_mask = ~0x0ULL; @@ -835,30 +836,6 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info) init_cpu_ftr_reg(SYS_ID_AA64PFR1_EL1, info->reg_id_aa64pfr1); init_cpu_ftr_reg(SYS_ID_AA64ZFR0_EL1, info->reg_id_aa64zfr0); - if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) { - init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0); - init_cpu_ftr_reg(SYS_ID_DFR1_EL1, info->reg_id_dfr1); - init_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0); - init_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1); - init_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2); - init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3); - init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4); - init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5); - init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6); - init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0); - init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1); - init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2); - init_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3); - init_cpu_ftr_reg(SYS_ID_MMFR4_EL1, info->reg_id_mmfr4); - init_cpu_ftr_reg(SYS_ID_MMFR5_EL1, info->reg_id_mmfr5); - init_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0); - init_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1); - init_cpu_ftr_reg(SYS_ID_PFR2_EL1, info->reg_id_pfr2); - init_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0); - init_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1); - init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2); - } - if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) { init_cpu_ftr_reg(SYS_ZCR_EL1, info->reg_zcr); sve_init_vq_map(); @@ -877,6 +854,31 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info) setup_boot_cpu_capabilities(); } +void init_cpu_32bit_features(struct cpuinfo_arm64 *info) +{ + init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0); + init_cpu_ftr_reg(SYS_ID_DFR1_EL1, info->reg_id_dfr1); + init_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0); + init_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1); + init_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2); + init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3); + init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4); + init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5); + init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6); + init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0); + init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1); + init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2); + init_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3); + init_cpu_ftr_reg(SYS_ID_MMFR4_EL1, info->reg_id_mmfr4); + init_cpu_ftr_reg(SYS_ID_MMFR5_EL1, info->reg_id_mmfr5); + init_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0); + init_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1); + init_cpu_ftr_reg(SYS_ID_PFR2_EL1, info->reg_id_pfr2); + init_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0); + init_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1); + init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2); +} + static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new) { const struct arm64_ftr_bits *ftrp; @@ -1804,6 +1806,15 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .field_pos = ID_AA64PFR0_EL0_SHIFT, .min_field_value = ID_AA64PFR0_EL0_32BIT_64BIT, }, + { + .capability = ARM64_HAS_ASYM_32BIT_EL0, + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64PFR0_EL1, + .sign = FTR_UNSIGNED, + .field_pos = ID_AA64PFR0_EL0_SHIFT, + .min_field_value = ID_AA64PFR0_EL0_32BIT_64BIT, + }, #ifdef CONFIG_KVM { .desc = "32-bit EL1 Support", @@ -2576,7 +2587,8 @@ static void verify_local_cpu_capabilities(void) verify_local_elf_hwcaps(arm64_elf_hwcaps); - if (system_supports_32bit_el0()) + if (system_supports_32bit_el0() && + this_cpu_has_cap(ARM64_HAS_32BIT_EL0)) verify_local_elf_hwcaps(compat_elf_hwcaps); if (system_supports_sve()) diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index d0076c2159e6..93c55986ca7f 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -362,32 +362,6 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64pfr1 = read_cpuid(ID_AA64PFR1_EL1); info->reg_id_aa64zfr0 = read_cpuid(ID_AA64ZFR0_EL1); - /* Update the 32bit ID registers only if AArch32 is implemented */ - if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) { - info->reg_id_dfr0 = read_cpuid(ID_DFR0_EL1); - info->reg_id_dfr1 = read_cpuid(ID_DFR1_EL1); - info->reg_id_isar0 = read_cpuid(ID_ISAR0_EL1); - info->reg_id_isar1 = read_cpuid(ID_ISAR1_EL1); - info->reg_id_isar2 = read_cpuid(ID_ISAR2_EL1); - info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1); - info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1); - info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1); - info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1); - info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1); - info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1); - info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1); - info->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1); - info->reg_id_mmfr4 = read_cpuid(ID_MMFR4_EL1); - info->reg_id_mmfr5 = read_cpuid(ID_MMFR5_EL1); - info->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1); - info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1); - info->reg_id_pfr2 = read_cpuid(ID_PFR2_EL1); - - info->reg_mvfr0 = read_cpuid(MVFR0_EL1); - info->reg_mvfr1 = read_cpuid(MVFR1_EL1); - info->reg_mvfr2 = read_cpuid(MVFR2_EL1); - } - if (IS_ENABLED(CONFIG_ARM64_SVE) && id_aa64pfr0_sve(info->reg_id_aa64pfr0)) info->reg_zcr = read_zcr_features(); @@ -395,10 +369,38 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) cpuinfo_detect_icache_policy(info); } +static void __cpuinfo_store_cpu_32bit(struct cpuinfo_arm64 *info) +{ + info->reg_id_dfr0 = read_cpuid(ID_DFR0_EL1); + info->reg_id_dfr1 = read_cpuid(ID_DFR1_EL1); + info->reg_id_isar0 = read_cpuid(ID_ISAR0_EL1); + info->reg_id_isar1 = read_cpuid(ID_ISAR1_EL1); + info->reg_id_isar2 = read_cpuid(ID_ISAR2_EL1); + info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1); + info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1); + info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1); + info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1); + info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1); + info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1); + info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1); + info->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1); + info->reg_id_mmfr4 = read_cpuid(ID_MMFR4_EL1); + info->reg_id_mmfr5 = read_cpuid(ID_MMFR5_EL1); + info->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1); + info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1); + info->reg_id_pfr2 = read_cpuid(ID_PFR2_EL1); + + info->reg_mvfr0 = read_cpuid(MVFR0_EL1); + info->reg_mvfr1 = read_cpuid(MVFR1_EL1); + info->reg_mvfr2 = read_cpuid(MVFR2_EL1); +} + void cpuinfo_store_cpu(void) { struct cpuinfo_arm64 *info = this_cpu_ptr(&cpu_data); __cpuinfo_store_cpu(info); + if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) + __cpuinfo_store_cpu_32bit(info); update_cpu_features(smp_processor_id(), info, &boot_cpu_data); } @@ -406,7 +408,11 @@ void __init cpuinfo_store_boot_cpu(void) { struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0); __cpuinfo_store_cpu(info); + if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) + __cpuinfo_store_cpu_32bit(info); boot_cpu_data = *info; init_cpu_features(&boot_cpu_data); + if (id_aa64pfr0_32bit_el0(boot_cpu_data.reg_id_aa64pfr0)) + init_cpu_32bit_features(&boot_cpu_data); } diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index f1804496b935..04ea32b3c34c 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -513,6 +513,15 @@ static void entry_task_switch(struct task_struct *next) __this_cpu_write(__entry_task, next); } +static void aarch32_thread_switch(struct task_struct *next) +{ + struct thread_info *ti = task_thread_info(next); + + if (system_supports_asym_32bit_el0() && is_compat_thread(ti) && + !this_cpu_has_cap(ARM64_HAS_32BIT_EL0)) + set_ti_thread_flag(ti, TIF_CHECK_32BIT_AFFINITY); +} + /* * ARM erratum 1418040 handling, affecting the 32bit view of CNTVCT. * Assuming the virtual counter is enabled at the beginning of times: @@ -562,6 +571,7 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, uao_thread_switch(next); ssbs_thread_switch(next); erratum_1418040_thread_switch(prev, next); + aarch32_thread_switch(next); /* * Complete any pending TLB or cache maintenance on this CPU in case @@ -620,6 +630,13 @@ void arch_setup_new_exec(void) current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; ptrauth_thread_init_user(current); + + /* + * If exec'ing a 32-bit task, force the asymmetric 32-bit feature + * check as the task may not go through a switch_to() call. + */ + if (system_supports_asym_32bit_el0() && is_compat_task()) + set_thread_flag(TIF_CHECK_32BIT_AFFINITY); } #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 3b4f31f35e45..c62951b2507a 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -8,6 +8,7 @@ #include <linux/cache.h> #include <linux/compat.h> +#include <linux/cpumask.h> #include <linux/errno.h> #include <linux/kernel.h> #include <linux/signal.h> @@ -907,6 +908,23 @@ static void do_signal(struct pt_regs *regs) restore_saved_sigmask(); } +static void check_aarch32_cpumask(void) +{ + bool supports_32bit_el0; + + preempt_disable(); + supports_32bit_el0 = this_cpu_has_cap(ARM64_HAS_32BIT_EL0); + preempt_enable(); + + /* + * If the task moved to uncapable CPU, SIGKILL it. + */ + if (!supports_32bit_el0) { + pr_warn_once("CPU affinity contains CPUs that are not capable of running 32-bit tasks\n"); + force_sig(SIGKILL); + } +} + asmlinkage void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) { @@ -929,6 +947,12 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, } else { local_daif_restore(DAIF_PROCCTX); + if (system_supports_asym_32bit_el0() && + thread_flags & _TIF_CHECK_32BIT_AFFINITY) { + clear_thread_flag(TIF_CHECK_32BIT_AFFINITY); + check_aarch32_cpumask(); + } + if (thread_flags & _TIF_UPROBE) uprobe_notify_resume(regs); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index c2fa57f56a94..6db9a1a27e98 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -812,7 +812,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) * with the asymmetric AArch32 case), return to userspace with * a fatal error. */ - if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) { + if (!system_supports_sym_32bit_el0() && + vcpu_mode_is_32bit(vcpu)) { vcpu->arch.target = -1; ret = ARM_EXCEPTION_IL; } diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index dfb5218137ca..0f67b53eaf17 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -226,7 +226,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) u64 mode = (*(u64 *)valp) & PSR_AA32_MODE_MASK; switch (mode) { case PSR_AA32_MODE_USR: - if (!system_supports_32bit_el0()) + if (!system_supports_sym_32bit_el0()) return -EINVAL; break; case PSR_AA32_MODE_FIQ: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 077293b5115f..c9fb172f9b01 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -670,7 +670,7 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) */ val = ((pmcr & ~ARMV8_PMU_PMCR_MASK) | (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E); - if (!system_supports_32bit_el0()) + if (!system_supports_sym_32bit_el0()) val |= ARMV8_PMU_PMCR_LC; __vcpu_sys_reg(vcpu, r->reg) = val; } @@ -722,7 +722,7 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, val = __vcpu_sys_reg(vcpu, PMCR_EL0); val &= ~ARMV8_PMU_PMCR_MASK; val |= p->regval & ARMV8_PMU_PMCR_MASK; - if (!system_supports_32bit_el0()) + if (!system_supports_sym_32bit_el0()) val |= ARMV8_PMU_PMCR_LC; __vcpu_sys_reg(vcpu, PMCR_EL0) = val; kvm_pmu_handle_pmcr(vcpu, val); diff --git a/kernel/sysctl.c b/kernel/sysctl.c index afad085960b8..acdf71d8a5fc 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2466,6 +2466,17 @@ static struct ctl_table kern_table[] = { .proc_handler = proc_dointvec, }, #endif +#if defined(CONFIG_ARM64) && defined(CONFIG_COMPAT) && defined(CONFIG_SMP) + { + .procname = "enable_asym_32bit", + .data = &sysctl_enable_asym_32bit, + .maxlen = sizeof (int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_ONE, + }, +#endif #ifdef CONFIG_DETECT_HUNG_TASK #ifdef CONFIG_SMP { -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 10:46 ` [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations Qais Yousef @ 2020-10-21 15:39 ` Will Deacon 2020-10-21 16:21 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 15:39 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 11:46:09AM +0100, Qais Yousef wrote: > When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type > of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. > The kernel will now return true for system_supports_32bit_el0() and > checks 32-bit tasks are affined to AArch32 capable CPUs only in > do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, > the tasks will get SIGKILLed. If the last CPU supporting 32-bit is > offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the > alternative is to prevent offlining through a new .cpu_disable feature > entry). > > In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, > this patch factors out the 32-bit cpuinfo and features setting into > separate functions: __cpuinfo_store_cpu_32bit(), > init_cpu_32bit_features(). The cpuinfo of the booting CPU > (boot_cpu_data) is now updated on the first 32-bit capable CPU even if > it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed > to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support > is enabled. The compat_elf_hwcaps are only verified for the > AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. > > Make sure that KVM never sees the asymmetric 32bit system. Guest can > still ignore ID registers and force run 32bit at EL0. > > Co-developed-by: Qais Yousef <qais.yousef@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Qais Yousef <qais.yousef@arm.com> [...] > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index 5e784e16ee89..312974ab2c85 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ I've looked through the patch and I still can't figure out why this extra flag is needed. We know if a CPU supports 32-bit EL0, and we know whether or not a task is 32-bit. So why the extra flag? Is it just a hangover from the old series? Cheers, Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 15:39 ` Will Deacon @ 2020-10-21 16:21 ` Qais Yousef 2020-10-21 16:52 ` Catalin Marinas 0 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 16:21 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 16:39, Will Deacon wrote: > On Wed, Oct 21, 2020 at 11:46:09AM +0100, Qais Yousef wrote: > > When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type > > of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. > > The kernel will now return true for system_supports_32bit_el0() and > > checks 32-bit tasks are affined to AArch32 capable CPUs only in > > do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, > > the tasks will get SIGKILLed. If the last CPU supporting 32-bit is > > offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the > > alternative is to prevent offlining through a new .cpu_disable feature > > entry). > > > > In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, > > this patch factors out the 32-bit cpuinfo and features setting into > > separate functions: __cpuinfo_store_cpu_32bit(), > > init_cpu_32bit_features(). The cpuinfo of the booting CPU > > (boot_cpu_data) is now updated on the first 32-bit capable CPU even if > > it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed > > to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support > > is enabled. The compat_elf_hwcaps are only verified for the > > AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. > > > > Make sure that KVM never sees the asymmetric 32bit system. Guest can > > still ignore ID registers and force run 32bit at EL0. > > > > Co-developed-by: Qais Yousef <qais.yousef@arm.com> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > [...] > > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > > index 5e784e16ee89..312974ab2c85 100644 > > --- a/arch/arm64/include/asm/thread_info.h > > +++ b/arch/arm64/include/asm/thread_info.h > > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > > +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ > > I've looked through the patch and I still can't figure out why this extra > flag is needed. We know if a CPU supports 32-bit EL0, and we know whether > or not a task is 32-bit. So why the extra flag? Is it just a hangover from > the old series? It did evolve a bit organically. AFAICS it helps as an optimization to avoid the checks unnecessarily. If it's not expensive to do the checks in the loop in do_notify_resume() we can omit it. We will still protect it with system_supports_asym_32bit_el0() so the check is done on these systems only. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 16:21 ` Qais Yousef @ 2020-10-21 16:52 ` Catalin Marinas 2020-10-21 17:39 ` Will Deacon 0 siblings, 1 reply; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 16:52 UTC (permalink / raw) To: Qais Yousef Cc: Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 05:21:21PM +0100, Qais Yousef wrote: > On 10/21/20 16:39, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 11:46:09AM +0100, Qais Yousef wrote: > > > When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type > > > of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. > > > The kernel will now return true for system_supports_32bit_el0() and > > > checks 32-bit tasks are affined to AArch32 capable CPUs only in > > > do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, > > > the tasks will get SIGKILLed. If the last CPU supporting 32-bit is > > > offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the > > > alternative is to prevent offlining through a new .cpu_disable feature > > > entry). > > > > > > In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, > > > this patch factors out the 32-bit cpuinfo and features setting into > > > separate functions: __cpuinfo_store_cpu_32bit(), > > > init_cpu_32bit_features(). The cpuinfo of the booting CPU > > > (boot_cpu_data) is now updated on the first 32-bit capable CPU even if > > > it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed > > > to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support > > > is enabled. The compat_elf_hwcaps are only verified for the > > > AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. > > > > > > Make sure that KVM never sees the asymmetric 32bit system. Guest can > > > still ignore ID registers and force run 32bit at EL0. > > > > > > Co-developed-by: Qais Yousef <qais.yousef@arm.com> > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > > [...] > > > > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > > > index 5e784e16ee89..312974ab2c85 100644 > > > --- a/arch/arm64/include/asm/thread_info.h > > > +++ b/arch/arm64/include/asm/thread_info.h > > > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > > > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > > > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > > > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > > > +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ > > > > I've looked through the patch and I still can't figure out why this extra > > flag is needed. We know if a CPU supports 32-bit EL0, and we know whether > > or not a task is 32-bit. So why the extra flag? Is it just a hangover from > > the old series? > > It did evolve a bit organically. > > AFAICS it helps as an optimization to avoid the checks unnecessarily. If it's > not expensive to do the checks in the loop in do_notify_resume() we can omit > it. We will still protect it with system_supports_asym_32bit_el0() so the check > is done on these systems only. Ah, I think I remember now. We didn't want ret_to_user (entry.S) to always go the work_pending path if there was no context switch for a 32-bit task. With the AArch32 check in do_notify_resume(), it would mean we add _TIF_32BIT to the _TIF_WORK_MASK. However, we could add an asm alternative if AArch32 asym is detected to always route TIF_32BIT tasks to work_pending. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 16:52 ` Catalin Marinas @ 2020-10-21 17:39 ` Will Deacon 2020-10-22 9:53 ` Catalin Marinas 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 17:39 UTC (permalink / raw) To: Catalin Marinas Cc: Qais Yousef, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 05:52:47PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 05:21:21PM +0100, Qais Yousef wrote: > > On 10/21/20 16:39, Will Deacon wrote: > > > On Wed, Oct 21, 2020 at 11:46:09AM +0100, Qais Yousef wrote: > > > > When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type > > > > of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. > > > > The kernel will now return true for system_supports_32bit_el0() and > > > > checks 32-bit tasks are affined to AArch32 capable CPUs only in > > > > do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, > > > > the tasks will get SIGKILLed. If the last CPU supporting 32-bit is > > > > offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the > > > > alternative is to prevent offlining through a new .cpu_disable feature > > > > entry). > > > > > > > > In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, > > > > this patch factors out the 32-bit cpuinfo and features setting into > > > > separate functions: __cpuinfo_store_cpu_32bit(), > > > > init_cpu_32bit_features(). The cpuinfo of the booting CPU > > > > (boot_cpu_data) is now updated on the first 32-bit capable CPU even if > > > > it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed > > > > to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support > > > > is enabled. The compat_elf_hwcaps are only verified for the > > > > AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. > > > > > > > > Make sure that KVM never sees the asymmetric 32bit system. Guest can > > > > still ignore ID registers and force run 32bit at EL0. > > > > > > > > Co-developed-by: Qais Yousef <qais.yousef@arm.com> > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > > > > [...] > > > > > > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > > > > index 5e784e16ee89..312974ab2c85 100644 > > > > --- a/arch/arm64/include/asm/thread_info.h > > > > +++ b/arch/arm64/include/asm/thread_info.h > > > > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > > > > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > > > > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > > > > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > > > > +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ > > > > > > I've looked through the patch and I still can't figure out why this extra > > > flag is needed. We know if a CPU supports 32-bit EL0, and we know whether > > > or not a task is 32-bit. So why the extra flag? Is it just a hangover from > > > the old series? > > > > It did evolve a bit organically. > > > > AFAICS it helps as an optimization to avoid the checks unnecessarily. If it's > > not expensive to do the checks in the loop in do_notify_resume() we can omit > > it. We will still protect it with system_supports_asym_32bit_el0() so the check > > is done on these systems only. > > Ah, I think I remember now. We didn't want ret_to_user (entry.S) to > always go the work_pending path if there was no context switch for a > 32-bit task. With the AArch32 check in do_notify_resume(), it would mean > we add _TIF_32BIT to the _TIF_WORK_MASK. > > However, we could add an asm alternative if AArch32 asym is detected to > always route TIF_32BIT tasks to work_pending. Or could we just use TIF_NOTIFY_RESUME, like we for for rseq()? Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations 2020-10-21 17:39 ` Will Deacon @ 2020-10-22 9:53 ` Catalin Marinas 0 siblings, 0 replies; 57+ messages in thread From: Catalin Marinas @ 2020-10-22 9:53 UTC (permalink / raw) To: Will Deacon Cc: Qais Yousef, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 06:39:00PM +0100, Will Deacon wrote: > On Wed, Oct 21, 2020 at 05:52:47PM +0100, Catalin Marinas wrote: > > On Wed, Oct 21, 2020 at 05:21:21PM +0100, Qais Yousef wrote: > > > On 10/21/20 16:39, Will Deacon wrote: > > > > On Wed, Oct 21, 2020 at 11:46:09AM +0100, Qais Yousef wrote: > > > > > When the CONFIG_ASYMMETRIC_AARCH32 option is enabled (EXPERT), the type > > > > > of the ARM64_HAS_32BIT_EL0 capability becomes WEAK_LOCAL_CPU_FEATURE. > > > > > The kernel will now return true for system_supports_32bit_el0() and > > > > > checks 32-bit tasks are affined to AArch32 capable CPUs only in > > > > > do_notify_resume(). If the affinity contains a non-capable AArch32 CPU, > > > > > the tasks will get SIGKILLed. If the last CPU supporting 32-bit is > > > > > offlined, the kernel will SIGKILL any scheduled 32-bit tasks (the > > > > > alternative is to prevent offlining through a new .cpu_disable feature > > > > > entry). > > > > > > > > > > In addition to the relaxation of the ARM64_HAS_32BIT_EL0 capability, > > > > > this patch factors out the 32-bit cpuinfo and features setting into > > > > > separate functions: __cpuinfo_store_cpu_32bit(), > > > > > init_cpu_32bit_features(). The cpuinfo of the booting CPU > > > > > (boot_cpu_data) is now updated on the first 32-bit capable CPU even if > > > > > it is a secondary one. The ID_AA64PFR0_EL0_64BIT_ONLY feature is relaxed > > > > > to FTR_NONSTRICT and FTR_HIGHER_SAFE when the asymmetric AArch32 support > > > > > is enabled. The compat_elf_hwcaps are only verified for the > > > > > AArch32-capable CPUs to still allow hotplugging AArch64-only CPUs. > > > > > > > > > > Make sure that KVM never sees the asymmetric 32bit system. Guest can > > > > > still ignore ID registers and force run 32bit at EL0. > > > > > > > > > > Co-developed-by: Qais Yousef <qais.yousef@arm.com> > > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > > > > > > [...] > > > > > > > > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > > > > > index 5e784e16ee89..312974ab2c85 100644 > > > > > --- a/arch/arm64/include/asm/thread_info.h > > > > > +++ b/arch/arm64/include/asm/thread_info.h > > > > > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > > > > > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > > > > > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > > > > > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > > > > > +#define TIF_CHECK_32BIT_AFFINITY 6 /* Check thread affinity for asymmetric AArch32 */ > > > > > > > > I've looked through the patch and I still can't figure out why this extra > > > > flag is needed. We know if a CPU supports 32-bit EL0, and we know whether > > > > or not a task is 32-bit. So why the extra flag? Is it just a hangover from > > > > the old series? > > > > > > It did evolve a bit organically. > > > > > > AFAICS it helps as an optimization to avoid the checks unnecessarily. If it's > > > not expensive to do the checks in the loop in do_notify_resume() we can omit > > > it. We will still protect it with system_supports_asym_32bit_el0() so the check > > > is done on these systems only. > > > > Ah, I think I remember now. We didn't want ret_to_user (entry.S) to > > always go the work_pending path if there was no context switch for a > > 32-bit task. With the AArch32 check in do_notify_resume(), it would mean > > we add _TIF_32BIT to the _TIF_WORK_MASK. > > > > However, we could add an asm alternative if AArch32 asym is detected to > > always route TIF_32BIT tasks to work_pending. > > Or could we just use TIF_NOTIFY_RESUME, like we for for rseq()? Maybe but it needs some analysis to make sure we don't lose this flag. It is cleared in some places. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC PATCH v2 3/4] arm64: export emulate_sys_reg() 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 1/4] arm64: kvm: Handle " Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations Qais Yousef @ 2020-10-21 10:46 ` Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs Qais Yousef 2020-10-21 11:26 ` [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Greg Kroah-Hartman 4 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 10:46 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel) Cc: Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch, Qais Yousef And get_arm64_ftr_reg() to allow exposing sanitized version of id_aa64fpr0 register to user space. To avoid a clash, rename emulate_sys_reg() in kvm code to be prefixed with 'kvm_'. Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- arch/arm64/include/asm/cpufeature.h | 3 +++ arch/arm64/kernel/cpufeature.c | 4 ++-- arch/arm64/kvm/sys_regs.c | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 338637907e6a..2b87f17b2bd4 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -80,6 +80,8 @@ struct arm64_ftr_reg { extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; +struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id); + extern unsigned int __read_mostly sysctl_enable_asym_32bit; /* @@ -579,6 +581,7 @@ void __init setup_cpu_features(void); void check_local_cpu_capabilities(void); u64 read_sanitised_ftr_reg(u32 id); +int emulate_sys_reg(u32 id, u64 *valp); static inline bool cpu_supports_mixed_endian_el0(void) { diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 1bbdfa9e911d..6f795c8221f4 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -648,7 +648,7 @@ static struct arm64_ftr_reg *get_arm64_ftr_reg_nowarn(u32 sys_id) * returns - Upon success, matching ftr_reg entry for id. * - NULL on failure but with an WARN_ON(). */ -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id) +struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id) { struct arm64_ftr_reg *reg; @@ -2774,7 +2774,7 @@ static inline int emulate_id_reg(u32 id, u64 *valp) return 0; } -static int emulate_sys_reg(u32 id, u64 *valp) +int emulate_sys_reg(u32 id, u64 *valp) { struct arm64_ftr_reg *regp; diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c9fb172f9b01..aad0ef7489db 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2353,7 +2353,7 @@ static bool is_imp_def_sys_reg(struct sys_reg_params *params) return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011; } -static int emulate_sys_reg(struct kvm_vcpu *vcpu, +static int kvm_emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) { const struct sys_reg_desc *r; @@ -2412,7 +2412,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu) params.regval = vcpu_get_reg(vcpu, Rt); params.is_write = !(esr & 1); - ret = emulate_sys_reg(vcpu, ¶ms); + ret = kvm_emulate_sys_reg(vcpu, ¶ms); if (!params.is_write) vcpu_set_reg(vcpu, Rt, params.regval); -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef ` (2 preceding siblings ...) 2020-10-21 10:46 ` [RFC PATCH v2 3/4] arm64: export emulate_sys_reg() Qais Yousef @ 2020-10-21 10:46 ` Qais Yousef 2020-10-21 11:09 ` Marc Zyngier 2020-10-21 11:28 ` Greg Kroah-Hartman 2020-10-21 11:26 ` [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Greg Kroah-Hartman 4 siblings, 2 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 10:46 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel) Cc: Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch, Qais Yousef So that userspace can detect if the cpu has aarch32 support at EL0. CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect what it does. And fixed to accept both u64 and u32 without causing the printf to print out a warning about mismatched type. This was caught while testing to check the new CPUREGS_USER_ATTR_RO(). The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based on a @cond to user space. The exported fields match the definition in arm64_ftr_reg so that the content of a register exported via MRS and sysfs are kept cohesive. The @cond in our case is that the system is asymmetric aarch32 and the controlling sysctl.enable_asym_32bit is enabled. Update Documentation/arm64/cpu-feature-registers.rst to reflect the newly visible EL0 field in ID_AA64FPR0_EL1. Note that the MRS interface will still return the sanitized content _only_. Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- Example output. I was surprised that the 2nd field (bits[7:4]) is printed out although it's set as FTR_HIDDEN. # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 0x0000000000000011 0x0000000000000011 0x0000000000000011 0x0000000000000011 0x0000000000000011 0x0000000000000011 # echo 1 > /proc/sys/kernel/enable_asym_32bit # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 0x0000000000000011 0x0000000000000011 0x0000000000000012 0x0000000000000012 0x0000000000000011 0x0000000000000011 Documentation/arm64/cpu-feature-registers.rst | 2 +- arch/arm64/kernel/cpufeature.c | 2 +- arch/arm64/kernel/cpuinfo.c | 58 +++++++++++++++++-- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst index f28853f80089..bfcbda6d6f35 100644 --- a/Documentation/arm64/cpu-feature-registers.rst +++ b/Documentation/arm64/cpu-feature-registers.rst @@ -166,7 +166,7 @@ infrastructure: +------------------------------+---------+---------+ | EL1 | [7-4] | n | +------------------------------+---------+---------+ - | EL0 | [3-0] | n | + | EL0 | [3-0] | y | +------------------------------+---------+---------+ diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 6f795c8221f4..0f7307c8ad80 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -221,7 +221,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), + ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), ARM64_FTR_END, }; diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index 93c55986ca7f..632b9d5b5230 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -231,25 +231,71 @@ static struct kobj_type cpuregs_kobj_type = { * future expansion without an ABI break. */ #define kobj_to_cpuinfo(kobj) container_of(kobj, struct cpuinfo_arm64, kobj) -#define CPUREGS_ATTR_RO(_name, _field) \ +#define CPUREGS_RAW_ATTR_RO(_name, _field) \ static ssize_t _name##_show(struct kobject *kobj, \ struct kobj_attribute *attr, char *buf) \ { \ struct cpuinfo_arm64 *info = kobj_to_cpuinfo(kobj); \ \ - if (info->reg_midr) \ - return sprintf(buf, "0x%016x\n", info->reg_##_field); \ - else \ + if (info->reg_midr) { \ + u64 val = info->reg_##_field; \ + return sprintf(buf, "0x%016llx\n", val); \ + } else { \ return 0; \ + } \ } \ static struct kobj_attribute cpuregs_attr_##_name = __ATTR_RO(_name) -CPUREGS_ATTR_RO(midr_el1, midr); -CPUREGS_ATTR_RO(revidr_el1, revidr); +/* + * Expose the Sanitized or RAW user space visible fields of a sys_register + * based @cond. + * + * if (@cond) + * expose raw register & user_mask + * else + * expose sanitized register & user_mask + * + * user_mask is based on arm64_ftr_reg definition. + */ +#define CPUREGS_USER_ATTR_RO(_name, _raw_field, _san_id, cond) \ + static ssize_t _name##_show(struct kobject *kobj, \ + struct kobj_attribute *attr, char *buf) \ + { \ + u64 val = 0; \ + \ + if (cond) { \ + struct arm64_ftr_reg *reg = get_arm64_ftr_reg(_san_id); \ + struct cpuinfo_arm64 *info = kobj_to_cpuinfo(kobj); \ + \ + if (!reg) \ + return 0; \ + \ + if (info->reg_midr) { \ + val = info->reg_##_raw_field & reg->user_mask; \ + val |= reg->user_val; \ + return sprintf(buf, "0x%016llx\n", val); \ + } else { \ + return 0; \ + } \ + } else { \ + val = 0; \ + if (!emulate_sys_reg(_san_id, &val)) { \ + return sprintf(buf, "0x%016llx\n", val); \ + } else { \ + return 0; \ + } \ + } \ + } \ + static struct kobj_attribute cpuregs_attr_##_name = __ATTR_RO(_name) + +CPUREGS_RAW_ATTR_RO(midr_el1, midr); +CPUREGS_RAW_ATTR_RO(revidr_el1, revidr); +CPUREGS_USER_ATTR_RO(id_aa64pfr0, id_aa64pfr0, SYS_ID_AA64PFR0_EL1, system_supports_asym_32bit_el0()); static struct attribute *cpuregs_id_attrs[] = { &cpuregs_attr_midr_el1.attr, &cpuregs_attr_revidr_el1.attr, + &cpuregs_attr_id_aa64pfr0.attr, NULL }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 10:46 ` [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs Qais Yousef @ 2020-10-21 11:09 ` Marc Zyngier 2020-10-21 11:25 ` Greg Kroah-Hartman 2020-10-21 12:15 ` Catalin Marinas 2020-10-21 11:28 ` Greg Kroah-Hartman 1 sibling, 2 replies; 57+ messages in thread From: Marc Zyngier @ 2020-10-21 11:09 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 2020-10-21 11:46, Qais Yousef wrote: > So that userspace can detect if the cpu has aarch32 support at EL0. > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better > reflect > what it does. And fixed to accept both u64 and u32 without causing the > printf to print out a warning about mismatched type. This was caught > while testing to check the new CPUREGS_USER_ATTR_RO(). > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > on a @cond to user space. The exported fields match the definition in > arm64_ftr_reg so that the content of a register exported via MRS and > sysfs are kept cohesive. > > The @cond in our case is that the system is asymmetric aarch32 and the > controlling sysctl.enable_asym_32bit is enabled. > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > newly visible EL0 field in ID_AA64FPR0_EL1. > > Note that the MRS interface will still return the sanitized content > _only_. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > > Example output. I was surprised that the 2nd field (bits[7:4]) is > printed out > although it's set as FTR_HIDDEN. > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000012 > 0x0000000000000012 > 0x0000000000000011 > 0x0000000000000011 This looks like a terrible userspace interface. It exposes unrelated features, and doesn't expose the single useful information that the kernel has: the cpumask describing the CPUs supporting AArch32 at EL0. Why not expose this synthetic piece of information which requires very little effort from userspace and doesn't spit out unrelated stuff? Not to mention the discrepancy with what userspace gets while reading the same register via the MRS emulation. Granted, the cpumask doesn't fit the cpu*/regs/identification hierarchy, but I don't think this fits either. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:09 ` Marc Zyngier @ 2020-10-21 11:25 ` Greg Kroah-Hartman 2020-10-21 11:46 ` Marc Zyngier 2020-10-21 12:15 ` Catalin Marinas 1 sibling, 1 reply; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-21 11:25 UTC (permalink / raw) To: Marc Zyngier Cc: Qais Yousef, Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > On 2020-10-21 11:46, Qais Yousef wrote: > > So that userspace can detect if the cpu has aarch32 support at EL0. > > > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect > > what it does. And fixed to accept both u64 and u32 without causing the > > printf to print out a warning about mismatched type. This was caught > > while testing to check the new CPUREGS_USER_ATTR_RO(). > > > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > > on a @cond to user space. The exported fields match the definition in > > arm64_ftr_reg so that the content of a register exported via MRS and > > sysfs are kept cohesive. > > > > The @cond in our case is that the system is asymmetric aarch32 and the > > controlling sysctl.enable_asym_32bit is enabled. > > > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > > newly visible EL0 field in ID_AA64FPR0_EL1. > > > > Note that the MRS interface will still return the sanitized content > > _only_. > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > --- > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > printed out > > although it's set as FTR_HIDDEN. > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000012 > > 0x0000000000000012 > > 0x0000000000000011 > > 0x0000000000000011 > > This looks like a terrible userspace interface. It's also not allowed, sorry. sysfs is "one value per file", which is NOT what is happening at all. This would be easy to see if there was a Documentation/ABI/ update, which is also required, was that here? thanks, greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:25 ` Greg Kroah-Hartman @ 2020-10-21 11:46 ` Marc Zyngier 2020-10-21 12:11 ` Greg Kroah-Hartman 2020-10-21 13:18 ` Qais Yousef 0 siblings, 2 replies; 57+ messages in thread From: Marc Zyngier @ 2020-10-21 11:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Qais Yousef, Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 2020-10-21 12:25, Greg Kroah-Hartman wrote: > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: >> On 2020-10-21 11:46, Qais Yousef wrote: >> > So that userspace can detect if the cpu has aarch32 support at EL0. >> > >> > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect >> > what it does. And fixed to accept both u64 and u32 without causing the >> > printf to print out a warning about mismatched type. This was caught >> > while testing to check the new CPUREGS_USER_ATTR_RO(). >> > >> > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based >> > on a @cond to user space. The exported fields match the definition in >> > arm64_ftr_reg so that the content of a register exported via MRS and >> > sysfs are kept cohesive. >> > >> > The @cond in our case is that the system is asymmetric aarch32 and the >> > controlling sysctl.enable_asym_32bit is enabled. >> > >> > Update Documentation/arm64/cpu-feature-registers.rst to reflect the >> > newly visible EL0 field in ID_AA64FPR0_EL1. >> > >> > Note that the MRS interface will still return the sanitized content >> > _only_. >> > >> > Signed-off-by: Qais Yousef <qais.yousef@arm.com> >> > --- >> > >> > Example output. I was surprised that the 2nd field (bits[7:4]) is >> > printed out >> > although it's set as FTR_HIDDEN. >> > >> > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > >> > # echo 1 > /proc/sys/kernel/enable_asym_32bit >> > >> > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 >> > 0x0000000000000011 >> > 0x0000000000000011 >> > 0x0000000000000012 >> > 0x0000000000000012 >> > 0x0000000000000011 >> > 0x0000000000000011 >> >> This looks like a terrible userspace interface. > > It's also not allowed, sorry. sysfs is "one value per file", which is > NOT what is happening at all. I *think* Qais got that part right, though it is hard to tell without knowing how many CPUs this system has (cpu/cpu* is ambiguous). M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:46 ` Marc Zyngier @ 2020-10-21 12:11 ` Greg Kroah-Hartman 2020-10-21 13:18 ` Qais Yousef 1 sibling, 0 replies; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-21 12:11 UTC (permalink / raw) To: Marc Zyngier Cc: Qais Yousef, Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 12:46:46PM +0100, Marc Zyngier wrote: > On 2020-10-21 12:25, Greg Kroah-Hartman wrote: > > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > > On 2020-10-21 11:46, Qais Yousef wrote: > > > > So that userspace can detect if the cpu has aarch32 support at EL0. > > > > > > > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect > > > > what it does. And fixed to accept both u64 and u32 without causing the > > > > printf to print out a warning about mismatched type. This was caught > > > > while testing to check the new CPUREGS_USER_ATTR_RO(). > > > > > > > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > > > > on a @cond to user space. The exported fields match the definition in > > > > arm64_ftr_reg so that the content of a register exported via MRS and > > > > sysfs are kept cohesive. > > > > > > > > The @cond in our case is that the system is asymmetric aarch32 and the > > > > controlling sysctl.enable_asym_32bit is enabled. > > > > > > > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > > > > newly visible EL0 field in ID_AA64FPR0_EL1. > > > > > > > > Note that the MRS interface will still return the sanitized content > > > > _only_. > > > > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > > --- > > > > > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > > printed out > > > > although it's set as FTR_HIDDEN. > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000012 > > > > 0x0000000000000012 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > This looks like a terrible userspace interface. > > > > It's also not allowed, sorry. sysfs is "one value per file", which is > > NOT what is happening at all. > > I *think* Qais got that part right, though it is hard to tell without > knowing how many CPUs this system has (cpu/cpu* is ambiguous). Ah, missed the '*' in the middle of that path, my fault. But without documentation, it's impossible to know... thanks, greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:46 ` Marc Zyngier 2020-10-21 12:11 ` Greg Kroah-Hartman @ 2020-10-21 13:18 ` Qais Yousef 1 sibling, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:18 UTC (permalink / raw) To: Marc Zyngier Cc: Greg Kroah-Hartman, Catalin Marinas, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 12:46, Marc Zyngier wrote: > On 2020-10-21 12:25, Greg Kroah-Hartman wrote: > > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > > On 2020-10-21 11:46, Qais Yousef wrote: > > > > So that userspace can detect if the cpu has aarch32 support at EL0. > > > > > > > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect > > > > what it does. And fixed to accept both u64 and u32 without causing the > > > > printf to print out a warning about mismatched type. This was caught > > > > while testing to check the new CPUREGS_USER_ATTR_RO(). > > > > > > > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > > > > on a @cond to user space. The exported fields match the definition in > > > > arm64_ftr_reg so that the content of a register exported via MRS and > > > > sysfs are kept cohesive. > > > > > > > > The @cond in our case is that the system is asymmetric aarch32 and the > > > > controlling sysctl.enable_asym_32bit is enabled. > > > > > > > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > > > > newly visible EL0 field in ID_AA64FPR0_EL1. > > > > > > > > Note that the MRS interface will still return the sanitized content > > > > _only_. > > > > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > > > --- > > > > > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > > printed out > > > > although it's set as FTR_HIDDEN. > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000012 > > > > 0x0000000000000012 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > This looks like a terrible userspace interface. > > > > It's also not allowed, sorry. sysfs is "one value per file", which is > > NOT what is happening at all. > > I *think* Qais got that part right, though it is hard to tell without > knowing how many CPUs this system has (cpu/cpu* is ambiguous). Correct. This interface already exists for other registers and reads a single value for each CPU. I just added the ability to read a new register. Though it has a slightly different behavior: reads Sanitized vs RAW info. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:09 ` Marc Zyngier 2020-10-21 11:25 ` Greg Kroah-Hartman @ 2020-10-21 12:15 ` Catalin Marinas 2020-10-21 13:20 ` Qais Yousef ` (2 more replies) 1 sibling, 3 replies; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 12:15 UTC (permalink / raw) To: Marc Zyngier Cc: Qais Yousef, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > On 2020-10-21 11:46, Qais Yousef wrote: > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > printed out > > although it's set as FTR_HIDDEN. > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000012 > > 0x0000000000000012 > > 0x0000000000000011 > > 0x0000000000000011 > > This looks like a terrible userspace interface. It exposes unrelated > features, Not sure why the EL1 field ended up in here, that's not relevant to the user. > and doesn't expose the single useful information that the kernel has: > the cpumask describing the CPUs supporting AArch32 at EL0. Why not expose > this synthetic piece of information which requires very little effort from > userspace and doesn't spit out unrelated stuff? I thought the whole idea is to try and avoid the "very little effort" part ;). > Not to mention the discrepancy with what userspace gets while reading > the same register via the MRS emulation. > > Granted, the cpumask doesn't fit the cpu*/regs/identification hierarchy, > but I don't think this fits either. We already expose MIDR and REVIDR via the current sysfs interface. We can expand it to include _all_ the other ID_* regs currently available to user via the MRS emulation and we won't have to debate what a new interface would look like. The MRS emulation and the sysfs info should probably match, though that means we need to expose the ID_AA64PFR0_EL1.EL0 field which we currently don't. I do agree that an AArch32 cpumask is an easier option both from the kernel implementation perspective and from the application usability one, though not as easy as automatic task placement by the scheduler (my first preference, followed by the id_* regs and the aarch32 mask, though not a strong preference for any). -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 12:15 ` Catalin Marinas @ 2020-10-21 13:20 ` Qais Yousef 2020-10-21 13:33 ` Morten Rasmussen 2020-10-21 14:41 ` Will Deacon 2 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:20 UTC (permalink / raw) To: Catalin Marinas Cc: Marc Zyngier, Will Deacon, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 13:15, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > On 2020-10-21 11:46, Qais Yousef wrote: > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > printed out > > > although it's set as FTR_HIDDEN. ^^^^^^^^^ > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000012 > > > 0x0000000000000012 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > > This looks like a terrible userspace interface. It exposes unrelated > > features, > > Not sure why the EL1 field ended up in here, that's not relevant to the > user. I was surprised by this too. Not sure a bug at my end or a bug in the way arm64_ftr_reg stores reg->user_val and reg->user_mask. FWIW, I did highlight the problem above. It is something to fix indeed. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 12:15 ` Catalin Marinas 2020-10-21 13:20 ` Qais Yousef @ 2020-10-21 13:33 ` Morten Rasmussen 2020-10-21 14:09 ` Catalin Marinas 2020-10-21 14:31 ` Qais Yousef 2020-10-21 14:41 ` Will Deacon 2 siblings, 2 replies; 57+ messages in thread From: Morten Rasmussen @ 2020-10-21 13:33 UTC (permalink / raw) To: Catalin Marinas Cc: Marc Zyngier, linux-arch, Will Deacon, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > one, though not as easy as automatic task placement by the scheduler (my > first preference, followed by the id_* regs and the aarch32 mask, though > not a strong preference for any). Automatic task placement by the scheduler would mean giving up the requirement that the user-space affinity mask must always be honoured. Is that on the table? Killing aarch32 tasks with an empty intersection between the user-space mask and aarch32_mask is not really "automatic" and would require the aarch32 capability to be exposed anyway. Morten ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 13:33 ` Morten Rasmussen @ 2020-10-21 14:09 ` Catalin Marinas 2020-10-21 14:41 ` Morten Rasmussen 2020-10-21 14:45 ` Will Deacon 2020-10-21 14:31 ` Qais Yousef 1 sibling, 2 replies; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 14:09 UTC (permalink / raw) To: Morten Rasmussen Cc: Marc Zyngier, linux-arch, Will Deacon, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 03:33:29PM +0200, Morten Rasmussen wrote: > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > one, though not as easy as automatic task placement by the scheduler (my > > first preference, followed by the id_* regs and the aarch32 mask, though > > not a strong preference for any). > > Automatic task placement by the scheduler would mean giving up the > requirement that the user-space affinity mask must always be honoured. > Is that on the table? I think Peter rejected it but I still find it a nicer interface from a dumb application perspective. It may interact badly with cpusets though (at least on Android). > Killing aarch32 tasks with an empty intersection between the > user-space mask and aarch32_mask is not really "automatic" and would > require the aarch32 capability to be exposed anyway. I agree, especially if overriding the user mask is not desirable. But if one doesn't play around with cpusets, 32-bit apps would run "fine" with the scheduler transparently placing them on the correct CPU. Anyway, if the task placement is entirely off the table, the next thing is asking applications to set their own mask and kill them if they do the wrong thing. Here I see two possibilities for killing an app: 1. When it ends up scheduled on a non-AArch32-capable CPU 2. If the user cpumask (bar the offline CPUs) is not a subset of the aarch32_mask Option 1 is simpler but 2 would be slightly more consistent. There's also the question on whether the kernel should allow an ELF32 to be loaded (and potentially killed subsequently) if the user mask is not correct on execve(). -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:09 ` Catalin Marinas @ 2020-10-21 14:41 ` Morten Rasmussen 2020-10-21 14:45 ` Will Deacon 1 sibling, 0 replies; 57+ messages in thread From: Morten Rasmussen @ 2020-10-21 14:41 UTC (permalink / raw) To: Catalin Marinas Cc: linux-arch, Peter Zijlstra (Intel), Marc Zyngier, Linus Torvalds, James Morse, Greg Kroah-Hartman, Will Deacon, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 03:33:29PM +0200, Morten Rasmussen wrote: > > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > > one, though not as easy as automatic task placement by the scheduler (my > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > not a strong preference for any). > > > > Automatic task placement by the scheduler would mean giving up the > > requirement that the user-space affinity mask must always be honoured. > > Is that on the table? > > I think Peter rejected it but I still find it a nicer interface from a > dumb application perspective. It may interact badly with cpusets though > (at least on Android). Agree that it would be nice for supporting legacy applications. Due to the cpuset interaction I think there is fair chance that user-space would want to know the aarch32 cpumask anyway though. > > > Killing aarch32 tasks with an empty intersection between the > > user-space mask and aarch32_mask is not really "automatic" and would > > require the aarch32 capability to be exposed anyway. > > I agree, especially if overriding the user mask is not desirable. But if > one doesn't play around with cpusets, 32-bit apps would run "fine" with > the scheduler transparently placing them on the correct CPU. Ruling out user-space setting affinity is another way of solving the problem ;-) > Anyway, if the task placement is entirely off the table, the next thing > is asking applications to set their own mask and kill them if they do > the wrong thing. Here I see two possibilities for killing an app: > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > aarch32_mask > > Option 1 is simpler but 2 would be slightly more consistent. I don't have strong preference. More consistent killing is probably nice for debugging purposes. If we go with 2, we would go round and kill all tasks in cpuset (both running and sleeping) if the cpuset mask was changed to not include aarch32 CPUs? > There's also the question on whether the kernel should allow an ELF32 to > be loaded (and potentially killed subsequently) if the user mask is not > correct on execve(). I wonder how many apps that would handle execve() failing? If we allow killing, the simplest solution if there is any doubt seems to be just to kill the task :-) Morten ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:09 ` Catalin Marinas 2020-10-21 14:41 ` Morten Rasmussen @ 2020-10-21 14:45 ` Will Deacon 2020-10-21 15:10 ` Catalin Marinas 1 sibling, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 14:45 UTC (permalink / raw) To: Catalin Marinas Cc: Morten Rasmussen, Marc Zyngier, linux-arch, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 03:33:29PM +0200, Morten Rasmussen wrote: > > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > > one, though not as easy as automatic task placement by the scheduler (my > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > not a strong preference for any). > > > > Automatic task placement by the scheduler would mean giving up the > > requirement that the user-space affinity mask must always be honoured. > > Is that on the table? > > I think Peter rejected it but I still find it a nicer interface from a > dumb application perspective. It may interact badly with cpusets though > (at least on Android). > > > Killing aarch32 tasks with an empty intersection between the > > user-space mask and aarch32_mask is not really "automatic" and would > > require the aarch32 capability to be exposed anyway. > > I agree, especially if overriding the user mask is not desirable. But if > one doesn't play around with cpusets, 32-bit apps would run "fine" with > the scheduler transparently placing them on the correct CPU. > > Anyway, if the task placement is entirely off the table, the next thing > is asking applications to set their own mask and kill them if they do > the wrong thing. Here I see two possibilities for killing an app: > > 1. When it ends up scheduled on a non-AArch32-capable CPU That sounds fine to me. If we could do the exception return and take a SIGILL, that's what we'd do, but we can't so we have to catch it before. > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > aarch32_mask > > Option 1 is simpler but 2 would be slightly more consistent. I disagree -- if we did this for something like fpsimd, then the consistent behaviour would be to SIGILL on the cores without the instructions. > There's also the question on whether the kernel should allow an ELF32 to > be loaded (and potentially killed subsequently) if the user mask is not > correct on execve(). I don't see the point in distinguishing between "you did execve() on a core without 32-bit" and "you did execve() on a core with 32-bit and then migrated to a core without 32-bit". Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:45 ` Will Deacon @ 2020-10-21 15:10 ` Catalin Marinas 2020-10-21 15:37 ` Will Deacon 0 siblings, 1 reply; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 15:10 UTC (permalink / raw) To: Will Deacon Cc: Morten Rasmussen, Marc Zyngier, linux-arch, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 03:45:43PM +0100, Will Deacon wrote: > On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > > Anyway, if the task placement is entirely off the table, the next thing > > is asking applications to set their own mask and kill them if they do > > the wrong thing. Here I see two possibilities for killing an app: > > > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > That sounds fine to me. If we could do the exception return and take a > SIGILL, that's what we'd do, but we can't so we have to catch it before. Indeed, the illegal ERET doesn't work for this scenario. > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > > aarch32_mask > > > > Option 1 is simpler but 2 would be slightly more consistent. > > I disagree -- if we did this for something like fpsimd, then the consistent > behaviour would be to SIGILL on the cores without the instructions. For fpsimd it makes sense since the main ISA is still available and the application may be able to do something with the signal. But here we can't do much since the entire AArch32 mode is not supported. That's why we went for SIGKILL instead of SIGILL but thinking of it, after execve() the signals are reset to SIG_DFL so SIGILL cannot be ignored. I think it depends on whether you look at this fault as a part of ISA not being available or as the overall application not compatible with the system it is running on. If the latter, option 2 above makes more sense. > > There's also the question on whether the kernel should allow an ELF32 to > > be loaded (and potentially killed subsequently) if the user mask is not > > correct on execve(). > > I don't see the point in distinguishing between "you did execve() on a core > without 32-bit" and "you did execve() on a core with 32-bit and then > migrated to a core without 32-bit". In the context of option 2 above, its more about whether execve() returns -ENOEXEC or the process gets a SIGKILL immediately. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 15:10 ` Catalin Marinas @ 2020-10-21 15:37 ` Will Deacon 2020-10-21 16:18 ` Catalin Marinas 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 15:37 UTC (permalink / raw) To: Catalin Marinas Cc: Morten Rasmussen, Marc Zyngier, linux-arch, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 04:10:06PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 03:45:43PM +0100, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > > > Anyway, if the task placement is entirely off the table, the next thing > > > is asking applications to set their own mask and kill them if they do > > > the wrong thing. Here I see two possibilities for killing an app: > > > > > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > > > That sounds fine to me. If we could do the exception return and take a > > SIGILL, that's what we'd do, but we can't so we have to catch it before. > > Indeed, the illegal ERET doesn't work for this scenario. > > > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > > > aarch32_mask > > > > > > Option 1 is simpler but 2 would be slightly more consistent. > > > > I disagree -- if we did this for something like fpsimd, then the consistent > > behaviour would be to SIGILL on the cores without the instructions. > > For fpsimd it makes sense since the main ISA is still available and the > application may be able to do something with the signal. But here we > can't do much since the entire AArch32 mode is not supported. That's why > we went for SIGKILL instead of SIGILL but thinking of it, after execve() > the signals are reset to SIG_DFL so SIGILL cannot be ignored. > > I think it depends on whether you look at this fault as a part of ISA > not being available or as the overall application not compatible with > the system it is running on. If the latter, option 2 above makes more > sense. Hmm, I'm not sure I see the distinction in practice: you still have a binary application that cannot run on all CPUs in the system. Who cares if some of the instructions work? > > > There's also the question on whether the kernel should allow an ELF32 to > > > be loaded (and potentially killed subsequently) if the user mask is not > > > correct on execve(). > > > > I don't see the point in distinguishing between "you did execve() on a core > > without 32-bit" and "you did execve() on a core with 32-bit and then > > migrated to a core without 32-bit". > > In the context of option 2 above, its more about whether execve() > returns -ENOEXEC or the process gets a SIGKILL immediately. I just don't see what we gain by returning -ENOEXEC except for extra code and behaviour in the ABI (and if you wanted consistentcy you'd also need to fail attempts to widen the affinity mask to include 64-bit-only cores from a 32-bit task). In other words, I don't think the kernel needs to hold userspace's hand for an opt-in feature that requires userspace to handle scheduling for optimal power/performance _anyway_. Allowing the affinity to be set arbitrarily and then killing the task if it ends up trying to run on the wrong CPU is both simple and sufficient. Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 15:37 ` Will Deacon @ 2020-10-21 16:18 ` Catalin Marinas 2020-10-21 17:19 ` Will Deacon 0 siblings, 1 reply; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 16:18 UTC (permalink / raw) To: Will Deacon Cc: Morten Rasmussen, Marc Zyngier, linux-arch, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel On Wed, Oct 21, 2020 at 04:37:38PM +0100, Will Deacon wrote: > On Wed, Oct 21, 2020 at 04:10:06PM +0100, Catalin Marinas wrote: > > On Wed, Oct 21, 2020 at 03:45:43PM +0100, Will Deacon wrote: > > > On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > > > > Anyway, if the task placement is entirely off the table, the next thing > > > > is asking applications to set their own mask and kill them if they do > > > > the wrong thing. Here I see two possibilities for killing an app: > > > > > > > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > > > > > That sounds fine to me. If we could do the exception return and take a > > > SIGILL, that's what we'd do, but we can't so we have to catch it before. > > > > Indeed, the illegal ERET doesn't work for this scenario. > > > > > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > > > > aarch32_mask > > > > > > > > Option 1 is simpler but 2 would be slightly more consistent. > > > > > > I disagree -- if we did this for something like fpsimd, then the consistent > > > behaviour would be to SIGILL on the cores without the instructions. > > > > For fpsimd it makes sense since the main ISA is still available and the > > application may be able to do something with the signal. But here we > > can't do much since the entire AArch32 mode is not supported. That's why > > we went for SIGKILL instead of SIGILL but thinking of it, after execve() > > the signals are reset to SIG_DFL so SIGILL cannot be ignored. > > > > I think it depends on whether you look at this fault as a part of ISA > > not being available or as the overall application not compatible with > > the system it is running on. If the latter, option 2 above makes more > > sense. > > Hmm, I'm not sure I see the distinction in practice: you still have a binary > application that cannot run on all CPUs in the system. Who cares if some of > the instructions work? The failure would be more predictable rather than the app running for a while and randomly getting SIGKILL. If it only fails on execve or sched_setaffinity, it may be easier to track down (well, there's the CPU hotplug as well that can change the cpumask intersection outside the user process control). > > > > There's also the question on whether the kernel should allow an ELF32 to > > > > be loaded (and potentially killed subsequently) if the user mask is not > > > > correct on execve(). > > > > > > I don't see the point in distinguishing between "you did execve() on a core > > > without 32-bit" and "you did execve() on a core with 32-bit and then > > > migrated to a core without 32-bit". > > > > In the context of option 2 above, its more about whether execve() > > returns -ENOEXEC or the process gets a SIGKILL immediately. > > I just don't see what we gain by returning -ENOEXEC except for extra code > and behaviour in the ABI (and if you wanted consistentcy you'd also need > to fail attempts to widen the affinity mask to include 64-bit-only cores > from a 32-bit task). The -ENOEXEC is more in line with the current behaviour not allowing ELF32 on systems that are not fully symmetric. So basically you'd have a global opt-in as sysctl and a per-application opt-in via the affinity mask. I do agree that it complicates the kernel implementation. > In other words, I don't think the kernel needs to hold userspace's hand > for an opt-in feature that requires userspace to handle scheduling for > optimal power/performance _anyway_. Allowing the affinity to be set > arbitrarily and then killing the task if it ends up trying to run on the > wrong CPU is both simple and sufficient. Fine by me if you want to keep things simple, less code to maintain. However, it would be good to know if the Android kernel/user guys are happy with this approach. If the Android kernel ends up carrying additional patches for task placement, I'd question why we need to merge this (partial) series at all. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 16:18 ` Catalin Marinas @ 2020-10-21 17:19 ` Will Deacon 2020-10-22 9:55 ` Morten Rasmussen 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 17:19 UTC (permalink / raw) To: Catalin Marinas Cc: Morten Rasmussen, Marc Zyngier, linux-arch, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, Qais Yousef, linux-arm-kernel, surenb, balejs On Wed, Oct 21, 2020 at 05:18:37PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 04:37:38PM +0100, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 04:10:06PM +0100, Catalin Marinas wrote: > > > On Wed, Oct 21, 2020 at 03:45:43PM +0100, Will Deacon wrote: > > > > On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > > > > > Anyway, if the task placement is entirely off the table, the next thing > > > > > is asking applications to set their own mask and kill them if they do > > > > > the wrong thing. Here I see two possibilities for killing an app: > > > > > > > > > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > > > > > > > That sounds fine to me. If we could do the exception return and take a > > > > SIGILL, that's what we'd do, but we can't so we have to catch it before. > > > > > > Indeed, the illegal ERET doesn't work for this scenario. > > > > > > > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > > > > > aarch32_mask > > > > > > > > > > Option 1 is simpler but 2 would be slightly more consistent. > > > > > > > > I disagree -- if we did this for something like fpsimd, then the consistent > > > > behaviour would be to SIGILL on the cores without the instructions. > > > > > > For fpsimd it makes sense since the main ISA is still available and the > > > application may be able to do something with the signal. But here we > > > can't do much since the entire AArch32 mode is not supported. That's why > > > we went for SIGKILL instead of SIGILL but thinking of it, after execve() > > > the signals are reset to SIG_DFL so SIGILL cannot be ignored. > > > > > > I think it depends on whether you look at this fault as a part of ISA > > > not being available or as the overall application not compatible with > > > the system it is running on. If the latter, option 2 above makes more > > > sense. > > > > Hmm, I'm not sure I see the distinction in practice: you still have a binary > > application that cannot run on all CPUs in the system. Who cares if some of > > the instructions work? > > The failure would be more predictable rather than the app running for a > while and randomly getting SIGKILL. If it only fails on execve or > sched_setaffinity, it may be easier to track down (well, there's the CPU > hotplug as well that can change the cpumask intersection outside the > user process control). But it's half-baked, because the moment the 32-bit task changes its affinity mask then you're back in the old situation. That's why I'm saying this doesn't add anything, because the rest of the series is designed entirely around delivering SIGKILL at the last minute rather than preventing us getting to that situation in the first place. The execve() case feels to me like we're considering doing something because we can, rather than because it's actually useful. > > > > > There's also the question on whether the kernel should allow an ELF32 to > > > > > be loaded (and potentially killed subsequently) if the user mask is not > > > > > correct on execve(). > > > > > > > > I don't see the point in distinguishing between "you did execve() on a core > > > > without 32-bit" and "you did execve() on a core with 32-bit and then > > > > migrated to a core without 32-bit". > > > > > > In the context of option 2 above, its more about whether execve() > > > returns -ENOEXEC or the process gets a SIGKILL immediately. > > > > I just don't see what we gain by returning -ENOEXEC except for extra code > > and behaviour in the ABI (and if you wanted consistentcy you'd also need > > to fail attempts to widen the affinity mask to include 64-bit-only cores > > from a 32-bit task). > > The -ENOEXEC is more in line with the current behaviour not allowing > ELF32 on systems that are not fully symmetric. So basically you'd have > a global opt-in as sysctl and a per-application opt-in via the affinity > mask. I think it's a bit strong calling that an opt-in, as the application could just happen to be using the right cpumask. > I do agree that it complicates the kernel implementation. > > > In other words, I don't think the kernel needs to hold userspace's hand > > for an opt-in feature that requires userspace to handle scheduling for > > optimal power/performance _anyway_. Allowing the affinity to be set > > arbitrarily and then killing the task if it ends up trying to run on the > > wrong CPU is both simple and sufficient. > > Fine by me if you want to keep things simple, less code to maintain. > > However, it would be good to know if the Android kernel/user guys are > happy with this approach. If the Android kernel ends up carrying > additional patches for task placement, I'd question why we need to merge > this (partial) series at all. Hmm, those folks aren't even on CC :( Adding Suren and Marco... Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 17:19 ` Will Deacon @ 2020-10-22 9:55 ` Morten Rasmussen 0 siblings, 0 replies; 57+ messages in thread From: Morten Rasmussen @ 2020-10-22 9:55 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, linux-arch, Peter Zijlstra (Intel), Marc Zyngier, Qais Yousef, surenb, James Morse, Greg Kroah-Hartman, Linus Torvalds, linux-arm-kernel, balejs On Wed, Oct 21, 2020 at 06:19:48PM +0100, Will Deacon wrote: > On Wed, Oct 21, 2020 at 05:18:37PM +0100, Catalin Marinas wrote: > > On Wed, Oct 21, 2020 at 04:37:38PM +0100, Will Deacon wrote: > > > On Wed, Oct 21, 2020 at 04:10:06PM +0100, Catalin Marinas wrote: > > > > On Wed, Oct 21, 2020 at 03:45:43PM +0100, Will Deacon wrote: > > > > > On Wed, Oct 21, 2020 at 03:09:46PM +0100, Catalin Marinas wrote: > > > > > > Anyway, if the task placement is entirely off the table, the next thing > > > > > > is asking applications to set their own mask and kill them if they do > > > > > > the wrong thing. Here I see two possibilities for killing an app: > > > > > > > > > > > > 1. When it ends up scheduled on a non-AArch32-capable CPU > > > > > > > > > > That sounds fine to me. If we could do the exception return and take a > > > > > SIGILL, that's what we'd do, but we can't so we have to catch it before. > > > > > > > > Indeed, the illegal ERET doesn't work for this scenario. > > > > > > > > > > 2. If the user cpumask (bar the offline CPUs) is not a subset of the > > > > > > aarch32_mask > > > > > > > > > > > > Option 1 is simpler but 2 would be slightly more consistent. > > > > > > > > > > I disagree -- if we did this for something like fpsimd, then the consistent > > > > > behaviour would be to SIGILL on the cores without the instructions. > > > > > > > > For fpsimd it makes sense since the main ISA is still available and the > > > > application may be able to do something with the signal. But here we > > > > can't do much since the entire AArch32 mode is not supported. That's why > > > > we went for SIGKILL instead of SIGILL but thinking of it, after execve() > > > > the signals are reset to SIG_DFL so SIGILL cannot be ignored. > > > > > > > > I think it depends on whether you look at this fault as a part of ISA > > > > not being available or as the overall application not compatible with > > > > the system it is running on. If the latter, option 2 above makes more > > > > sense. > > > > > > Hmm, I'm not sure I see the distinction in practice: you still have a binary > > > application that cannot run on all CPUs in the system. Who cares if some of > > > the instructions work? > > > > The failure would be more predictable rather than the app running for a > > while and randomly getting SIGKILL. If it only fails on execve or > > sched_setaffinity, it may be easier to track down (well, there's the CPU > > hotplug as well that can change the cpumask intersection outside the > > user process control). Migration between cpusets is another failure scenario where the app can get SIGKILL randomly. > But it's half-baked, because the moment the 32-bit task changes its affinity > mask then you're back in the old situation. That's why I'm saying this > doesn't add anything, because the rest of the series is designed entirely > around delivering SIGKILL at the last minute rather than preventing us > getting to that situation in the first place. The execve() case feels to me > like we're considering doing something because we can, rather than because > it's actually useful. Agree. ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 13:33 ` Morten Rasmussen 2020-10-21 14:09 ` Catalin Marinas @ 2020-10-21 14:31 ` Qais Yousef 2020-10-22 10:16 ` Morten Rasmussen 1 sibling, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 14:31 UTC (permalink / raw) To: Morten Rasmussen Cc: Catalin Marinas, Marc Zyngier, linux-arch, Will Deacon, Peter Zijlstra (Intel), Greg Kroah-Hartman, James Morse, Linus Torvalds, linux-arm-kernel On 10/21/20 15:33, Morten Rasmussen wrote: > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > one, though not as easy as automatic task placement by the scheduler (my > > first preference, followed by the id_* regs and the aarch32 mask, though > > not a strong preference for any). > > Automatic task placement by the scheduler would mean giving up the > requirement that the user-space affinity mask must always be honoured. > Is that on the table? > > Killing aarch32 tasks with an empty intersection between the user-space > mask and aarch32_mask is not really "automatic" and would require the > aarch32 capability to be exposed anyway. I just noticed this nasty corner case too. Documentation/admin-guide/cgroup-v1/cpusets.rst: Section 1.9 "If such a task had been bound to some subset of its cpuset using the sched_setaffinity() call, the task will be allowed to run on any CPU allowed in its new cpuset, negating the effect of the prior sched_setaffinity() call." So user space must put the tasks into a valid cpuset to fix the problem. Or make the scheduler behave like the affinity is associated with a cpuset. Can user space put the task into the correct cpuset without a race? Clone3 syscall learnt to specify a cgroup to attach to when forking. Should we do the same for execve()? Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:31 ` Qais Yousef @ 2020-10-22 10:16 ` Morten Rasmussen 2020-10-22 10:48 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Morten Rasmussen @ 2020-10-22 10:16 UTC (permalink / raw) To: Qais Yousef Cc: linux-arch, Marc Zyngier, Peter Zijlstra (Intel), Catalin Marinas, Linus Torvalds, James Morse, Greg Kroah-Hartman, Will Deacon, linux-arm-kernel On Wed, Oct 21, 2020 at 03:31:53PM +0100, Qais Yousef wrote: > On 10/21/20 15:33, Morten Rasmussen wrote: > > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > > one, though not as easy as automatic task placement by the scheduler (my > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > not a strong preference for any). > > > > Automatic task placement by the scheduler would mean giving up the > > requirement that the user-space affinity mask must always be honoured. > > Is that on the table? > > > > Killing aarch32 tasks with an empty intersection between the user-space > > mask and aarch32_mask is not really "automatic" and would require the > > aarch32 capability to be exposed anyway. > > I just noticed this nasty corner case too. > > > Documentation/admin-guide/cgroup-v1/cpusets.rst: Section 1.9 > > "If such a task had been bound to some subset of its cpuset using the > sched_setaffinity() call, the task will be allowed to run on any CPU allowed in > its new cpuset, negating the effect of the prior sched_setaffinity() call." > > So user space must put the tasks into a valid cpuset to fix the problem. Or > make the scheduler behave like the affinity is associated with a cpuset. > > Can user space put the task into the correct cpuset without a race? Clone3 > syscall learnt to specify a cgroup to attach to when forking. Should we do the > same for execve()? Putting a task in a cpuset overrides any affinity mask applied by a previous cpuset or sched_setaffinity() call. I wouldn't call it a corner case though. Android user-space is exploiting it all the time on some devices through the foreground, background, and top-app cgroups. If a tasks fork() the child task will belong to the same cgroup automatically. If you execve() you retain the previous affinity mask and cgroup. So putting parent task about to execve() into aarch32 into a cpuset with only aarch32 CPUs should be enough to never have the task or any of its child tasks SIGKILLED. A few simple experiments with fork() and execve() seems to confirm that. I don't see any changes needed in the kernel. Changing cgroup through clone could of course fail if user-space specifies an unsuitable cgroup. Fixing that would be part of fixing the cpuset setup in user-space which is required anyway. Morten ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 10:16 ` Morten Rasmussen @ 2020-10-22 10:48 ` Qais Yousef 0 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-22 10:48 UTC (permalink / raw) To: Morten Rasmussen Cc: linux-arch, Marc Zyngier, Peter Zijlstra (Intel), Catalin Marinas, Linus Torvalds, James Morse, Greg Kroah-Hartman, Will Deacon, linux-arm-kernel On 10/22/20 12:16, Morten Rasmussen wrote: > On Wed, Oct 21, 2020 at 03:31:53PM +0100, Qais Yousef wrote: > > On 10/21/20 15:33, Morten Rasmussen wrote: > > > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > not a strong preference for any). > > > > > > Automatic task placement by the scheduler would mean giving up the > > > requirement that the user-space affinity mask must always be honoured. > > > Is that on the table? > > > > > > Killing aarch32 tasks with an empty intersection between the user-space > > > mask and aarch32_mask is not really "automatic" and would require the > > > aarch32 capability to be exposed anyway. > > > > I just noticed this nasty corner case too. > > > > > > Documentation/admin-guide/cgroup-v1/cpusets.rst: Section 1.9 > > > > "If such a task had been bound to some subset of its cpuset using the > > sched_setaffinity() call, the task will be allowed to run on any CPU allowed in > > its new cpuset, negating the effect of the prior sched_setaffinity() call." > > > > So user space must put the tasks into a valid cpuset to fix the problem. Or > > make the scheduler behave like the affinity is associated with a cpuset. > > > > Can user space put the task into the correct cpuset without a race? Clone3 > > syscall learnt to specify a cgroup to attach to when forking. Should we do the > > same for execve()? > > Putting a task in a cpuset overrides any affinity mask applied by a > previous cpuset or sched_setaffinity() call. I wouldn't call it a corner > case though. Android user-space is exploiting it all the time on some > devices through the foreground, background, and top-app cgroups. Yep. What I was referring to is that if we go the kernel fixing affinity automatically route, that cpuset behavior will be problematic. In this case fixing the affinity at the task level will not be enough because cpusets will override it. So catering for that is another complication to take into account. > If a tasks fork() the child task will belong to the same cgroup > automatically. If you execve() you retain the previous affinity mask and > cgroup. So putting parent task about to execve() into aarch32 into a > cpuset with only aarch32 CPUs should be enough to never have the task or > any of its child tasks SIGKILLED. > > A few simple experiments with fork() and execve() seems to confirm that. +1 This made me wonder what happens when SCHED_RESET_ON_FORK is set. It only resets policty and priority. So we're good. > I don't see any changes needed in the kernel. Changing cgroup through > clone could of course fail if user-space specifies an unsuitable cgroup. > Fixing that would be part of fixing the cpuset setup in user-space which > is required anyway. +1 Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 12:15 ` Catalin Marinas 2020-10-21 13:20 ` Qais Yousef 2020-10-21 13:33 ` Morten Rasmussen @ 2020-10-21 14:41 ` Will Deacon 2020-10-21 15:03 ` Qais Yousef 2020-10-22 13:47 ` Qais Yousef 2 siblings, 2 replies; 57+ messages in thread From: Will Deacon @ 2020-10-21 14:41 UTC (permalink / raw) To: Catalin Marinas Cc: Marc Zyngier, Qais Yousef, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > On 2020-10-21 11:46, Qais Yousef wrote: > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > printed out > > > although it's set as FTR_HIDDEN. > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > 0x0000000000000012 > > > 0x0000000000000012 > > > 0x0000000000000011 > > > 0x0000000000000011 > > > > This looks like a terrible userspace interface. It exposes unrelated > > features, > > Not sure why the EL1 field ended up in here, that's not relevant to the > user. > > > and doesn't expose the single useful information that the kernel has: > > the cpumask describing the CPUs supporting AArch32 at EL0. Why not expose > > this synthetic piece of information which requires very little effort from > > userspace and doesn't spit out unrelated stuff? > > I thought the whole idea is to try and avoid the "very little effort" > part ;). > > > Not to mention the discrepancy with what userspace gets while reading > > the same register via the MRS emulation. > > > > Granted, the cpumask doesn't fit the cpu*/regs/identification hierarchy, > > but I don't think this fits either. > > We already expose MIDR and REVIDR via the current sysfs interface. We > can expand it to include _all_ the other ID_* regs currently available > to user via the MRS emulation and we won't have to debate what a new > interface would look like. The MRS emulation and the sysfs info should > probably match, though that means we need to expose the > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > I do agree that an AArch32 cpumask is an easier option both from the > kernel implementation perspective and from the application usability > one, though not as easy as automatic task placement by the scheduler (my > first preference, followed by the id_* regs and the aarch32 mask, though > not a strong preference for any). If a cpumask is easier to implement and easier to use, then I think that's what we should do. It's also then dead easy to disable if necessary by just returning 0. The only alternative I would prefer is not having to expose this information altogether, but I'm not sure that figuring this out from MIDR/REVIDR alone is reliable. Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:41 ` Will Deacon @ 2020-10-21 15:03 ` Qais Yousef 2020-10-21 15:23 ` Will Deacon 2020-10-22 13:47 ` Qais Yousef 1 sibling, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 15:03 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 15:41, Will Deacon wrote: > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > > On 2020-10-21 11:46, Qais Yousef wrote: > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > > printed out > > > > although it's set as FTR_HIDDEN. > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > 0x0000000000000012 > > > > 0x0000000000000012 > > > > 0x0000000000000011 > > > > 0x0000000000000011 > > > > > > This looks like a terrible userspace interface. It exposes unrelated > > > features, > > > > Not sure why the EL1 field ended up in here, that's not relevant to the > > user. > > > > > and doesn't expose the single useful information that the kernel has: > > > the cpumask describing the CPUs supporting AArch32 at EL0. Why not expose > > > this synthetic piece of information which requires very little effort from > > > userspace and doesn't spit out unrelated stuff? > > > > I thought the whole idea is to try and avoid the "very little effort" > > part ;). > > > > > Not to mention the discrepancy with what userspace gets while reading > > > the same register via the MRS emulation. > > > > > > Granted, the cpumask doesn't fit the cpu*/regs/identification hierarchy, > > > but I don't think this fits either. > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > can expand it to include _all_ the other ID_* regs currently available > > to user via the MRS emulation and we won't have to debate what a new > > interface would look like. The MRS emulation and the sysfs info should > > probably match, though that means we need to expose the > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > I do agree that an AArch32 cpumask is an easier option both from the > > kernel implementation perspective and from the application usability > > one, though not as easy as automatic task placement by the scheduler (my > > first preference, followed by the id_* regs and the aarch32 mask, though > > not a strong preference for any). > > If a cpumask is easier to implement and easier to use, then I think that's > what we should do. It's also then dead easy to disable if necessary by > just returning 0. The only alternative I would prefer is not having to > expose this information altogether, but I'm not sure that figuring this > out from MIDR/REVIDR alone is reliable. I did suggest this before, but I'll try gain. If we want to assume a custom bootloader and custom user space, we can make them provide the mask. For example, the new sysctl_enable_asym_32bit could be a cpumask instead of a bool as it currently is. Or we can make it a cmdline parameter too. In both cases some admin (bootloader or init process) has to ensure to fill it correctly for the target platform. The bootloader should be able to read the registers to figure out the mask. So more weight to make it a cmdline param. Then the rest of user space can easily parse the cpumask from /proc/sys/kernel/enable_asym_32bit or from /proc/cmdline. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 15:03 ` Qais Yousef @ 2020-10-21 15:23 ` Will Deacon 2020-10-21 16:07 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 15:23 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 04:03:13PM +0100, Qais Yousef wrote: > On 10/21/20 15:41, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 01:15:59PM +0100, Catalin Marinas wrote: > > > On Wed, Oct 21, 2020 at 12:09:58PM +0100, Marc Zyngier wrote: > > > > On 2020-10-21 11:46, Qais Yousef wrote: > > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is > > > > > printed out > > > > > although it's set as FTR_HIDDEN. > > > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > > > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > > > > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > 0x0000000000000012 > > > > > 0x0000000000000012 > > > > > 0x0000000000000011 > > > > > 0x0000000000000011 > > > > > > > > This looks like a terrible userspace interface. It exposes unrelated > > > > features, > > > > > > Not sure why the EL1 field ended up in here, that's not relevant to the > > > user. > > > > > > > and doesn't expose the single useful information that the kernel has: > > > > the cpumask describing the CPUs supporting AArch32 at EL0. Why not expose > > > > this synthetic piece of information which requires very little effort from > > > > userspace and doesn't spit out unrelated stuff? > > > > > > I thought the whole idea is to try and avoid the "very little effort" > > > part ;). > > > > > > > Not to mention the discrepancy with what userspace gets while reading > > > > the same register via the MRS emulation. > > > > > > > > Granted, the cpumask doesn't fit the cpu*/regs/identification hierarchy, > > > > but I don't think this fits either. > > > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > can expand it to include _all_ the other ID_* regs currently available > > > to user via the MRS emulation and we won't have to debate what a new > > > interface would look like. The MRS emulation and the sysfs info should > > > probably match, though that means we need to expose the > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > kernel implementation perspective and from the application usability > > > one, though not as easy as automatic task placement by the scheduler (my > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > not a strong preference for any). > > > > If a cpumask is easier to implement and easier to use, then I think that's > > what we should do. It's also then dead easy to disable if necessary by > > just returning 0. The only alternative I would prefer is not having to > > expose this information altogether, but I'm not sure that figuring this > > out from MIDR/REVIDR alone is reliable. > > I did suggest this before, but I'll try gain. If we want to assume a custom > bootloader and custom user space, we can make them provide the mask. Who mentioned a custom bootloader? In the context of Android, we're talking about a user-space that already manages scheduling affinity. > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > a bool as it currently is. Or we can make it a cmdline parameter too. > In both cases some admin (bootloader or init process) has to ensure to fill it > correctly for the target platform. The bootloader should be able to read the > registers to figure out the mask. So more weight to make it a cmdline param. I think this is adding complexity for the sake of it. I'm much more in favour of keeping the implementation and ABI as simple as possible: expose the fact that the system is heterogenous, have an opt-in for userspace to say it can handle that and let it handle it. Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 15:23 ` Will Deacon @ 2020-10-21 16:07 ` Qais Yousef 2020-10-21 17:23 ` Will Deacon 0 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 16:07 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 16:23, Will Deacon wrote: > > > If a cpumask is easier to implement and easier to use, then I think that's > > > what we should do. It's also then dead easy to disable if necessary by > > > just returning 0. The only alternative I would prefer is not having to > > > expose this information altogether, but I'm not sure that figuring this > > > out from MIDR/REVIDR alone is reliable. > > > > I did suggest this before, but I'll try gain. If we want to assume a custom > > bootloader and custom user space, we can make them provide the mask. > > Who mentioned a custom bootloader? In the context of Android, we're Custom bootloader as in a bootloader that needs to opt-in to enable the feature (pass the right cmdline param). Catalin suggested to make this a sysctl to allow also for runtime toggling. But the initial intention was to have this to enable it at cmdline. > talking about a user-space that already manages scheduling affinity. > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > a bool as it currently is. Or we can make it a cmdline parameter too. > > In both cases some admin (bootloader or init process) has to ensure to fill it > > correctly for the target platform. The bootloader should be able to read the > > registers to figure out the mask. So more weight to make it a cmdline param. > > I think this is adding complexity for the sake of it. I'm much more in I actually think it reduces complexity. No special ABI to generate the mask from the kernel. The same opt-in flag is the cpumask too. > favour of keeping the implementation and ABI as simple as possible: expose > the fact that the system is heterogenous, have an opt-in for userspace to > say it can handle that and let it handle it. It still has to do that. It's just the origin of the cpumask will change. So it really depends how you view the opt-in. Ie: it needs to be discovered vs the user space knows it exists and it just wants to enable it. So far we've been going in the latter direction AFAICT. My current implementation is terrible for discovery. I don't feel strongly about it anyway. Just an idea. I can understand the lack of appeal. Not sure if there's a none ugly solution yet :-) Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 16:07 ` Qais Yousef @ 2020-10-21 17:23 ` Will Deacon 2020-10-21 19:57 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 17:23 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 05:07:30PM +0100, Qais Yousef wrote: > On 10/21/20 16:23, Will Deacon wrote: > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > > what we should do. It's also then dead easy to disable if necessary by > > > > just returning 0. The only alternative I would prefer is not having to > > > > expose this information altogether, but I'm not sure that figuring this > > > > out from MIDR/REVIDR alone is reliable. > > > > > > I did suggest this before, but I'll try gain. If we want to assume a custom > > > bootloader and custom user space, we can make them provide the mask. > > > > Who mentioned a custom bootloader? In the context of Android, we're > > Custom bootloader as in a bootloader that needs to opt-in to enable the > feature (pass the right cmdline param). Catalin suggested to make this a sysctl > to allow also for runtime toggling. But the initial intention was to have this > to enable it at cmdline. Hmm, ok, I don't think allowing the cmdline to be specified means its a custom bootloader. > > talking about a user-space that already manages scheduling affinity. > > > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > > a bool as it currently is. Or we can make it a cmdline parameter too. > > > In both cases some admin (bootloader or init process) has to ensure to fill it > > > correctly for the target platform. The bootloader should be able to read the > > > registers to figure out the mask. So more weight to make it a cmdline param. > > > > I think this is adding complexity for the sake of it. I'm much more in > > I actually think it reduces complexity. No special ABI to generate the mask > from the kernel. The same opt-in flag is the cpumask too. Maybe I'm misunderstanding your proposal but having a cpumask instead of a bool means you now have to consider policy on a per-cpu basis, which adds an extra dimension to this. For example, do you allow that mask to be changed at runtime so that differents sets of CPUs now support 32-bit? Do you preserve it across hotplug? Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 17:23 ` Will Deacon @ 2020-10-21 19:57 ` Qais Yousef 2020-10-21 20:26 ` Will Deacon 0 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 19:57 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 18:23, Will Deacon wrote: > On Wed, Oct 21, 2020 at 05:07:30PM +0100, Qais Yousef wrote: > > On 10/21/20 16:23, Will Deacon wrote: > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > > > what we should do. It's also then dead easy to disable if necessary by > > > > > just returning 0. The only alternative I would prefer is not having to > > > > > expose this information altogether, but I'm not sure that figuring this > > > > > out from MIDR/REVIDR alone is reliable. > > > > > > > > I did suggest this before, but I'll try gain. If we want to assume a custom > > > > bootloader and custom user space, we can make them provide the mask. > > > > > > Who mentioned a custom bootloader? In the context of Android, we're > > > > Custom bootloader as in a bootloader that needs to opt-in to enable the > > feature (pass the right cmdline param). Catalin suggested to make this a sysctl > > to allow also for runtime toggling. But the initial intention was to have this > > to enable it at cmdline. > > Hmm, ok, I don't think allowing the cmdline to be specified means its a > custom bootloader. True it could be just added to chosen property in the DT file without any bootloader changes. Bad usage of English probably. I just meant the bootloader might need to be made aware of the opt-in process too. So it can potentially co-operate more. > > > talking about a user-space that already manages scheduling affinity. > > > > > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > > > a bool as it currently is. Or we can make it a cmdline parameter too. > > > > In both cases some admin (bootloader or init process) has to ensure to fill it > > > > correctly for the target platform. The bootloader should be able to read the > > > > registers to figure out the mask. So more weight to make it a cmdline param. > > > > > > I think this is adding complexity for the sake of it. I'm much more in > > > > I actually think it reduces complexity. No special ABI to generate the mask > > from the kernel. The same opt-in flag is the cpumask too. > > Maybe I'm misunderstanding your proposal but having a cpumask instead of What I meant is that if we change the requirement to opt-in from a boolean switch sysctl.enable_32bit_asym=1 to require the bootloader/init scripts provide the mask of aarch32 capable cpus sysctl.asym_32bit_cpus=0xf0 This will achieve multiple things at the same time: * Defer cpus specification to platform designers who want to enable this feature on their platform. * We don't need a separate API to export which cpus are 32bit capable. They can read it directly from /proc/sys/kernel/asym_32bit_cpus. When it's 0 it means the system is not asymmetric. * If/when we want to disable this support in the future. The sysctl handler will just have to return 0 all the time and ignore all writes. So it's changing the way user space opts-in. The kernel will still treat it as a boolean, and probably sanity check the cpus to make sure they match what it sees. > a bool means you now have to consider policy on a per-cpu basis, which > adds an extra dimension to this. For example, do you allow that mask to > be changed at runtime so that differents sets of CPUs now support 32-bit? > Do you preserve it across hotplug? I can see how cpumask word was confusing. I hope the above is clearer. We don't change how the kernel works, but rather what the user space have to do to opt-in. Hopefully hitting 2 birds with one stone :-) Thanks! -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 19:57 ` Qais Yousef @ 2020-10-21 20:26 ` Will Deacon 2020-10-22 8:16 ` Catalin Marinas 0 siblings, 1 reply; 57+ messages in thread From: Will Deacon @ 2020-10-21 20:26 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 08:57:36PM +0100, Qais Yousef wrote: > On 10/21/20 18:23, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 05:07:30PM +0100, Qais Yousef wrote: > > > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > > > > a bool as it currently is. Or we can make it a cmdline parameter too. > > > > > In both cases some admin (bootloader or init process) has to ensure to fill it > > > > > correctly for the target platform. The bootloader should be able to read the > > > > > registers to figure out the mask. So more weight to make it a cmdline param. > > > > > > > > I think this is adding complexity for the sake of it. I'm much more in > > > > > > I actually think it reduces complexity. No special ABI to generate the mask > > > from the kernel. The same opt-in flag is the cpumask too. > > > > Maybe I'm misunderstanding your proposal but having a cpumask instead of > > What I meant is that if we change the requirement to opt-in from a boolean > switch > > sysctl.enable_32bit_asym=1 > > to require the bootloader/init scripts provide the mask of aarch32 capable cpus > > sysctl.asym_32bit_cpus=0xf0 > > This will achieve multiple things at the same time: > > * Defer cpus specification to platform designers who want to > enable this feature on their platform. What do you mean by this? The kernel will obviously have to check that the mask is correct (i.e. it doesn't contain 64-bit only CPUs), at which point it's a boolean in disguise. > * We don't need a separate API to export which cpus are 32bit capable. > They can read it directly from /proc/sys/kernel/asym_32bit_cpus. > When it's 0 it means the system is not asymmetric. I don't see how this is better than a separate cpumask for this purpose. It feels like you're trying to overload the control and the identification, but that just makes things confusing and hard to use as I now need to know which logical CPUs correspond to which physical CPUs in order to set the command-line. > * If/when we want to disable this support in the future. The sysctl > handler will just have to return 0 all the time and ignore all > writes. It can do that as a boolean too, right? Will ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 20:26 ` Will Deacon @ 2020-10-22 8:16 ` Catalin Marinas 2020-10-22 9:58 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Catalin Marinas @ 2020-10-22 8:16 UTC (permalink / raw) To: Will Deacon Cc: Qais Yousef, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 09:26:27PM +0100, Will Deacon wrote: > On Wed, Oct 21, 2020 at 08:57:36PM +0100, Qais Yousef wrote: > > On 10/21/20 18:23, Will Deacon wrote: > > > On Wed, Oct 21, 2020 at 05:07:30PM +0100, Qais Yousef wrote: > > > > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > > > > > a bool as it currently is. Or we can make it a cmdline parameter too. > > > > > > In both cases some admin (bootloader or init process) has to ensure to fill it > > > > > > correctly for the target platform. The bootloader should be able to read the > > > > > > registers to figure out the mask. So more weight to make it a cmdline param. > > > > > > > > > > I think this is adding complexity for the sake of it. I'm much more in > > > > > > > > I actually think it reduces complexity. No special ABI to generate the mask > > > > from the kernel. The same opt-in flag is the cpumask too. > > > > > > Maybe I'm misunderstanding your proposal but having a cpumask instead of > > > > What I meant is that if we change the requirement to opt-in from a boolean > > switch > > > > sysctl.enable_32bit_asym=1 > > > > to require the bootloader/init scripts provide the mask of aarch32 capable cpus > > > > sysctl.asym_32bit_cpus=0xf0 [...] > > * We don't need a separate API to export which cpus are 32bit capable. > > They can read it directly from /proc/sys/kernel/asym_32bit_cpus. > > When it's 0 it means the system is not asymmetric. > > I don't see how this is better than a separate cpumask for this purpose. > It feels like you're trying to overload the control and the identification, > but that just makes things confusing and hard to use as I now need to know > which logical CPUs correspond to which physical CPUs in order to set the > command-line. I agree. Let's leave the identification to the kernel as it has access to the CPUID registers and can provide the cpumask. The control in this case doesn't need to be more than a boolean and its meaning is that the user knows what it is doing. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 8:16 ` Catalin Marinas @ 2020-10-22 9:58 ` Qais Yousef 0 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-22 9:58 UTC (permalink / raw) To: Catalin Marinas Cc: Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/22/20 09:16, Catalin Marinas wrote: > On Wed, Oct 21, 2020 at 09:26:27PM +0100, Will Deacon wrote: > > On Wed, Oct 21, 2020 at 08:57:36PM +0100, Qais Yousef wrote: > > > On 10/21/20 18:23, Will Deacon wrote: > > > > On Wed, Oct 21, 2020 at 05:07:30PM +0100, Qais Yousef wrote: > > > > > > > For example, the new sysctl_enable_asym_32bit could be a cpumask instead of > > > > > > > a bool as it currently is. Or we can make it a cmdline parameter too. > > > > > > > In both cases some admin (bootloader or init process) has to ensure to fill it > > > > > > > correctly for the target platform. The bootloader should be able to read the > > > > > > > registers to figure out the mask. So more weight to make it a cmdline param. > > > > > > > > > > > > I think this is adding complexity for the sake of it. I'm much more in > > > > > > > > > > I actually think it reduces complexity. No special ABI to generate the mask > > > > > from the kernel. The same opt-in flag is the cpumask too. > > > > > > > > Maybe I'm misunderstanding your proposal but having a cpumask instead of > > > > > > What I meant is that if we change the requirement to opt-in from a boolean > > > switch > > > > > > sysctl.enable_32bit_asym=1 > > > > > > to require the bootloader/init scripts provide the mask of aarch32 capable cpus > > > > > > sysctl.asym_32bit_cpus=0xf0 > [...] > > > * We don't need a separate API to export which cpus are 32bit capable. > > > They can read it directly from /proc/sys/kernel/asym_32bit_cpus. > > > When it's 0 it means the system is not asymmetric. > > > > I don't see how this is better than a separate cpumask for this purpose. > > It feels like you're trying to overload the control and the identification, > > but that just makes things confusing and hard to use as I now need to know > > which logical CPUs correspond to which physical CPUs in order to set the > > command-line. I tend to disagree with some of the statements. But I'll leave it at that. Whatever makes the ship move :) > I agree. Let's leave the identification to the kernel as it has access > to the CPUID registers and can provide the cpumask. The control in this > case doesn't need to be more than a boolean and its meaning is that the > user knows what it is doing. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 14:41 ` Will Deacon 2020-10-21 15:03 ` Qais Yousef @ 2020-10-22 13:47 ` Qais Yousef 2020-10-22 13:55 ` Greg Kroah-Hartman 1 sibling, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-22 13:47 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Greg Kroah-Hartman, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 15:41, Will Deacon wrote: > > We already expose MIDR and REVIDR via the current sysfs interface. We > > can expand it to include _all_ the other ID_* regs currently available > > to user via the MRS emulation and we won't have to debate what a new > > interface would look like. The MRS emulation and the sysfs info should > > probably match, though that means we need to expose the > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > I do agree that an AArch32 cpumask is an easier option both from the > > kernel implementation perspective and from the application usability > > one, though not as easy as automatic task placement by the scheduler (my > > first preference, followed by the id_* regs and the aarch32 mask, though > > not a strong preference for any). > > If a cpumask is easier to implement and easier to use, then I think that's > what we should do. It's also then dead easy to disable if necessary by > just returning 0. The only alternative I would prefer is not having to > expose this information altogether, but I'm not sure that figuring this > out from MIDR/REVIDR alone is reliable. So the mask idea is about adding a new /sys/devices/system/cpu/aarch32_cpus ? I just need to make sure that Peter and Greg are happy with this arm64 specific mask added to sysfs in this manner. Not sure if there's a precedent of archs exporting special masks in sysfs. Or maybe people had something else in mind about how his this mask should be exported? Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 13:47 ` Qais Yousef @ 2020-10-22 13:55 ` Greg Kroah-Hartman 2020-10-22 14:31 ` Catalin Marinas 2020-10-26 19:02 ` Qais Yousef 0 siblings, 2 replies; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-22 13:55 UTC (permalink / raw) To: Qais Yousef Cc: Will Deacon, Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > On 10/21/20 15:41, Will Deacon wrote: > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > can expand it to include _all_ the other ID_* regs currently available > > > to user via the MRS emulation and we won't have to debate what a new > > > interface would look like. The MRS emulation and the sysfs info should > > > probably match, though that means we need to expose the > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > kernel implementation perspective and from the application usability > > > one, though not as easy as automatic task placement by the scheduler (my > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > not a strong preference for any). > > > > If a cpumask is easier to implement and easier to use, then I think that's > > what we should do. It's also then dead easy to disable if necessary by > > just returning 0. The only alternative I would prefer is not having to > > expose this information altogether, but I'm not sure that figuring this > > out from MIDR/REVIDR alone is reliable. > > So the mask idea is about adding a new > > /sys/devices/system/cpu/aarch32_cpus > > ? Is this a file, a directory, or what? What's the contents? Without any of that, I have no idea if it's "ok" or not... thanks, greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 13:55 ` Greg Kroah-Hartman @ 2020-10-22 14:31 ` Catalin Marinas 2020-10-22 14:34 ` Qais Yousef 2020-10-26 19:02 ` Qais Yousef 1 sibling, 1 reply; 57+ messages in thread From: Catalin Marinas @ 2020-10-22 14:31 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Qais Yousef, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Thu, Oct 22, 2020 at 03:55:59PM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > > On 10/21/20 15:41, Will Deacon wrote: > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > > can expand it to include _all_ the other ID_* regs currently available > > > > to user via the MRS emulation and we won't have to debate what a new > > > > interface would look like. The MRS emulation and the sysfs info should > > > > probably match, though that means we need to expose the > > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > > kernel implementation perspective and from the application usability > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > not a strong preference for any). > > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > what we should do. It's also then dead easy to disable if necessary by > > > just returning 0. The only alternative I would prefer is not having to > > > expose this information altogether, but I'm not sure that figuring this > > > out from MIDR/REVIDR alone is reliable. > > > > So the mask idea is about adding a new > > > > /sys/devices/system/cpu/aarch32_cpus > > > > ? > > Is this a file, a directory, or what? What's the contents? > > Without any of that, I have no idea if it's "ok" or not... I don't know what Qais thinks but a file matching the syntax of the present/online/offline/possible files (e.g. 0-3) would look more consistent. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 14:31 ` Catalin Marinas @ 2020-10-22 14:34 ` Qais Yousef 0 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-22 14:34 UTC (permalink / raw) To: Catalin Marinas Cc: Greg Kroah-Hartman, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/22/20 15:31, Catalin Marinas wrote: > On Thu, Oct 22, 2020 at 03:55:59PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > > > On 10/21/20 15:41, Will Deacon wrote: > > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > > > can expand it to include _all_ the other ID_* regs currently available > > > > > to user via the MRS emulation and we won't have to debate what a new > > > > > interface would look like. The MRS emulation and the sysfs info should > > > > > probably match, though that means we need to expose the > > > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > > > kernel implementation perspective and from the application usability > > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > > not a strong preference for any). > > > > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > > what we should do. It's also then dead easy to disable if necessary by > > > > just returning 0. The only alternative I would prefer is not having to > > > > expose this information altogether, but I'm not sure that figuring this > > > > out from MIDR/REVIDR alone is reliable. > > > > > > So the mask idea is about adding a new > > > > > > /sys/devices/system/cpu/aarch32_cpus > > > > > > ? > > > > Is this a file, a directory, or what? What's the contents? > > > > Without any of that, I have no idea if it's "ok" or not... > > I don't know what Qais thinks but a file matching the syntax of the > present/online/offline/possible files (e.g. 0-3) would look more > consistent. Yes. Sorry I thought it's obvious. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-22 13:55 ` Greg Kroah-Hartman 2020-10-22 14:31 ` Catalin Marinas @ 2020-10-26 19:02 ` Qais Yousef 2020-10-26 19:08 ` Greg Kroah-Hartman 1 sibling, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-26 19:02 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Will Deacon, Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/22/20 15:55, Greg Kroah-Hartman wrote: > On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > > On 10/21/20 15:41, Will Deacon wrote: > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > > can expand it to include _all_ the other ID_* regs currently available > > > > to user via the MRS emulation and we won't have to debate what a new > > > > interface would look like. The MRS emulation and the sysfs info should > > > > probably match, though that means we need to expose the > > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > > kernel implementation perspective and from the application usability > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > not a strong preference for any). > > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > what we should do. It's also then dead easy to disable if necessary by > > > just returning 0. The only alternative I would prefer is not having to > > > expose this information altogether, but I'm not sure that figuring this > > > out from MIDR/REVIDR alone is reliable. > > > > So the mask idea is about adding a new > > > > /sys/devices/system/cpu/aarch32_cpus > > > > ? > > Is this a file, a directory, or what? What's the contents? > > Without any of that, I have no idea if it's "ok" or not... Hopefully the below patch explains better. Note that I added the new attribute to driver/base/cpu.c, but assuming we will still want to go down this route, we will need a generic way for archs to add their attributes to /sys/devices/system/cpu/. Something like having a special define for archs to append their own attributes list #define SYSFS_SYSTEM_CPU_ARCH_ATTRIBUTES Or probably there's a way to add this file (attribute) dynamically from arch code that I just didn't figure out how to do yet. Thanks -- Qais Yousef ---------->8------------ From 96dfdfdacb2a26a60ba19051e8c72e839eb5408b Mon Sep 17 00:00:00 2001 From: Qais Yousef <qais.yousef@arm.com> Date: Mon, 26 Oct 2020 16:33:32 +0000 Subject: [PATCH] arm64: export aarch32_online mask in sysfs This patch to be applied on top of arm64 Asymmetric AArch32 support. It explores the option of exporting the AArch32 capable cpus as a mask on sysfs. This is to help drive the discussion on the API before sending the next version which I yet to address some of the review comments. The output looks like: # cat /sys/devices/system/cpu/aarch32_online 0-5 Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- Documentation/ABI/testing/sysfs-devices-system-cpu | 7 +++++++ arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/cpufeature.c | 8 ++++++++ drivers/base/cpu.c | 12 ++++++++++++ 4 files changed, 29 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu index b555df825447..9ccb5c3f5ee3 100644 --- a/Documentation/ABI/testing/sysfs-devices-system-cpu +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu @@ -36,6 +36,13 @@ Description: CPU topology files that describe kernel limits related to See Documentation/admin-guide/cputopology.rst for more information. +What: /sys/devices/system/cpu/aarch32_online +Date: October 2020 +Contact: Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org> +Description: CPU topology file that describes which cpus support AArch32 at + EL0. Only available on arm64. + + The value is updated when a cpu becomes online then sticks. What: /sys/devices/system/cpu/probe /sys/devices/system/cpu/release diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 2b87f17b2bd4..edd18002ad81 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -380,6 +380,8 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, return false; } +extern cpumask_t aarch32_el0_mask; + extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS); extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS]; extern struct static_key_false arm64_const_caps_ready; diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 0f7307c8ad80..662bbc2b15cd 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1723,6 +1723,13 @@ cpucap_panic_on_conflict(const struct arm64_cpu_capabilities *cap) return !!(cap->type & ARM64_CPUCAP_PANIC_ON_CONFLICT); } +cpumask_t aarch32_el0_mask; +static void cpu_enable_aarch32_el0(struct arm64_cpu_capabilities const *cap) +{ + if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) + cpumask_set_cpu(smp_processor_id(), &aarch32_el0_mask); +} + static const struct arm64_cpu_capabilities arm64_features[] = { { .desc = "GIC system register CPU interface", @@ -1809,6 +1816,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { { .capability = ARM64_HAS_ASYM_32BIT_EL0, .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .cpu_enable = cpu_enable_aarch32_el0, .matches = has_cpuid_feature, .sys_reg = SYS_ID_AA64PFR0_EL1, .sign = FTR_UNSIGNED, diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index d2136ab9b14a..569baacde508 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -459,6 +459,15 @@ EXPORT_SYMBOL_GPL(cpu_device_create); static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); #endif +#ifdef CONFIG_ARM64 +static ssize_t print_aarch32_online(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return cpumap_print_to_pagebuf(true, buf, &aarch32_el0_mask); +} +static DEVICE_ATTR(aarch32_online, 0444, print_aarch32_online, NULL); +#endif + static struct attribute *cpu_root_attrs[] = { #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE &dev_attr_probe.attr, @@ -475,6 +484,9 @@ static struct attribute *cpu_root_attrs[] = { #endif #ifdef CONFIG_GENERIC_CPU_AUTOPROBE &dev_attr_modalias.attr, +#endif +#ifdef CONFIG_ARM64 + &dev_attr_aarch32_online.attr, #endif NULL }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-26 19:02 ` Qais Yousef @ 2020-10-26 19:08 ` Greg Kroah-Hartman 2020-10-26 19:18 ` Qais Yousef 0 siblings, 1 reply; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-26 19:08 UTC (permalink / raw) To: Qais Yousef Cc: Will Deacon, Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Mon, Oct 26, 2020 at 07:02:50PM +0000, Qais Yousef wrote: > On 10/22/20 15:55, Greg Kroah-Hartman wrote: > > On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > > > On 10/21/20 15:41, Will Deacon wrote: > > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > > > can expand it to include _all_ the other ID_* regs currently available > > > > > to user via the MRS emulation and we won't have to debate what a new > > > > > interface would look like. The MRS emulation and the sysfs info should > > > > > probably match, though that means we need to expose the > > > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > > > kernel implementation perspective and from the application usability > > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > > not a strong preference for any). > > > > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > > what we should do. It's also then dead easy to disable if necessary by > > > > just returning 0. The only alternative I would prefer is not having to > > > > expose this information altogether, but I'm not sure that figuring this > > > > out from MIDR/REVIDR alone is reliable. > > > > > > So the mask idea is about adding a new > > > > > > /sys/devices/system/cpu/aarch32_cpus > > > > > > ? > > > > Is this a file, a directory, or what? What's the contents? > > > > Without any of that, I have no idea if it's "ok" or not... > > Hopefully the below patch explains better. Note that I added the new attribute > to driver/base/cpu.c, but assuming we will still want to go down this route, we > will need a generic way for archs to add their attributes to > /sys/devices/system/cpu/. > > Something like having a special define for archs to append their own > attributes list > > #define SYSFS_SYSTEM_CPU_ARCH_ATTRIBUTES > > Or probably there's a way to add this file (attribute) dynamically from arch > code that I just didn't figure out how to do yet. Please do that, sysfs files should not be present when the information is not needed from them. Look at the is_visible() callback for the attribute for how to do it. > > Thanks > > -- > Qais Yousef > > > ---------->8------------ > > >From 96dfdfdacb2a26a60ba19051e8c72e839eb5408b Mon Sep 17 00:00:00 2001 > From: Qais Yousef <qais.yousef@arm.com> > Date: Mon, 26 Oct 2020 16:33:32 +0000 > Subject: [PATCH] arm64: export aarch32_online mask in sysfs > > This patch to be applied on top of arm64 Asymmetric AArch32 support. > > It explores the option of exporting the AArch32 capable cpus as a mask > on sysfs. > > This is to help drive the discussion on the API before sending the next > version which I yet to address some of the review comments. > > The output looks like: > > # cat /sys/devices/system/cpu/aarch32_online > 0-5 > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > Documentation/ABI/testing/sysfs-devices-system-cpu | 7 +++++++ > arch/arm64/include/asm/cpufeature.h | 2 ++ > arch/arm64/kernel/cpufeature.c | 8 ++++++++ > drivers/base/cpu.c | 12 ++++++++++++ > 4 files changed, 29 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu > index b555df825447..9ccb5c3f5ee3 100644 > --- a/Documentation/ABI/testing/sysfs-devices-system-cpu > +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu > @@ -36,6 +36,13 @@ Description: CPU topology files that describe kernel limits related to > > See Documentation/admin-guide/cputopology.rst for more information. > > +What: /sys/devices/system/cpu/aarch32_online > +Date: October 2020 > +Contact: Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org> > +Description: CPU topology file that describes which cpus support AArch32 at > + EL0. Only available on arm64. > + > + The value is updated when a cpu becomes online then sticks. What does "then sticks" mean? > > What: /sys/devices/system/cpu/probe > /sys/devices/system/cpu/release > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 2b87f17b2bd4..edd18002ad81 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -380,6 +380,8 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, > return false; > } > > +extern cpumask_t aarch32_el0_mask; > + > extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS); > extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS]; > extern struct static_key_false arm64_const_caps_ready; > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 0f7307c8ad80..662bbc2b15cd 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1723,6 +1723,13 @@ cpucap_panic_on_conflict(const struct arm64_cpu_capabilities *cap) > return !!(cap->type & ARM64_CPUCAP_PANIC_ON_CONFLICT); > } > > +cpumask_t aarch32_el0_mask; > +static void cpu_enable_aarch32_el0(struct arm64_cpu_capabilities const *cap) > +{ > + if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) > + cpumask_set_cpu(smp_processor_id(), &aarch32_el0_mask); > +} > + > static const struct arm64_cpu_capabilities arm64_features[] = { > { > .desc = "GIC system register CPU interface", > @@ -1809,6 +1816,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > { > .capability = ARM64_HAS_ASYM_32BIT_EL0, > .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > + .cpu_enable = cpu_enable_aarch32_el0, > .matches = has_cpuid_feature, > .sys_reg = SYS_ID_AA64PFR0_EL1, > .sign = FTR_UNSIGNED, > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index d2136ab9b14a..569baacde508 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -459,6 +459,15 @@ EXPORT_SYMBOL_GPL(cpu_device_create); > static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); > #endif > > +#ifdef CONFIG_ARM64 > +static ssize_t print_aarch32_online(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return cpumap_print_to_pagebuf(true, buf, &aarch32_el0_mask); > +} > +static DEVICE_ATTR(aarch32_online, 0444, print_aarch32_online, NULL); DEVICE_ATTR_RO()? > +#endif Hah, no, no arch-specific stuff in here, sorry. Please do this properly in your arch-specific code only. thanks, greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-26 19:08 ` Greg Kroah-Hartman @ 2020-10-26 19:18 ` Qais Yousef 0 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-26 19:18 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Will Deacon, Catalin Marinas, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/26/20 20:08, Greg Kroah-Hartman wrote: > On Mon, Oct 26, 2020 at 07:02:50PM +0000, Qais Yousef wrote: > > On 10/22/20 15:55, Greg Kroah-Hartman wrote: > > > On Thu, Oct 22, 2020 at 02:47:52PM +0100, Qais Yousef wrote: > > > > On 10/21/20 15:41, Will Deacon wrote: > > > > > > We already expose MIDR and REVIDR via the current sysfs interface. We > > > > > > can expand it to include _all_ the other ID_* regs currently available > > > > > > to user via the MRS emulation and we won't have to debate what a new > > > > > > interface would look like. The MRS emulation and the sysfs info should > > > > > > probably match, though that means we need to expose the > > > > > > ID_AA64PFR0_EL1.EL0 field which we currently don't. > > > > > > > > > > > > I do agree that an AArch32 cpumask is an easier option both from the > > > > > > kernel implementation perspective and from the application usability > > > > > > one, though not as easy as automatic task placement by the scheduler (my > > > > > > first preference, followed by the id_* regs and the aarch32 mask, though > > > > > > not a strong preference for any). > > > > > > > > > > If a cpumask is easier to implement and easier to use, then I think that's > > > > > what we should do. It's also then dead easy to disable if necessary by > > > > > just returning 0. The only alternative I would prefer is not having to > > > > > expose this information altogether, but I'm not sure that figuring this > > > > > out from MIDR/REVIDR alone is reliable. > > > > > > > > So the mask idea is about adding a new > > > > > > > > /sys/devices/system/cpu/aarch32_cpus > > > > > > > > ? > > > > > > Is this a file, a directory, or what? What's the contents? > > > > > > Without any of that, I have no idea if it's "ok" or not... > > > > Hopefully the below patch explains better. Note that I added the new attribute > > to driver/base/cpu.c, but assuming we will still want to go down this route, we > > will need a generic way for archs to add their attributes to > > /sys/devices/system/cpu/. > > > > Something like having a special define for archs to append their own > > attributes list > > > > #define SYSFS_SYSTEM_CPU_ARCH_ATTRIBUTES > > > > Or probably there's a way to add this file (attribute) dynamically from arch > > code that I just didn't figure out how to do yet. > > Please do that, sysfs files should not be present when the information > is not needed from them. Look at the is_visible() callback for the > attribute for how to do it. Okay, thanks for the hint. Will look at that. > > > > Thanks > > > > -- > > Qais Yousef > > > > > > ---------->8------------ > > > > >From 96dfdfdacb2a26a60ba19051e8c72e839eb5408b Mon Sep 17 00:00:00 2001 > > From: Qais Yousef <qais.yousef@arm.com> > > Date: Mon, 26 Oct 2020 16:33:32 +0000 > > Subject: [PATCH] arm64: export aarch32_online mask in sysfs > > > > This patch to be applied on top of arm64 Asymmetric AArch32 support. > > > > It explores the option of exporting the AArch32 capable cpus as a mask > > on sysfs. > > > > This is to help drive the discussion on the API before sending the next > > version which I yet to address some of the review comments. > > > > The output looks like: > > > > # cat /sys/devices/system/cpu/aarch32_online > > 0-5 > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > --- > > Documentation/ABI/testing/sysfs-devices-system-cpu | 7 +++++++ > > arch/arm64/include/asm/cpufeature.h | 2 ++ > > arch/arm64/kernel/cpufeature.c | 8 ++++++++ > > drivers/base/cpu.c | 12 ++++++++++++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu > > index b555df825447..9ccb5c3f5ee3 100644 > > --- a/Documentation/ABI/testing/sysfs-devices-system-cpu > > +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu > > @@ -36,6 +36,13 @@ Description: CPU topology files that describe kernel limits related to > > > > See Documentation/admin-guide/cputopology.rst for more information. > > > > +What: /sys/devices/system/cpu/aarch32_online > > +Date: October 2020 > > +Contact: Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org> > > +Description: CPU topology file that describes which cpus support AArch32 at > > + EL0. Only available on arm64. > > + > > + The value is updated when a cpu becomes online then sticks. > > What does "then sticks" mean? Was thinking like a sticky bit. When a cpu becomes online and we discover that it is aarch32 capable, we set the bit. But never clear it again if the cpu goes offline later. I'll reword it. > > > > > > What: /sys/devices/system/cpu/probe > > /sys/devices/system/cpu/release > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > > index 2b87f17b2bd4..edd18002ad81 100644 > > --- a/arch/arm64/include/asm/cpufeature.h > > +++ b/arch/arm64/include/asm/cpufeature.h > > @@ -380,6 +380,8 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, > > return false; > > } > > > > +extern cpumask_t aarch32_el0_mask; > > + > > extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS); > > extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS]; > > extern struct static_key_false arm64_const_caps_ready; > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 0f7307c8ad80..662bbc2b15cd 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -1723,6 +1723,13 @@ cpucap_panic_on_conflict(const struct arm64_cpu_capabilities *cap) > > return !!(cap->type & ARM64_CPUCAP_PANIC_ON_CONFLICT); > > } > > > > +cpumask_t aarch32_el0_mask; > > +static void cpu_enable_aarch32_el0(struct arm64_cpu_capabilities const *cap) > > +{ > > + if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) > > + cpumask_set_cpu(smp_processor_id(), &aarch32_el0_mask); > > +} > > + > > static const struct arm64_cpu_capabilities arm64_features[] = { > > { > > .desc = "GIC system register CPU interface", > > @@ -1809,6 +1816,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > > { > > .capability = ARM64_HAS_ASYM_32BIT_EL0, > > .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > > + .cpu_enable = cpu_enable_aarch32_el0, > > .matches = has_cpuid_feature, > > .sys_reg = SYS_ID_AA64PFR0_EL1, > > .sign = FTR_UNSIGNED, > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > > index d2136ab9b14a..569baacde508 100644 > > --- a/drivers/base/cpu.c > > +++ b/drivers/base/cpu.c > > @@ -459,6 +459,15 @@ EXPORT_SYMBOL_GPL(cpu_device_create); > > static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); > > #endif > > > > +#ifdef CONFIG_ARM64 > > +static ssize_t print_aarch32_online(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + return cpumap_print_to_pagebuf(true, buf, &aarch32_el0_mask); > > +} > > +static DEVICE_ATTR(aarch32_online, 0444, print_aarch32_online, NULL); > > DEVICE_ATTR_RO()? Indeed. > > > +#endif > > Hah, no, no arch-specific stuff in here, sorry. Please do this properly > in your arch-specific code only. Of course. It was just to see this is okay. Let me figure out how to clean this up. Thanks! -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 10:46 ` [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs Qais Yousef 2020-10-21 11:09 ` Marc Zyngier @ 2020-10-21 11:28 ` Greg Kroah-Hartman 2020-10-21 13:22 ` Qais Yousef 1 sibling, 1 reply; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-21 11:28 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 11:46:11AM +0100, Qais Yousef wrote: > So that userspace can detect if the cpu has aarch32 support at EL0. > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect > what it does. And fixed to accept both u64 and u32 without causing the > printf to print out a warning about mismatched type. This was caught > while testing to check the new CPUREGS_USER_ATTR_RO(). > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > on a @cond to user space. The exported fields match the definition in > arm64_ftr_reg so that the content of a register exported via MRS and > sysfs are kept cohesive. > > The @cond in our case is that the system is asymmetric aarch32 and the > controlling sysctl.enable_asym_32bit is enabled. > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > newly visible EL0 field in ID_AA64FPR0_EL1. > > Note that the MRS interface will still return the sanitized content > _only_. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > > Example output. I was surprised that the 2nd field (bits[7:4]) is printed out > although it's set as FTR_HIDDEN. > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000011 > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > 0x0000000000000011 > 0x0000000000000011 > 0x0000000000000012 > 0x0000000000000012 > 0x0000000000000011 > 0x0000000000000011 > > Documentation/arm64/cpu-feature-registers.rst | 2 +- > arch/arm64/kernel/cpufeature.c | 2 +- > arch/arm64/kernel/cpuinfo.c | 58 +++++++++++++++++-- > 3 files changed, 54 insertions(+), 8 deletions(-) > > diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst > index f28853f80089..bfcbda6d6f35 100644 > --- a/Documentation/arm64/cpu-feature-registers.rst > +++ b/Documentation/arm64/cpu-feature-registers.rst > @@ -166,7 +166,7 @@ infrastructure: > +------------------------------+---------+---------+ > | EL1 | [7-4] | n | > +------------------------------+---------+---------+ > - | EL0 | [3-0] | n | > + | EL0 | [3-0] | y | > +------------------------------+---------+---------+ > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 6f795c8221f4..0f7307c8ad80 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -221,7 +221,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > + ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > ARM64_FTR_END, > }; > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > index 93c55986ca7f..632b9d5b5230 100644 > --- a/arch/arm64/kernel/cpuinfo.c > +++ b/arch/arm64/kernel/cpuinfo.c > @@ -231,25 +231,71 @@ static struct kobj_type cpuregs_kobj_type = { > * future expansion without an ABI break. > */ > #define kobj_to_cpuinfo(kobj) container_of(kobj, struct cpuinfo_arm64, kobj) > -#define CPUREGS_ATTR_RO(_name, _field) \ > +#define CPUREGS_RAW_ATTR_RO(_name, _field) \ > static ssize_t _name##_show(struct kobject *kobj, \ > struct kobj_attribute *attr, char *buf) \ > { \ > struct cpuinfo_arm64 *info = kobj_to_cpuinfo(kobj); \ > \ > - if (info->reg_midr) \ > - return sprintf(buf, "0x%016x\n", info->reg_##_field); \ > - else \ > + if (info->reg_midr) { \ > + u64 val = info->reg_##_field; \ > + return sprintf(buf, "0x%016llx\n", val); \ Nit, for sysfs, use the new sysfs_emit() call instead of sprintf(). thanks, greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs 2020-10-21 11:28 ` Greg Kroah-Hartman @ 2020-10-21 13:22 ` Qais Yousef 0 siblings, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:22 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 13:28, Greg Kroah-Hartman wrote: > On Wed, Oct 21, 2020 at 11:46:11AM +0100, Qais Yousef wrote: > > So that userspace can detect if the cpu has aarch32 support at EL0. > > > > CPUREGS_ATTR_RO() was renamed to CPUREGS_RAW_ATTR_RO() to better reflect > > what it does. And fixed to accept both u64 and u32 without causing the > > printf to print out a warning about mismatched type. This was caught > > while testing to check the new CPUREGS_USER_ATTR_RO(). > > > > The new CPUREGS_USER_ATTR_RO() exports a Sanitised or RAW sys_reg based > > on a @cond to user space. The exported fields match the definition in > > arm64_ftr_reg so that the content of a register exported via MRS and > > sysfs are kept cohesive. > > > > The @cond in our case is that the system is asymmetric aarch32 and the > > controlling sysctl.enable_asym_32bit is enabled. > > > > Update Documentation/arm64/cpu-feature-registers.rst to reflect the > > newly visible EL0 field in ID_AA64FPR0_EL1. > > > > Note that the MRS interface will still return the sanitized content > > _only_. > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > --- > > > > Example output. I was surprised that the 2nd field (bits[7:4]) is printed out > > although it's set as FTR_HIDDEN. > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000011 > > > > # echo 1 > /proc/sys/kernel/enable_asym_32bit > > > > # cat /sys/devices/system/cpu/cpu*/regs/identification/id_aa64pfr0 > > 0x0000000000000011 > > 0x0000000000000011 > > 0x0000000000000012 > > 0x0000000000000012 > > 0x0000000000000011 > > 0x0000000000000011 > > > > Documentation/arm64/cpu-feature-registers.rst | 2 +- > > arch/arm64/kernel/cpufeature.c | 2 +- > > arch/arm64/kernel/cpuinfo.c | 58 +++++++++++++++++-- > > 3 files changed, 54 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst > > index f28853f80089..bfcbda6d6f35 100644 > > --- a/Documentation/arm64/cpu-feature-registers.rst > > +++ b/Documentation/arm64/cpu-feature-registers.rst > > @@ -166,7 +166,7 @@ infrastructure: > > +------------------------------+---------+---------+ > > | EL1 | [7-4] | n | > > +------------------------------+---------+---------+ > > - | EL0 | [3-0] | n | > > + | EL0 | [3-0] | y | > > +------------------------------+---------+---------+ > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 6f795c8221f4..0f7307c8ad80 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -221,7 +221,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = { > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0), > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY), > > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > > + ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY), > > ARM64_FTR_END, > > }; > > > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > > index 93c55986ca7f..632b9d5b5230 100644 > > --- a/arch/arm64/kernel/cpuinfo.c > > +++ b/arch/arm64/kernel/cpuinfo.c > > @@ -231,25 +231,71 @@ static struct kobj_type cpuregs_kobj_type = { > > * future expansion without an ABI break. > > */ > > #define kobj_to_cpuinfo(kobj) container_of(kobj, struct cpuinfo_arm64, kobj) > > -#define CPUREGS_ATTR_RO(_name, _field) \ > > +#define CPUREGS_RAW_ATTR_RO(_name, _field) \ > > static ssize_t _name##_show(struct kobject *kobj, \ > > struct kobj_attribute *attr, char *buf) \ > > { \ > > struct cpuinfo_arm64 *info = kobj_to_cpuinfo(kobj); \ > > \ > > - if (info->reg_midr) \ > > - return sprintf(buf, "0x%016x\n", info->reg_##_field); \ > > - else \ > > + if (info->reg_midr) { \ > > + u64 val = info->reg_##_field; \ > > + return sprintf(buf, "0x%016llx\n", val); \ > > Nit, for sysfs, use the new sysfs_emit() call instead of sprintf(). Will do. Thanks! -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef ` (3 preceding siblings ...) 2020-10-21 10:46 ` [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs Qais Yousef @ 2020-10-21 11:26 ` Greg Kroah-Hartman 2020-10-21 13:15 ` Qais Yousef 4 siblings, 1 reply; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-21 11:26 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 11:46:07AM +0100, Qais Yousef wrote: > This series adds basic support for Asymmetric AArch32 systems. Full rationale > is in v1's cover letter. > > https://lore.kernel.org/linux-arch/20201008181641.32767-1-qais.yousef@arm.com/ That is not good, provide full rational in each place, not everyone has web access at all time. Also, you forgot to document this in Documentation/ABI/ like is required for sysfs files, and I thought I asked for last time. > Changes in v2: > > * We now reset vcpu->arch.target to force re-initialized for KVM patch. > (Marc) > > * Fix a bug where this_cpu_has_cap() must be called with preemption > disabled in check_aarch32_cpumask(). > > * Add new sysctl.enable_asym_32bit. (Catalin) > > * Export id_aar64fpr0 register in sysfs which allows user space to > discover which cpus support 32bit@EL0. The sysctl must be enabled for > the user space to discover the asymmetry. (Will/Catalin) > > * Fixing up affinity in the kernel approach was dropped. The support > assumes the user space that wants to enable this support knows how to > guarantee correct affinities for 32bit apps by using cpusets. I asked you to work with Intel to come up with an agreement of how this is going to be represented in userspace. Why did you not do that? Without even looking at the patch set, this is not ok... greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems 2020-10-21 11:26 ` [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Greg Kroah-Hartman @ 2020-10-21 13:15 ` Qais Yousef 2020-10-21 13:31 ` Greg Kroah-Hartman 0 siblings, 1 reply; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:15 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch Hi Greg On 10/21/20 13:26, Greg Kroah-Hartman wrote: > On Wed, Oct 21, 2020 at 11:46:07AM +0100, Qais Yousef wrote: > > This series adds basic support for Asymmetric AArch32 systems. Full rationale > > is in v1's cover letter. > > > > https://lore.kernel.org/linux-arch/20201008181641.32767-1-qais.yousef@arm.com/ > > That is not good, provide full rational in each place, not everyone has > web access at all time. Sorry. I usually copy the whole thing but for the first time I do this as I thought it'd be better to be less wordy. I'll copy the whole thing again next time. > Also, you forgot to document this in Documentation/ABI/ like is required > for sysfs files, and I thought I asked for last time. Last time there was no sysfs info. It's introduced for the first time here. There's still no consensus on which direction to go, that is fix it in the scheduler or let user space handle it all. > > Changes in v2: > > > > * We now reset vcpu->arch.target to force re-initialized for KVM patch. > > (Marc) > > > > * Fix a bug where this_cpu_has_cap() must be called with preemption > > disabled in check_aarch32_cpumask(). > > > > * Add new sysctl.enable_asym_32bit. (Catalin) > > > > * Export id_aar64fpr0 register in sysfs which allows user space to > > discover which cpus support 32bit@EL0. The sysctl must be enabled for > > the user space to discover the asymmetry. (Will/Catalin) > > > > * Fixing up affinity in the kernel approach was dropped. The support > > assumes the user space that wants to enable this support knows how to > > guarantee correct affinities for 32bit apps by using cpusets. > > I asked you to work with Intel to come up with an agreement of how this > is going to be represented in userspace. Why did you not do that? I did chip in to that thread. AFAIU they're doing something completely different and unrelated. Their goal is unclear too. They care about big.LITTLE type of support for Intel and collating already existing information in a different/new place. I don't see the point. I saw they had several similar comments from others. They need to send a new version to see if anything changes. > Without even looking at the patch set, this is not ok... Sorry about that. Please keep in mind we're still debating if we want to support this upstream. And if we do, what shape this should take. My first version highlighted how things could look like if scheduler took care of the problem. Now this RFC tries to highlight how things could look like if we go with pure user space based solution. It's to help maintainers get a better appreciation of what implementation details incurred in either direction. At least that was my intention. I'll improve on the cover letter next time. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems 2020-10-21 13:15 ` Qais Yousef @ 2020-10-21 13:31 ` Greg Kroah-Hartman 2020-10-21 13:55 ` Qais Yousef 2020-10-21 14:35 ` Catalin Marinas 0 siblings, 2 replies; 57+ messages in thread From: Greg Kroah-Hartman @ 2020-10-21 13:31 UTC (permalink / raw) To: Qais Yousef Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 02:15:04PM +0100, Qais Yousef wrote: > > Without even looking at the patch set, this is not ok... > > Sorry about that. Please keep in mind we're still debating if we want to > support this upstream. What do you mean by this? Do you mean you will keep an out-of-tree patchset for all time for all future kernel versions for everyone to pull from and you will support for all chips that end up with this type of functionality? That's a huge task to do, you all must have a lot of money to burn! It is a "trivial cost" to get changes merged upstream compared to the amount of time and money it costs to keep stuff out of the tree. Why you would not want to do this is beyond me. But hey, I'm not in charge of your company's budget, for good reasons :) good luck! greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems 2020-10-21 13:31 ` Greg Kroah-Hartman @ 2020-10-21 13:55 ` Qais Yousef 2020-10-21 14:35 ` Catalin Marinas 1 sibling, 0 replies; 57+ messages in thread From: Qais Yousef @ 2020-10-21 13:55 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On 10/21/20 15:31, Greg Kroah-Hartman wrote: > On Wed, Oct 21, 2020 at 02:15:04PM +0100, Qais Yousef wrote: > > > Without even looking at the patch set, this is not ok... > > > > Sorry about that. Please keep in mind we're still debating if we want to > > support this upstream. > > What do you mean by this? Err. I meant that we don't know how and if upstream would like to support this. The patches are on the list to discuss what it takes for inclusion. Like all other patches, it could be accepted or rejected. I am working on it to be accepted *of course* but it's not my decision. I just can't assume or take it for granted this will go in. I didn't see a straight no yet, so hopefully this is moving in the right direction. My point is there's still more discussion to be had and what presented in this RFC could change completely. Thanks -- Qais Yousef > Do you mean you will keep an out-of-tree patchset for all time for all > future kernel versions for everyone to pull from and you will support > for all chips that end up with this type of functionality? > > That's a huge task to do, you all must have a lot of money to burn! > > It is a "trivial cost" to get changes merged upstream compared to the > amount of time and money it costs to keep stuff out of the tree. Why > you would not want to do this is beyond me. > > But hey, I'm not in charge of your company's budget, for good reasons :) > > good luck! > > greg k-h ^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems 2020-10-21 13:31 ` Greg Kroah-Hartman 2020-10-21 13:55 ` Qais Yousef @ 2020-10-21 14:35 ` Catalin Marinas 1 sibling, 0 replies; 57+ messages in thread From: Catalin Marinas @ 2020-10-21 14:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Qais Yousef, Will Deacon, Marc Zyngier, Peter Zijlstra (Intel), Morten Rasmussen, Linus Torvalds, James Morse, linux-arm-kernel, linux-arch On Wed, Oct 21, 2020 at 03:31:05PM +0200, Greg Kroah-Hartman wrote: > On Wed, Oct 21, 2020 at 02:15:04PM +0100, Qais Yousef wrote: > > > Without even looking at the patch set, this is not ok... > > > > Sorry about that. Please keep in mind we're still debating if we want to > > support this upstream. > > What do you mean by this? > > Do you mean you will keep an out-of-tree patchset for all time for all > future kernel versions for everyone to pull from and you will support > for all chips that end up with this type of functionality? > > That's a huge task to do, you all must have a lot of money to burn! From a maintainer perspective (and not as an Arm employee), hopefully the high cost would be a deterrent to other such hardware combinations in the future ;). Even if it will (likely) end up in mainline, we shouldn't make it straightforward. -- Catalin ^ permalink raw reply [flat|nested] 57+ messages in thread
end of thread, other threads:[~2020-11-02 17:59 UTC | newest] Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-21 10:46 [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 1/4] arm64: kvm: Handle " Qais Yousef 2020-10-21 12:02 ` Marc Zyngier 2020-10-21 13:35 ` Qais Yousef 2020-10-21 13:51 ` Marc Zyngier 2020-10-21 14:38 ` Qais Yousef 2020-11-02 17:58 ` Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 2/4] arm64: Add support for asymmetric AArch32 EL0 configurations Qais Yousef 2020-10-21 15:39 ` Will Deacon 2020-10-21 16:21 ` Qais Yousef 2020-10-21 16:52 ` Catalin Marinas 2020-10-21 17:39 ` Will Deacon 2020-10-22 9:53 ` Catalin Marinas 2020-10-21 10:46 ` [RFC PATCH v2 3/4] arm64: export emulate_sys_reg() Qais Yousef 2020-10-21 10:46 ` [RFC PATCH v2 4/4] arm64: Export id_aar64fpr0 via sysfs Qais Yousef 2020-10-21 11:09 ` Marc Zyngier 2020-10-21 11:25 ` Greg Kroah-Hartman 2020-10-21 11:46 ` Marc Zyngier 2020-10-21 12:11 ` Greg Kroah-Hartman 2020-10-21 13:18 ` Qais Yousef 2020-10-21 12:15 ` Catalin Marinas 2020-10-21 13:20 ` Qais Yousef 2020-10-21 13:33 ` Morten Rasmussen 2020-10-21 14:09 ` Catalin Marinas 2020-10-21 14:41 ` Morten Rasmussen 2020-10-21 14:45 ` Will Deacon 2020-10-21 15:10 ` Catalin Marinas 2020-10-21 15:37 ` Will Deacon 2020-10-21 16:18 ` Catalin Marinas 2020-10-21 17:19 ` Will Deacon 2020-10-22 9:55 ` Morten Rasmussen 2020-10-21 14:31 ` Qais Yousef 2020-10-22 10:16 ` Morten Rasmussen 2020-10-22 10:48 ` Qais Yousef 2020-10-21 14:41 ` Will Deacon 2020-10-21 15:03 ` Qais Yousef 2020-10-21 15:23 ` Will Deacon 2020-10-21 16:07 ` Qais Yousef 2020-10-21 17:23 ` Will Deacon 2020-10-21 19:57 ` Qais Yousef 2020-10-21 20:26 ` Will Deacon 2020-10-22 8:16 ` Catalin Marinas 2020-10-22 9:58 ` Qais Yousef 2020-10-22 13:47 ` Qais Yousef 2020-10-22 13:55 ` Greg Kroah-Hartman 2020-10-22 14:31 ` Catalin Marinas 2020-10-22 14:34 ` Qais Yousef 2020-10-26 19:02 ` Qais Yousef 2020-10-26 19:08 ` Greg Kroah-Hartman 2020-10-26 19:18 ` Qais Yousef 2020-10-21 11:28 ` Greg Kroah-Hartman 2020-10-21 13:22 ` Qais Yousef 2020-10-21 11:26 ` [RFC PATCH v2 0/4] Add support for Asymmetric AArch32 systems Greg Kroah-Hartman 2020-10-21 13:15 ` Qais Yousef 2020-10-21 13:31 ` Greg Kroah-Hartman 2020-10-21 13:55 ` Qais Yousef 2020-10-21 14:35 ` Catalin Marinas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).