Messages in this thread | | | From | Kees Cook <> | Date | Wed, 21 Feb 2018 10:04:47 -0800 | Subject | Re: [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation |
| |
On Wed, Feb 21, 2018 at 6:39 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Feb 21, 2018 at 11:18:27AM +0000, Dave Martin wrote: >> On Tue, Feb 20, 2018 at 02:46:24PM -0800, Kees Cook wrote: >> > The PAN emulation notification was only happening for non-boot CPUs >> > if CPU capabilities had already been configured. This seems to be the >> > wrong place, as it's system-wide and isn't attached to capabilities, >> > so its reporting didn't normally happen. Instead, report it once from >> > the boot CPU. Additionally removes the redundant "feature" word from the >> > "CPU features:" line. >> > >> > Before (redundant "feature", and missing PAN emulation report): >> > >> > SMP: Total of 4 processors activated. >> > CPU features: detected feature: 32-bit EL0 Support >> > CPU features: detected feature: Kernel page table isolation (KPTI) >> > CPU: All CPU(s) started at EL2 >> > >> > After: >> > >> > SMP: Total of 4 processors activated. >> > CPU features: detected: 32-bit EL0 Support >> > CPU features: detected: Kernel page table isolation (KPTI) >> > CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching >> > CPU: All CPU(s) started at EL2 >> > >> > Signed-off-by: Kees Cook <keescook@chromium.org> >> > --- >> > arch/arm64/kernel/cpufeature.c | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> > >> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> > index 29b1f873e337..6c799ca58b53 100644 >> > --- a/arch/arm64/kernel/cpufeature.c >> > +++ b/arch/arm64/kernel/cpufeature.c >> > @@ -1333,9 +1333,6 @@ static void verify_local_cpu_capabilities(void) >> > >> > if (system_supports_sve()) >> > verify_sve_features(); >> > - >> > - if (system_uses_ttbr0_pan()) >> > - pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n"); >> > } >> > >> > void check_local_cpu_capabilities(void) >> > @@ -1360,7 +1357,7 @@ void check_local_cpu_capabilities(void) >> > >> > static void __init setup_feature_capabilities(void) >> > { >> > - update_cpu_capabilities(arm64_features, "detected feature:"); >> > + update_cpu_capabilities(arm64_features, "detected:"); >> >> Although I get what you're saying about redundant use of the word >> "features", this feels like cosmetic churn that is unrelated to the >> problem this patch is addressing. > > Given it seems sensible, shall we just split that into a separate patch? > > FWIW, for a patch with just this change: > > Acked-by: Mark Rutland <mark.rutland@arm.com>
Sure!
>> It could be worth reviewing the CPU errata messages and other >> miscellaneous printks together to make them less verbose and more >> consistent all in one go, but that would be a separate patch... >> >> > enable_cpu_capabilities(arm64_features); >> > } >> > >> > @@ -1394,6 +1391,9 @@ void __init setup_cpu_features(void) >> > if (system_supports_32bit_el0()) >> > setup_elf_hwcaps(compat_elf_hwcaps); >> > >> > + if (system_uses_ttbr0_pan()) >> > + pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n"); >> > + >> >> Moving this seems sensible. The other option would be to paste it into >> update_cpu_capabilities(), but the message would still potentially get >> printed multiple times, so that doesn't feel like the right approach. > > I think that more ideally, we'd give this an entry in the arm64_features array, > but because it's effectively a negative feature, it's a little tricky.
Yeah, I looked at that but it seemed like it would needlessly burn a capability bit for no real gain.
> This also looks fine to me, so FWIW: > > Acked-by: Mark Rutland <mark.rutland@arm.com>
Thanks, I'll split the patches!
-Kees
-- Kees Cook Pixel Security
| |