* Re: [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation
@ 2018-02-21 14:39 ` Mark Rutland
0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2018-02-21 14:39 UTC (permalink / raw)
To: Dave Martin
Cc: Kees Cook, Catalin Marinas, Suzuki K Poulose, Will Deacon,
linux-kernel, linux-arm-kernel
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>
> 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.
This also looks fine to me, so FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation
2018-02-21 14:39 ` Mark Rutland
@ 2018-02-21 16:26 ` Dave Martin
-1 siblings, 0 replies; 10+ messages in thread
From: Dave Martin @ 2018-02-21 16:26 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Feb 21, 2018 at 02:39:55PM +0000, Mark Rutland 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?
Can do, though I still think it's a bit dubious. The original message
as it stands isn't wrong. I won't lose sleep over it though.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation
@ 2018-02-21 16:26 ` Dave Martin
0 siblings, 0 replies; 10+ messages in thread
From: Dave Martin @ 2018-02-21 16:26 UTC (permalink / raw)
To: Mark Rutland
Cc: Kees Cook, Suzuki K Poulose, Catalin Marinas, Will Deacon,
linux-kernel, linux-arm-kernel
On Wed, Feb 21, 2018 at 02:39:55PM +0000, Mark Rutland 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?
Can do, though I still think it's a bit dubious. The original message
as it stands isn't wrong. I won't lose sleep over it though.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation
2018-02-21 14:39 ` Mark Rutland
@ 2018-02-21 18:04 ` Kees Cook
-1 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-02-21 18:04 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm64: cpufeature: Trim feature reporting and include PAN emulation
@ 2018-02-21 18:04 ` Kees Cook
0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-02-21 18:04 UTC (permalink / raw)
To: Mark Rutland
Cc: Dave Martin, Catalin Marinas, Suzuki K Poulose, Will Deacon, LKML,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread