Re: [PATCH] clarify how insecure CPU is
On Sun, Mar 04, 2018 at 03:01:48PM +0100, Pavel Machek wrote: > > Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is > > affected". > > That's not what Thomas was explaining to me. It is in the comment he pasted: * Check whether the machine is affected by erratum 400. This is * used to select the proper idle routine and to enable the check * whether the machine is affected in arch_post_acpi_init(), which * sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check. > So.. what's magical about it, why do we need two bits, and why is that > not explained in the header file? Lemme enable line numbers so that you can find it: arch/x86/include/asm/cpufeatures.h: 19 /* 20 * Note: If the comment begins with a quoted string, that string is used 21 * in /proc/cpuinfo instead of the macro name. If the string is "", 22 * this feature bit is not displayed in /proc/cpuinfo at all. > Please go through the email thread, No, you read Thomas' mail again. > I'm trying to understand what is going on here, Nothing's going on, it works as designed. X86_BUG_AMD_E400 marks all CPUs which could be affected by erratum 400 and X86_BUG_AMD_APIC_C1E is the bit we set when we detect that the CPU is *actually* affected because we need to do the detection late, after ACPI has been initialized. A CPU might be affected by the erratum - bit X86_BUG_AMD_E400 - but if the BIOS doesn't enter C1E, then the erratum doesn't come to manifest itself, i.e., we don't set X86_BUG_AMD_APIC_C1E. If it is still not clear, read the erratum 400 description in the revision guide. The code works perfectly fine. Unless you're experiencing a problem with it. Then I'm all ears. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [PATCH] clarify how insecure CPU is
On Sun, Mar 04, 2018 at 03:01:48PM +0100, Pavel Machek wrote: > > Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is > > affected". > > That's not what Thomas was explaining to me. It is in the comment he pasted: * Check whether the machine is affected by erratum 400. This is * used to select the proper idle routine and to enable the check * whether the machine is affected in arch_post_acpi_init(), which * sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check. > So.. what's magical about it, why do we need two bits, and why is that > not explained in the header file? Lemme enable line numbers so that you can find it: arch/x86/include/asm/cpufeatures.h: 19 /* 20 * Note: If the comment begins with a quoted string, that string is used 21 * in /proc/cpuinfo instead of the macro name. If the string is "", 22 * this feature bit is not displayed in /proc/cpuinfo at all. > Please go through the email thread, No, you read Thomas' mail again. > I'm trying to understand what is going on here, Nothing's going on, it works as designed. X86_BUG_AMD_E400 marks all CPUs which could be affected by erratum 400 and X86_BUG_AMD_APIC_C1E is the bit we set when we detect that the CPU is *actually* affected because we need to do the detection late, after ACPI has been initialized. A CPU might be affected by the erratum - bit X86_BUG_AMD_E400 - but if the BIOS doesn't enter C1E, then the erratum doesn't come to manifest itself, i.e., we don't set X86_BUG_AMD_APIC_C1E. If it is still not clear, read the erratum 400 description in the revision guide. The code works perfectly fine. Unless you're experiencing a problem with it. Then I'm all ears. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [PATCH] clarify how insecure CPU is
On Sun 2018-03-04 10:29:18, Borislav Petkov wrote: > On Sun, Mar 04, 2018 at 09:51:59AM +0100, Pavel Machek wrote: > > diff --git a/arch/x86/include/asm/cpufeatures.h > > b/arch/x86/include/asm/cpufeatures.h > > index f41079d..4901742 100644 > > --- a/arch/x86/include/asm/cpufeatures.h > > +++ b/arch/x86/include/asm/cpufeatures.h > > @@ -341,7 +341,7 @@ > > #define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */ > > #define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */ > > #define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD > > Erratum 383 */ > > -#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD > > Erratum 400 */ > > +#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is > > affected AMD Erratum 400, special idle routine is needed */ > > #define X86_BUG_11AP X86_BUG(5) /* Bad local APIC > > aka 11AP */ > > #define X86_BUG_FXSAVE_LEAKX86_BUG(6) /* FXSAVE leaks > > FOP/FIP/FOP */ > > #define X86_BUG_CLFLUSH_MONITORX86_BUG(7) /* AAI65, CLFLUSH > > required before MONITOR */ > > @@ -356,7 +356,7 @@ > > #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector > > preserves the base */ > > #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without > > input dep on GS */ > > #define X86_BUG_MONITORX86_BUG(12) /* IPI required to > > wake up remote CPU */ > > -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the > > affected by Erratum 400 */ > > +#define X86_BUG_AMD_E400 X86_BUG(13) /* System may be affected > > by Erratum 400, X86_BUG_AMD_APIC_C1E might be needed */ > > Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is > affected". That's not what Thomas was explaining to me. > And then you don't need the above comment change. And you can't remove > "apic_c1e" there because it is magical. So.. what's magical about it, why do we need two bits, and why is that not explained in the header file? Please go through the email thread, I'm trying to understand what is going on here, and no, the comments in the header are not helpful. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Sun 2018-03-04 10:29:18, Borislav Petkov wrote: > On Sun, Mar 04, 2018 at 09:51:59AM +0100, Pavel Machek wrote: > > diff --git a/arch/x86/include/asm/cpufeatures.h > > b/arch/x86/include/asm/cpufeatures.h > > index f41079d..4901742 100644 > > --- a/arch/x86/include/asm/cpufeatures.h > > +++ b/arch/x86/include/asm/cpufeatures.h > > @@ -341,7 +341,7 @@ > > #define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */ > > #define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */ > > #define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD > > Erratum 383 */ > > -#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD > > Erratum 400 */ > > +#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is > > affected AMD Erratum 400, special idle routine is needed */ > > #define X86_BUG_11AP X86_BUG(5) /* Bad local APIC > > aka 11AP */ > > #define X86_BUG_FXSAVE_LEAKX86_BUG(6) /* FXSAVE leaks > > FOP/FIP/FOP */ > > #define X86_BUG_CLFLUSH_MONITORX86_BUG(7) /* AAI65, CLFLUSH > > required before MONITOR */ > > @@ -356,7 +356,7 @@ > > #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector > > preserves the base */ > > #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without > > input dep on GS */ > > #define X86_BUG_MONITORX86_BUG(12) /* IPI required to > > wake up remote CPU */ > > -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the > > affected by Erratum 400 */ > > +#define X86_BUG_AMD_E400 X86_BUG(13) /* System may be affected > > by Erratum 400, X86_BUG_AMD_APIC_C1E might be needed */ > > Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is > affected". That's not what Thomas was explaining to me. > And then you don't need the above comment change. And you can't remove > "apic_c1e" there because it is magical. So.. what's magical about it, why do we need two bits, and why is that not explained in the header file? Please go through the email thread, I'm trying to understand what is going on here, and no, the comments in the header are not helpful. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Sun, Mar 04, 2018 at 09:51:59AM +0100, Pavel Machek wrote: > diff --git a/arch/x86/include/asm/cpufeatures.h > b/arch/x86/include/asm/cpufeatures.h > index f41079d..4901742 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -341,7 +341,7 @@ > #define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */ > #define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */ > #define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD > Erratum 383 */ > -#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD Erratum > 400 */ > +#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is affected AMD > Erratum 400, special idle routine is needed */ > #define X86_BUG_11AP X86_BUG(5) /* Bad local APIC aka 11AP */ > #define X86_BUG_FXSAVE_LEAK X86_BUG(6) /* FXSAVE leaks FOP/FIP/FOP > */ > #define X86_BUG_CLFLUSH_MONITOR X86_BUG(7) /* AAI65, CLFLUSH > required before MONITOR */ > @@ -356,7 +356,7 @@ > #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector > preserves the base */ > #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep > on GS */ > #define X86_BUG_MONITOR X86_BUG(12) /* IPI required to > wake up remote CPU */ > -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the > affected by Erratum 400 */ > +#define X86_BUG_AMD_E400 X86_BUG(13) /* System may be affected > by Erratum 400, X86_BUG_AMD_APIC_C1E might be needed */ Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is affected". And then you don't need the above comment change. And you can't remove "apic_c1e" there because it is magical. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [PATCH] clarify how insecure CPU is
On Sun, Mar 04, 2018 at 09:51:59AM +0100, Pavel Machek wrote: > diff --git a/arch/x86/include/asm/cpufeatures.h > b/arch/x86/include/asm/cpufeatures.h > index f41079d..4901742 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -341,7 +341,7 @@ > #define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */ > #define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */ > #define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD > Erratum 383 */ > -#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD Erratum > 400 */ > +#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is affected AMD > Erratum 400, special idle routine is needed */ > #define X86_BUG_11AP X86_BUG(5) /* Bad local APIC aka 11AP */ > #define X86_BUG_FXSAVE_LEAK X86_BUG(6) /* FXSAVE leaks FOP/FIP/FOP > */ > #define X86_BUG_CLFLUSH_MONITOR X86_BUG(7) /* AAI65, CLFLUSH > required before MONITOR */ > @@ -356,7 +356,7 @@ > #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector > preserves the base */ > #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep > on GS */ > #define X86_BUG_MONITOR X86_BUG(12) /* IPI required to > wake up remote CPU */ > -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the > affected by Erratum 400 */ > +#define X86_BUG_AMD_E400 X86_BUG(13) /* System may be affected > by Erratum 400, X86_BUG_AMD_APIC_C1E might be needed */ Not "might be needed" - "X86_BUG_AMD_APIC_C1E will be set if platform is affected". And then you don't need the above comment change. And you can't remove "apic_c1e" there because it is magical. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [PATCH] clarify how insecure CPU is
Hi!
> > > > > >
> > > > > > First, what is going on with X86_BUG_AMD_E400 and
> > > > > > X86_BUG_AMD_APIC_C1E
> > > > > > ? They seem to refer to the same bug, perhaps comment should mention
> > > > > > that? (Do we need two flags for one bug?)
> > > > > >
> > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to
> > > > > > address "Meltdown" problem, but not "Spectre". Should it be limited
> > > > > > to
> > > > > > PPro and newer Intel CPUs?
> > > > > >
> > > > > > Should another erratum be added for "Spectre"? This is present even
> > > > > > on
> > > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some
> > > > > > Atom chips?
> > > > > >
> > > > > > Plus... is this reasonable interface?
> > > > > >
> > > > > > bugs: cpu_insecure
> > > > >
> > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for
> > > > > the
> > > > > rest of the mess.
> > > >
> > > > Could you explain (best with code comment) what is going on with
> > > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the
> > > > same bug.
> > >
> > > Sorry, that;s really not the time for this.
> >
> > Ok, is there better time now? The code is a bit confusing...
>
> What's confusing? Here are the relevant code snippets in invocation order.
>
> /*
>* Check whether the machine is affected by erratum 400. This is
>* used to select the proper idle routine and to enable the check
>* whether the machine is affected in arch_post_acpi_init(), which
>* sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check.
>*/
> if (cpu_has_amd_erratum(c, amd_erratum_400))
> set_cpu_bug(c, X86_BUG_AMD_E400);
>
> This sets the errate 400 bug bit to tell subsequent code that the CPU might
> be affected by that erratum.
>
> void select_idle_routine(const struct cpuinfo_x86 *c)
> {
>
> if (boot_cpu_has_bug(X86_BUG_AMD_E400)) {
> pr_info("using AMD E400 aware idle routine\n");
> x86_idle = amd_e400_idle;
>
> Selects the idle routine depending on the bug flag
>
> void __init arch_post_acpi_subsys_init(void)
> {
> u32 lo, hi;
>
> if (!boot_cpu_has_bug(X86_BUG_AMD_E400))
> return;
>
> /*
>* AMD E400 detection needs to happen after ACPI has been enabled. If
>* the machine is affected K8_INTP_C1E_ACTIVE_MASK bits are set in
>* MSR_K8_INT_PENDING_MSG.
>*/
> rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
> if (!(lo & K8_INTP_C1E_ACTIVE_MASK))
> return;
>
> Late detection whether C1E which halts TSC and APIC is enabled. This needs
> to be done after ACPI is initialized.
>
> /*
> * AMD Erratum 400 aware idle routine. We handle it the same way as C3 power
> * states (local apic timer and TSC stop).
> */
> static void amd_e400_idle(void)
> {
> /*
>* We cannot use static_cpu_has_bug() here because X86_BUG_AMD_APIC_C1E
>* gets set after static_cpu_has() places have been converted via
>* alternatives.
>*/
> if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) {
> default_idle();
> return;
> }
>
> The actual idle routine. If the C1E bug flag is not set, CPU is not
> affected, use default idle, otherwise handle it like other C-States which
> stop TSC and APIC.
>
>
> The comments are clear enough, but Feel free to send patches which enhance
> them if you think thats necessary.
Thanks for explanation. Might this be good idea?
Signed-off-by: Pavel Machek
diff --git a/arch/x86/include/asm/cpufeatures.h
b/arch/x86/include/asm/cpufeatures.h
index f41079d..4901742 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -341,7 +341,7 @@
#define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */
#define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */
#define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD Erratum
383 */
-#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD Erratum
400 */
+#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is affected AMD
Erratum 400, special idle routine is needed */
#define X86_BUG_11AP X86_BUG(5) /* Bad local APIC aka 11AP */
#define X86_BUG_FXSAVE_LEAKX86_BUG(6) /* FXSAVE leaks FOP/FIP/FOP
*/
#define X86_BUG_CLFLUSH_MONITORX86_BUG(7) /* AAI65, CLFLUSH
required before MONITOR */
@@ -356,7 +356,7 @@
#define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector
preserves the base */
#define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep
on GS */
#define X86_BUG_MONITORX86_BUG(12) /* IPI required to
wake up remote CPU */
-#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the
affected by Erratum 400 */
+#define X86_BUG_AMD_E400
Re: [PATCH] clarify how insecure CPU is
Hi!
> > > > > >
> > > > > > First, what is going on with X86_BUG_AMD_E400 and
> > > > > > X86_BUG_AMD_APIC_C1E
> > > > > > ? They seem to refer to the same bug, perhaps comment should mention
> > > > > > that? (Do we need two flags for one bug?)
> > > > > >
> > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to
> > > > > > address "Meltdown" problem, but not "Spectre". Should it be limited
> > > > > > to
> > > > > > PPro and newer Intel CPUs?
> > > > > >
> > > > > > Should another erratum be added for "Spectre"? This is present even
> > > > > > on
> > > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some
> > > > > > Atom chips?
> > > > > >
> > > > > > Plus... is this reasonable interface?
> > > > > >
> > > > > > bugs: cpu_insecure
> > > > >
> > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for
> > > > > the
> > > > > rest of the mess.
> > > >
> > > > Could you explain (best with code comment) what is going on with
> > > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the
> > > > same bug.
> > >
> > > Sorry, that;s really not the time for this.
> >
> > Ok, is there better time now? The code is a bit confusing...
>
> What's confusing? Here are the relevant code snippets in invocation order.
>
> /*
>* Check whether the machine is affected by erratum 400. This is
>* used to select the proper idle routine and to enable the check
>* whether the machine is affected in arch_post_acpi_init(), which
>* sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check.
>*/
> if (cpu_has_amd_erratum(c, amd_erratum_400))
> set_cpu_bug(c, X86_BUG_AMD_E400);
>
> This sets the errate 400 bug bit to tell subsequent code that the CPU might
> be affected by that erratum.
>
> void select_idle_routine(const struct cpuinfo_x86 *c)
> {
>
> if (boot_cpu_has_bug(X86_BUG_AMD_E400)) {
> pr_info("using AMD E400 aware idle routine\n");
> x86_idle = amd_e400_idle;
>
> Selects the idle routine depending on the bug flag
>
> void __init arch_post_acpi_subsys_init(void)
> {
> u32 lo, hi;
>
> if (!boot_cpu_has_bug(X86_BUG_AMD_E400))
> return;
>
> /*
>* AMD E400 detection needs to happen after ACPI has been enabled. If
>* the machine is affected K8_INTP_C1E_ACTIVE_MASK bits are set in
>* MSR_K8_INT_PENDING_MSG.
>*/
> rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
> if (!(lo & K8_INTP_C1E_ACTIVE_MASK))
> return;
>
> Late detection whether C1E which halts TSC and APIC is enabled. This needs
> to be done after ACPI is initialized.
>
> /*
> * AMD Erratum 400 aware idle routine. We handle it the same way as C3 power
> * states (local apic timer and TSC stop).
> */
> static void amd_e400_idle(void)
> {
> /*
>* We cannot use static_cpu_has_bug() here because X86_BUG_AMD_APIC_C1E
>* gets set after static_cpu_has() places have been converted via
>* alternatives.
>*/
> if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) {
> default_idle();
> return;
> }
>
> The actual idle routine. If the C1E bug flag is not set, CPU is not
> affected, use default idle, otherwise handle it like other C-States which
> stop TSC and APIC.
>
>
> The comments are clear enough, but Feel free to send patches which enhance
> them if you think thats necessary.
Thanks for explanation. Might this be good idea?
Signed-off-by: Pavel Machek
diff --git a/arch/x86/include/asm/cpufeatures.h
b/arch/x86/include/asm/cpufeatures.h
index f41079d..4901742 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -341,7 +341,7 @@
#define X86_BUG_FDIV X86_BUG(1) /* FPU FDIV */
#define X86_BUG_COMA X86_BUG(2) /* Cyrix 6x86 coma */
#define X86_BUG_AMD_TLB_MMATCH X86_BUG(3) /* "tlb_mmatch" AMD Erratum
383 */
-#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* "apic_c1e" AMD Erratum
400 */
+#define X86_BUG_AMD_APIC_C1E X86_BUG(4) /* System is affected AMD
Erratum 400, special idle routine is needed */
#define X86_BUG_11AP X86_BUG(5) /* Bad local APIC aka 11AP */
#define X86_BUG_FXSAVE_LEAKX86_BUG(6) /* FXSAVE leaks FOP/FIP/FOP
*/
#define X86_BUG_CLFLUSH_MONITORX86_BUG(7) /* AAI65, CLFLUSH
required before MONITOR */
@@ -356,7 +356,7 @@
#define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector
preserves the base */
#define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep
on GS */
#define X86_BUG_MONITORX86_BUG(12) /* IPI required to
wake up remote CPU */
-#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the
affected by Erratum 400 */
+#define X86_BUG_AMD_E400
Re: [PATCH] clarify how insecure CPU is
On Sat, 3 Mar 2018, Pavel Machek wrote:
> On Tue 2018-01-09 00:44:30, Thomas Gleixner wrote:
> > On Tue, 9 Jan 2018, Pavel Machek wrote:
> >
> > > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote:
> > > > On Mon, 8 Jan 2018, Pavel Machek wrote:
> > > >
> > > > >
> > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E
> > > > > ? They seem to refer to the same bug, perhaps comment should mention
> > > > > that? (Do we need two flags for one bug?)
> > > > >
> > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to
> > > > > address "Meltdown" problem, but not "Spectre". Should it be limited to
> > > > > PPro and newer Intel CPUs?
> > > > >
> > > > > Should another erratum be added for "Spectre"? This is present even on
> > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some
> > > > > Atom chips?
> > > > >
> > > > > Plus... is this reasonable interface?
> > > > >
> > > > > bugs : cpu_insecure
> > > >
> > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for
> > > > the
> > > > rest of the mess.
> > >
> > > Could you explain (best with code comment) what is going on with
> > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the
> > > same bug.
> >
> > Sorry, that;s really not the time for this.
>
> Ok, is there better time now? The code is a bit confusing...
What's confusing? Here are the relevant code snippets in invocation order.
/*
* Check whether the machine is affected by erratum 400. This is
* used to select the proper idle routine and to enable the check
* whether the machine is affected in arch_post_acpi_init(), which
* sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check.
*/
if (cpu_has_amd_erratum(c, amd_erratum_400))
set_cpu_bug(c, X86_BUG_AMD_E400);
This sets the errate 400 bug bit to tell subsequent code that the CPU might
be affected by that erratum.
void select_idle_routine(const struct cpuinfo_x86 *c)
{
if (boot_cpu_has_bug(X86_BUG_AMD_E400)) {
pr_info("using AMD E400 aware idle routine\n");
x86_idle = amd_e400_idle;
Selects the idle routine depending on the bug flag
void __init arch_post_acpi_subsys_init(void)
{
u32 lo, hi;
if (!boot_cpu_has_bug(X86_BUG_AMD_E400))
return;
/*
* AMD E400 detection needs to happen after ACPI has been enabled. If
* the machine is affected K8_INTP_C1E_ACTIVE_MASK bits are set in
* MSR_K8_INT_PENDING_MSG.
*/
rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
if (!(lo & K8_INTP_C1E_ACTIVE_MASK))
return;
Late detection whether C1E which halts TSC and APIC is enabled. This needs
to be done after ACPI is initialized.
/*
* AMD Erratum 400 aware idle routine. We handle it the same way as C3 power
* states (local apic timer and TSC stop).
*/
static void amd_e400_idle(void)
{
/*
* We cannot use static_cpu_has_bug() here because X86_BUG_AMD_APIC_C1E
* gets set after static_cpu_has() places have been converted via
* alternatives.
*/
if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) {
default_idle();
return;
}
The actual idle routine. If the C1E bug flag is not set, CPU is not
affected, use default idle, otherwise handle it like other C-States which
stop TSC and APIC.
The comments are clear enough, but Feel free to send patches which enhance
them if you think thats necessary.
Thanks,
tglx
Re: [PATCH] clarify how insecure CPU is
On Sat, 3 Mar 2018, Pavel Machek wrote:
> On Tue 2018-01-09 00:44:30, Thomas Gleixner wrote:
> > On Tue, 9 Jan 2018, Pavel Machek wrote:
> >
> > > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote:
> > > > On Mon, 8 Jan 2018, Pavel Machek wrote:
> > > >
> > > > >
> > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E
> > > > > ? They seem to refer to the same bug, perhaps comment should mention
> > > > > that? (Do we need two flags for one bug?)
> > > > >
> > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to
> > > > > address "Meltdown" problem, but not "Spectre". Should it be limited to
> > > > > PPro and newer Intel CPUs?
> > > > >
> > > > > Should another erratum be added for "Spectre"? This is present even on
> > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some
> > > > > Atom chips?
> > > > >
> > > > > Plus... is this reasonable interface?
> > > > >
> > > > > bugs : cpu_insecure
> > > >
> > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for
> > > > the
> > > > rest of the mess.
> > >
> > > Could you explain (best with code comment) what is going on with
> > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the
> > > same bug.
> >
> > Sorry, that;s really not the time for this.
>
> Ok, is there better time now? The code is a bit confusing...
What's confusing? Here are the relevant code snippets in invocation order.
/*
* Check whether the machine is affected by erratum 400. This is
* used to select the proper idle routine and to enable the check
* whether the machine is affected in arch_post_acpi_init(), which
* sets the X86_BUG_AMD_APIC_C1E bug depending on the MSR check.
*/
if (cpu_has_amd_erratum(c, amd_erratum_400))
set_cpu_bug(c, X86_BUG_AMD_E400);
This sets the errate 400 bug bit to tell subsequent code that the CPU might
be affected by that erratum.
void select_idle_routine(const struct cpuinfo_x86 *c)
{
if (boot_cpu_has_bug(X86_BUG_AMD_E400)) {
pr_info("using AMD E400 aware idle routine\n");
x86_idle = amd_e400_idle;
Selects the idle routine depending on the bug flag
void __init arch_post_acpi_subsys_init(void)
{
u32 lo, hi;
if (!boot_cpu_has_bug(X86_BUG_AMD_E400))
return;
/*
* AMD E400 detection needs to happen after ACPI has been enabled. If
* the machine is affected K8_INTP_C1E_ACTIVE_MASK bits are set in
* MSR_K8_INT_PENDING_MSG.
*/
rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
if (!(lo & K8_INTP_C1E_ACTIVE_MASK))
return;
Late detection whether C1E which halts TSC and APIC is enabled. This needs
to be done after ACPI is initialized.
/*
* AMD Erratum 400 aware idle routine. We handle it the same way as C3 power
* states (local apic timer and TSC stop).
*/
static void amd_e400_idle(void)
{
/*
* We cannot use static_cpu_has_bug() here because X86_BUG_AMD_APIC_C1E
* gets set after static_cpu_has() places have been converted via
* alternatives.
*/
if (!boot_cpu_has_bug(X86_BUG_AMD_APIC_C1E)) {
default_idle();
return;
}
The actual idle routine. If the C1E bug flag is not set, CPU is not
affected, use default idle, otherwise handle it like other C-States which
stop TSC and APIC.
The comments are clear enough, but Feel free to send patches which enhance
them if you think thats necessary.
Thanks,
tglx
Re: [PATCH] clarify how insecure CPU is
On Tue 2018-01-09 00:44:30, Thomas Gleixner wrote: > On Tue, 9 Jan 2018, Pavel Machek wrote: > > > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > > > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > > > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > > > ? They seem to refer to the same bug, perhaps comment should mention > > > > that? (Do we need two flags for one bug?) > > > > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > > > PPro and newer Intel CPUs? > > > > > > > > Should another erratum be added for "Spectre"? This is present even on > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > > > Atom chips? > > > > > > > > Plus... is this reasonable interface? > > > > > > > > bugs: cpu_insecure > > > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > > > rest of the mess. > > > > Could you explain (best with code comment) what is going on with > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the > > same bug. > > Sorry, that;s really not the time for this. Ok, is there better time now? The code is a bit confusing... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Tue 2018-01-09 00:44:30, Thomas Gleixner wrote: > On Tue, 9 Jan 2018, Pavel Machek wrote: > > > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > > > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > > > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > > > ? They seem to refer to the same bug, perhaps comment should mention > > > > that? (Do we need two flags for one bug?) > > > > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > > > PPro and newer Intel CPUs? > > > > > > > > Should another erratum be added for "Spectre"? This is present even on > > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > > > Atom chips? > > > > > > > > Plus... is this reasonable interface? > > > > > > > > bugs: cpu_insecure > > > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > > > rest of the mess. > > > > Could you explain (best with code comment) what is going on with > > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the > > same bug. > > Sorry, that;s really not the time for this. Ok, is there better time now? The code is a bit confusing... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Tue, 9 Jan 2018, Pavel Machek wrote: > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > > ? They seem to refer to the same bug, perhaps comment should mention > > > that? (Do we need two flags for one bug?) > > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > > PPro and newer Intel CPUs? > > > > > > Should another erratum be added for "Spectre"? This is present even on > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > > Atom chips? > > > > > > Plus... is this reasonable interface? > > > > > > bugs : cpu_insecure > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > > rest of the mess. > > Could you explain (best with code comment) what is going on with > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the > same bug. Sorry, that;s really not the time for this. > Plus, as I explained: "bugs: meltdown, spectre" seems to be bad idea, > as userland application can not easily tell between "no bug" and "bug > not known to kernel". https://lkml.kernel.org/r/20180107214913.096657...@linutronix.de https://lkml.kernel.org/r/20180107214913.177414...@linutronix.de Thanks, tglx
Re: [PATCH] clarify how insecure CPU is
On Tue, 9 Jan 2018, Pavel Machek wrote: > On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > > ? They seem to refer to the same bug, perhaps comment should mention > > > that? (Do we need two flags for one bug?) > > > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > > PPro and newer Intel CPUs? > > > > > > Should another erratum be added for "Spectre"? This is present even on > > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > > Atom chips? > > > > > > Plus... is this reasonable interface? > > > > > > bugs : cpu_insecure > > > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > > rest of the mess. > > Could you explain (best with code comment) what is going on with > X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the > same bug. Sorry, that;s really not the time for this. > Plus, as I explained: "bugs: meltdown, spectre" seems to be bad idea, > as userland application can not easily tell between "no bug" and "bug > not known to kernel". https://lkml.kernel.org/r/20180107214913.096657...@linutronix.de https://lkml.kernel.org/r/20180107214913.177414...@linutronix.de Thanks, tglx
Re: [PATCH] clarify how insecure CPU is
On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > ? They seem to refer to the same bug, perhaps comment should mention > > that? (Do we need two flags for one bug?) > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > PPro and newer Intel CPUs? > > > > Should another erratum be added for "Spectre"? This is present even on > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > Atom chips? > > > > Plus... is this reasonable interface? > > > > bugs: cpu_insecure > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > rest of the mess. Could you explain (best with code comment) what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the same bug. Plus, as I explained: "bugs: meltdown, spectre" seems to be bad idea, as userland application can not easily tell between "no bug" and "bug not known to kernel". Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Mon 2018-01-08 21:27:25, Thomas Gleixner wrote: > On Mon, 8 Jan 2018, Pavel Machek wrote: > > > > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > > ? They seem to refer to the same bug, perhaps comment should mention > > that? (Do we need two flags for one bug?) > > > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > > address "Meltdown" problem, but not "Spectre". Should it be limited to > > PPro and newer Intel CPUs? > > > > Should another erratum be added for "Spectre"? This is present even on > > AMD CPUs, but should not be present in 486, maybe Pentium, and some > > Atom chips? > > > > Plus... is this reasonable interface? > > > > bugs: cpu_insecure > > We've renamed it to meltdown already and added spectre_v1/v2 bits for the > rest of the mess. Could you explain (best with code comment) what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the same bug. Plus, as I explained: "bugs: meltdown, spectre" seems to be bad idea, as userland application can not easily tell between "no bug" and "bug not known to kernel". Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] clarify how insecure CPU is
On Mon, 8 Jan 2018, Pavel Machek wrote: > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > ? They seem to refer to the same bug, perhaps comment should mention > that? (Do we need two flags for one bug?) > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > address "Meltdown" problem, but not "Spectre". Should it be limited to > PPro and newer Intel CPUs? > > Should another erratum be added for "Spectre"? This is present even on > AMD CPUs, but should not be present in 486, maybe Pentium, and some > Atom chips? > > Plus... is this reasonable interface? > > bugs : cpu_insecure We've renamed it to meltdown already and added spectre_v1/v2 bits for the rest of the mess.
Re: [PATCH] clarify how insecure CPU is
On Mon, 8 Jan 2018, Pavel Machek wrote: > > First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E > ? They seem to refer to the same bug, perhaps comment should mention > that? (Do we need two flags for one bug?) > > Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to > address "Meltdown" problem, but not "Spectre". Should it be limited to > PPro and newer Intel CPUs? > > Should another erratum be added for "Spectre"? This is present even on > AMD CPUs, but should not be present in 486, maybe Pentium, and some > Atom chips? > > Plus... is this reasonable interface? > > bugs : cpu_insecure We've renamed it to meltdown already and added spectre_v1/v2 bits for the rest of the mess.
[PATCH] clarify how insecure CPU is
First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the same bug, perhaps comment should mention that? (Do we need two flags for one bug?) Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to address "Meltdown" problem, but not "Spectre". Should it be limited to PPro and newer Intel CPUs? Should another erratum be added for "Spectre"? This is present even on AMD CPUs, but should not be present in 486, maybe Pentium, and some Atom chips? Plus... is this reasonable interface? bugs: cpu_insecure I believe we should a) have something more descriptive than 'cpu_insecure', like 'mem_always_r' (because poor user has no chance to know if it is Meltdown, Spectre, or something else) b) have has_meltdown : yes/no, because otherwise poor userspace can not tell if CPU is actually bug-free, or if the kernel is just too old to know about specific bug. With all the backport, this is quite important. Best regards, Pavel diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 07cdd17..d46958e 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -340,7 +340,7 @@ #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector preserves the base */ #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep on GS */ #define X86_BUG_MONITORX86_BUG(12) /* IPI required to wake up remote CPU */ -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the affected by Erratum 400 */ -#define X86_BUG_CPU_INSECURE X86_BUG(14) /* CPU is insecure and needs kernel page table isolation */ +#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the affected by Erratum 400, check for X86_BUG_AMD_APIC_C1E */ +#define X86_BUG_CPU_INSECURE X86_BUG(14) /* CPU always allows reading mapped memory, aka "Meltdown", kernel page table isolation needed */ #endif /* _ASM_X86_CPUFEATURES_H */ -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
[PATCH] clarify how insecure CPU is
First, what is going on with X86_BUG_AMD_E400 and X86_BUG_AMD_APIC_C1E ? They seem to refer to the same bug, perhaps comment should mention that? (Do we need two flags for one bug?) Next, maybe X86_BUG_CPU_INSECURE is a bit too generic? This seems to address "Meltdown" problem, but not "Spectre". Should it be limited to PPro and newer Intel CPUs? Should another erratum be added for "Spectre"? This is present even on AMD CPUs, but should not be present in 486, maybe Pentium, and some Atom chips? Plus... is this reasonable interface? bugs: cpu_insecure I believe we should a) have something more descriptive than 'cpu_insecure', like 'mem_always_r' (because poor user has no chance to know if it is Meltdown, Spectre, or something else) b) have has_meltdown : yes/no, because otherwise poor userspace can not tell if CPU is actually bug-free, or if the kernel is just too old to know about specific bug. With all the backport, this is quite important. Best regards, Pavel diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 07cdd17..d46958e 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -340,7 +340,7 @@ #define X86_BUG_NULL_SEG X86_BUG(10) /* Nulling a selector preserves the base */ #define X86_BUG_SWAPGS_FENCE X86_BUG(11) /* SWAPGS without input dep on GS */ #define X86_BUG_MONITORX86_BUG(12) /* IPI required to wake up remote CPU */ -#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the affected by Erratum 400 */ -#define X86_BUG_CPU_INSECURE X86_BUG(14) /* CPU is insecure and needs kernel page table isolation */ +#define X86_BUG_AMD_E400 X86_BUG(13) /* CPU is among the affected by Erratum 400, check for X86_BUG_AMD_APIC_C1E */ +#define X86_BUG_CPU_INSECURE X86_BUG(14) /* CPU always allows reading mapped memory, aka "Meltdown", kernel page table isolation needed */ #endif /* _ASM_X86_CPUFEATURES_H */ -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
