| Message ID | 20231017052322.1211099-4-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: cpufeatures: Display enabled cores | expand |
On Tue, 17 Oct 2023 06:23:22 +0100, Jeremy Linton <jeremy.linton@arm.com> wrote: > > Now that we have the ability to display the list of cores > with a feature when it is selectivly enabled, lets display the > cores enabled for 32-bit use at EL0. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index b7b67bac0e60..512cbe446b41 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) > if (!has_cpuid_feature(entry, scope)) > return allow_mismatched_32bit_el0; > > - if (scope == SCOPE_SYSTEM) > - pr_info("detected: 32-bit EL0 Support\n"); > + if (scope == SCOPE_SYSTEM) { > + struct arm64_cpu_capabilities *has_32bit; > + > + has_32bit = (struct arm64_cpu_capabilities *)entry; > + > + has_32bit->cpus = system_32bit_el0_cpumask(); This seems rather dodgy. 'entry' comes from a static const array which will, in all likelihood be mapped R/O pretty soon after the initial CPU bringup. Try offlining/onlining a CPU and you should see a firework similar to what I have below (I hacked the CnP property, but that's no different from what you are doing): bash-5.1# echo 0 >/sys/devices/system/cpu/cpu2/online [ 51.367137] psci: CPU2 killed (polled 0 ms) bash-5.1# echo 1 >/sys/devices/system/cpu/cpu2/online [ 56.703092] Unable to handle kernel write to read-only memory at virtual address ffff800080d63db0 [ 56.703494] Mem abort info: [ 56.703627] ESR = 0x000000009600004f [ 56.703799] EC = 0x25: DABT (current EL), IL = 32 bits [ 56.704052] SET = 0, FnV = 0 [ 56.704196] EA = 0, S1PTW = 0 [ 56.704340] FSC = 0x0f: level 3 permission fault [ 56.704575] Data abort info: [ 56.704715] ISV = 0, ISS = 0x0000004f, ISS2 = 0x00000000 [ 56.704961] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 [ 56.705195] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 56.705435] swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000008142e000 [ 56.705742] [ffff800080d63db0] pgd=10000000dbfff003, p4d=10000000dbfff003, pud=10000000dbffe003, pmd=10000000dbffc003, pte=0060000080d63783 [ 56.706310] Internal error: Oops: 000000009600004f [#1] PREEMPT SMP [ 56.706591] Modules linked in: [ 56.706741] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.6.0-rc1-00007-g805cc7d30885-dirty #1702 [ 56.707145] Hardware name: linux,dummy-virt (DT) [ 56.707359] pstate: 800001c5 (Nzcv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 56.707676] pc : cpu_enable_cnp+0x24/0x60 [ 56.707873] lr : verify_local_cpu_caps+0x6c/0x140 [ 56.708046] sp : ffff800082103e10 [ 56.708201] x29: ffff800082103e10 x28: 0000000000000000 x27: 0000000000000000 [ 56.708517] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000 [ 56.708837] x23: ffff800081eec540 x22: 0000000000000003 x21: ffff8000813945f0 [ 56.709166] x20: ffff800081394358 x19: ffff800080d63db0 x18: 0000000000000000 [ 56.709488] x17: 000000040044ffff x16: 00500073b5503510 x15: 0000000000000000 [ 56.709805] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [ 56.710100] x11: 0000000000000000 x10: 0000000000000000 x9 : ffff8000800273ac [ 56.710416] x8 : ffff80008003b640 x7 : 0000000000000000 x6 : 0000000000000000 [ 56.710738] x5 : 0000000000000000 x4 : ffff800081394000 x3 : ffff800081394000 [ 56.711070] x2 : ffff800081241ef8 x1 : ffff800080027218 x0 : ffff800080d63db0 [ 56.711396] Call trace: [ 56.711512] cpu_enable_cnp+0x24/0x60 [ 56.711684] verify_local_cpu_caps+0x6c/0x140 [ 56.711885] verify_local_cpu_capabilities+0x1c/0x198 [ 56.712112] check_local_cpu_capabilities+0x28/0x50 [ 56.712337] secondary_start_kernel+0xb4/0x1a0 [ 56.712546] __secondary_switched+0xb8/0xc0 [ 56.712739] Code: 910003fd b0009b63 d00090c2 913be042 (f9000002) [ 56.713013] ---[ end trace 0000000000000000 ]--- [ 56.713224] Kernel panic - not syncing: Attempted to kill the idle task! [ 56.713518] SMP: stopping secondary CPUs [ 56.713752] Kernel Offset: disabled [ 56.713908] CPU features: 0x00000000,50024d43,1947f723 [ 56.714144] Memory Limit: none [ 56.714292] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]--- Thanks, M.
Hi, On 10/17/23 13:01, Marc Zyngier wrote: > On Tue, 17 Oct 2023 06:23:22 +0100, > Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> Now that we have the ability to display the list of cores >> with a feature when it is selectivly enabled, lets display the >> cores enabled for 32-bit use at EL0. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index b7b67bac0e60..512cbe446b41 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) >> if (!has_cpuid_feature(entry, scope)) >> return allow_mismatched_32bit_el0; >> >> - if (scope == SCOPE_SYSTEM) >> - pr_info("detected: 32-bit EL0 Support\n"); >> + if (scope == SCOPE_SYSTEM) { >> + struct arm64_cpu_capabilities *has_32bit; >> + >> + has_32bit = (struct arm64_cpu_capabilities *)entry; >> + >> + has_32bit->cpus = system_32bit_el0_cpumask(); > > This seems rather dodgy. 'entry' comes from a static const array which > will, in all likelihood be mapped R/O pretty soon after the initial > CPU bringup. Try offlining/onlining a CPU and you should see a > firework similar to what I have below (I hacked the CnP property, but > that's no different from what you are doing): Yes, dodgy is a good word. The other choices, maintain a mask just for the print or dump the static key and always use the cpu_32bit_el0_mask or some combination, weren't much better in the "ick" category. If anyone sees a better way I'm open to suggestion, although simply dropping this last patch is fine too. > > bash-5.1# echo 0 >/sys/devices/system/cpu/cpu2/online > [ 51.367137] psci: CPU2 killed (polled 0 ms) > bash-5.1# echo 1 >/sys/devices/system/cpu/cpu2/online > [ 56.703092] Unable to handle kernel write to read-only memory at virtual address ffff800080d63db0 > [ 56.703494] Mem abort info: > [ 56.703627] ESR = 0x000000009600004f > [ 56.703799] EC = 0x25: DABT (current EL), IL = 32 bits > [ 56.704052] SET = 0, FnV = 0 > [ 56.704196] EA = 0, S1PTW = 0 > [ 56.704340] FSC = 0x0f: level 3 permission fault > [ 56.704575] Data abort info: > [ 56.704715] ISV = 0, ISS = 0x0000004f, ISS2 = 0x00000000 > [ 56.704961] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 > [ 56.705195] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [ 56.705435] swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000008142e000 > [ 56.705742] [ffff800080d63db0] pgd=10000000dbfff003, p4d=10000000dbfff003, pud=10000000dbffe003, pmd=10000000dbffc003, pte=0060000080d63783 > [ 56.706310] Internal error: Oops: 000000009600004f [#1] PREEMPT SMP > [ 56.706591] Modules linked in: > [ 56.706741] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.6.0-rc1-00007-g805cc7d30885-dirty #1702 > [ 56.707145] Hardware name: linux,dummy-virt (DT) > [ 56.707359] pstate: 800001c5 (Nzcv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 56.707676] pc : cpu_enable_cnp+0x24/0x60 > [ 56.707873] lr : verify_local_cpu_caps+0x6c/0x140 > [ 56.708046] sp : ffff800082103e10 > [ 56.708201] x29: ffff800082103e10 x28: 0000000000000000 x27: 0000000000000000 > [ 56.708517] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000 > [ 56.708837] x23: ffff800081eec540 x22: 0000000000000003 x21: ffff8000813945f0 > [ 56.709166] x20: ffff800081394358 x19: ffff800080d63db0 x18: 0000000000000000 > [ 56.709488] x17: 000000040044ffff x16: 00500073b5503510 x15: 0000000000000000 > [ 56.709805] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 > [ 56.710100] x11: 0000000000000000 x10: 0000000000000000 x9 : ffff8000800273ac > [ 56.710416] x8 : ffff80008003b640 x7 : 0000000000000000 x6 : 0000000000000000 > [ 56.710738] x5 : 0000000000000000 x4 : ffff800081394000 x3 : ffff800081394000 > [ 56.711070] x2 : ffff800081241ef8 x1 : ffff800080027218 x0 : ffff800080d63db0 > [ 56.711396] Call trace: > [ 56.711512] cpu_enable_cnp+0x24/0x60 > [ 56.711684] verify_local_cpu_caps+0x6c/0x140 > [ 56.711885] verify_local_cpu_capabilities+0x1c/0x198 > [ 56.712112] check_local_cpu_capabilities+0x28/0x50 > [ 56.712337] secondary_start_kernel+0xb4/0x1a0 > [ 56.712546] __secondary_switched+0xb8/0xc0 > [ 56.712739] Code: 910003fd b0009b63 d00090c2 913be042 (f9000002) > [ 56.713013] ---[ end trace 0000000000000000 ]--- > [ 56.713224] Kernel panic - not syncing: Attempted to kill the idle task! > [ 56.713518] SMP: stopping secondary CPUs > [ 56.713752] Kernel Offset: disabled > [ 56.713908] CPU features: 0x00000000,50024d43,1947f723 > [ 56.714144] Memory Limit: none > [ 56.714292] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]--- > > Thanks, > > M. >
On Tue, 17 Oct 2023 20:15:43 +0100, Jeremy Linton <jeremy.linton@arm.com> wrote: > > Hi, > > On 10/17/23 13:01, Marc Zyngier wrote: > > On Tue, 17 Oct 2023 06:23:22 +0100, > > Jeremy Linton <jeremy.linton@arm.com> wrote: > >> > >> Now that we have the ability to display the list of cores > >> with a feature when it is selectivly enabled, lets display the > >> cores enabled for 32-bit use at EL0. > >> > >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > >> --- > >> arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- > >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > >> index b7b67bac0e60..512cbe446b41 100644 > >> --- a/arch/arm64/kernel/cpufeature.c > >> +++ b/arch/arm64/kernel/cpufeature.c > >> @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) > >> if (!has_cpuid_feature(entry, scope)) > >> return allow_mismatched_32bit_el0; > >> - if (scope == SCOPE_SYSTEM) > >> - pr_info("detected: 32-bit EL0 Support\n"); > >> + if (scope == SCOPE_SYSTEM) { > >> + struct arm64_cpu_capabilities *has_32bit; > >> + > >> + has_32bit = (struct arm64_cpu_capabilities *)entry; > >> + > >> + has_32bit->cpus = system_32bit_el0_cpumask(); > > > > This seems rather dodgy. 'entry' comes from a static const array which > > will, in all likelihood be mapped R/O pretty soon after the initial > > CPU bringup. Try offlining/onlining a CPU and you should see a > > firework similar to what I have below (I hacked the CnP property, but > > that's no different from what you are doing): > > Yes, dodgy is a good word. The other choices, maintain a mask just for > the print or dump the static key and always use the cpu_32bit_el0_mask > or some combination, weren't much better in the "ick" category. If > anyone sees a better way I'm open to suggestion, although simply > dropping this last patch is fine too. An obvious choice would be to replace the 'cpus' cpumask with a function that evaluates a cpumask stored separately. M.
On 10/18/23 07:43, Marc Zyngier wrote: > On Tue, 17 Oct 2023 20:15:43 +0100, > Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> Hi, >> >> On 10/17/23 13:01, Marc Zyngier wrote: >>> On Tue, 17 Oct 2023 06:23:22 +0100, >>> Jeremy Linton <jeremy.linton@arm.com> wrote: >>>> >>>> Now that we have the ability to display the list of cores >>>> with a feature when it is selectivly enabled, lets display the >>>> cores enabled for 32-bit use at EL0. >>>> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> --- >>>> arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- >>>> 1 file changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >>>> index b7b67bac0e60..512cbe446b41 100644 >>>> --- a/arch/arm64/kernel/cpufeature.c >>>> +++ b/arch/arm64/kernel/cpufeature.c >>>> @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) >>>> if (!has_cpuid_feature(entry, scope)) >>>> return allow_mismatched_32bit_el0; >>>> - if (scope == SCOPE_SYSTEM) >>>> - pr_info("detected: 32-bit EL0 Support\n"); >>>> + if (scope == SCOPE_SYSTEM) { >>>> + struct arm64_cpu_capabilities *has_32bit; >>>> + >>>> + has_32bit = (struct arm64_cpu_capabilities *)entry; >>>> + >>>> + has_32bit->cpus = system_32bit_el0_cpumask(); >>> >>> This seems rather dodgy. 'entry' comes from a static const array which >>> will, in all likelihood be mapped R/O pretty soon after the initial >>> CPU bringup. Try offlining/onlining a CPU and you should see a >>> firework similar to what I have below (I hacked the CnP property, but >>> that's no different from what you are doing): >> >> Yes, dodgy is a good word. The other choices, maintain a mask just for >> the print or dump the static key and always use the cpu_32bit_el0_mask >> or some combination, weren't much better in the "ick" category. If >> anyone sees a better way I'm open to suggestion, although simply >> dropping this last patch is fine too. > > An obvious choice would be to replace the 'cpus' cpumask with a > function that evaluates a cpumask stored separately. Right, I was too busy trying to cleanup the 32-bit mask, that works too.
On 2023-10-18 16:42, Jeremy Linton wrote: > On 10/18/23 07:43, Marc Zyngier wrote: >> On Tue, 17 Oct 2023 20:15:43 +0100, >> Jeremy Linton <jeremy.linton@arm.com> wrote: >>> >>> Hi, >>> >>> On 10/17/23 13:01, Marc Zyngier wrote: >>>> On Tue, 17 Oct 2023 06:23:22 +0100, >>>> Jeremy Linton <jeremy.linton@arm.com> wrote: >>>>> >>>>> Now that we have the ability to display the list of cores >>>>> with a feature when it is selectivly enabled, lets display the >>>>> cores enabled for 32-bit use at EL0. >>>>> >>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>>> --- >>>>> arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- >>>>> 1 file changed, 13 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/cpufeature.c >>>>> b/arch/arm64/kernel/cpufeature.c >>>>> index b7b67bac0e60..512cbe446b41 100644 >>>>> --- a/arch/arm64/kernel/cpufeature.c >>>>> +++ b/arch/arm64/kernel/cpufeature.c >>>>> @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct >>>>> arm64_cpu_capabilities *entry, int scope) >>>>> if (!has_cpuid_feature(entry, scope)) >>>>> return allow_mismatched_32bit_el0; >>>>> - if (scope == SCOPE_SYSTEM) >>>>> - pr_info("detected: 32-bit EL0 Support\n"); >>>>> + if (scope == SCOPE_SYSTEM) { >>>>> + struct arm64_cpu_capabilities *has_32bit; >>>>> + >>>>> + has_32bit = (struct arm64_cpu_capabilities *)entry; >>>>> + >>>>> + has_32bit->cpus = system_32bit_el0_cpumask(); >>>> >>>> This seems rather dodgy. 'entry' comes from a static const array which >>>> will, in all likelihood be mapped R/O pretty soon after the initial >>>> CPU bringup. Try offlining/onlining a CPU and you should see a >>>> firework similar to what I have below (I hacked the CnP property, but >>>> that's no different from what you are doing): >>> >>> Yes, dodgy is a good word. The other choices, maintain a mask just for >>> the print or dump the static key and always use the cpu_32bit_el0_mask >>> or some combination, weren't much better in the "ick" category. If >>> anyone sees a better way I'm open to suggestion, although simply >>> dropping this last patch is fine too. >> >> An obvious choice would be to replace the 'cpus' cpumask with a >> function that evaluates a cpumask stored separately. > > Right, I was too busy trying to cleanup the 32-bit mask, that works too. But if a cpumask does already exist, wouldn't the *most* logical thing be to use it? If it is somehow harmful to the existing machinery to set all of the CPUs in cpu_32bit_el0_mask if all of the CPUs support 32-bit EL0, that seems like a mind-bogglingly dumb design which deserves fixing in its own right. To me the obvious thing would be to allocate and populate cpu_32bit_el0_mask unconditionally so the initial feature detection can work as normal, and then if anyone's really really bothered, free it again later if we determine there's no mismatch. Thanks, Robin.
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index b7b67bac0e60..512cbe446b41 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1533,8 +1533,17 @@ static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) if (!has_cpuid_feature(entry, scope)) return allow_mismatched_32bit_el0; - if (scope == SCOPE_SYSTEM) - pr_info("detected: 32-bit EL0 Support\n"); + if (scope == SCOPE_SYSTEM) { + struct arm64_cpu_capabilities *has_32bit; + + has_32bit = (struct arm64_cpu_capabilities *)entry; + + has_32bit->cpus = system_32bit_el0_cpumask(); + if (has_32bit->cpus == cpu_possible_mask) + has_32bit->cpus = cpu_online_mask; + if (has_32bit->cpus == cpu_none_mask) + has_32bit->cpus = NULL; + } return true; } @@ -2307,9 +2316,11 @@ static const struct arm64_cpu_capabilities arm64_features[] = { ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, NV, IMP) }, { + .desc = "32-bit EL0 Support", .capability = ARM64_HAS_32BIT_EL0_DO_NOT_USE, .type = ARM64_CPUCAP_SYSTEM_FEATURE, .matches = has_32bit_el0, + .cpus = cpu_none_mask, ARM64_CPUID_FIELDS(ID_AA64PFR0_EL1, EL0, AARCH32) }, #ifdef CONFIG_KVM
Now that we have the ability to display the list of cores with a feature when it is selectivly enabled, lets display the cores enabled for 32-bit use at EL0. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/kernel/cpufeature.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)