Re: [PATCH] x86/boot: Do not test if AC and ID eflags are changeable on x86_64

[H. Peter Anvin]

On March 8, 2025 11:34:49 AM PST, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>* H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>
>> On March 7, 2025 4:15:42 AM PST, Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
>> >On Fri, Mar 7, 2025 at 12:58 PM H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>> >>
>> >> On March 7, 2025 1:10:03 AM PST, Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
>> >> >The test for the changeabitily of AC and ID eflags is used to
>> >> >distinguish between i386 and i486 processors (AC) and to test
>> >> >for cpuid instruction support (ID).
>> >> >
>> >> >Skip these tests on x86_64 processors as they always supports cpuid.
>> >> >
>> >> >Also change the return type of has_eflag() to bool.
>> >> >
>> >> >Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
>> >> >Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> >> >Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> >> >Cc: Borislav Petkov <bp@xxxxxxxxx>
>> >> >Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>> >> >Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>> >> >---
>> >> > arch/x86/boot/cpuflags.c | 26 +++++++++-----------------
>> >> > arch/x86/boot/cpuflags.h |  6 +++++-
>> >> > 2 files changed, 14 insertions(+), 18 deletions(-)
>> >> >
>> >> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
>> >> >index d75237ba7ce9..2150a016176f 100644
>> >> >--- a/arch/x86/boot/cpuflags.c
>> >> >+++ b/arch/x86/boot/cpuflags.c
>> >> >@@ -29,40 +29,32 @@ static int has_fpu(void)
>> >> >       return fsw == 0 && (fcw & 0x103f) == 0x003f;
>> >> > }
>> >> >
>> >> >+#ifdef CONFIG_X86_32
>> >> > /*
>> >> >  * For building the 16-bit code we want to explicitly specify 32-bit
>> >> >  * push/pop operations, rather than just saying 'pushf' or 'popf' and
>> >> >- * letting the compiler choose. But this is also included from the
>> >> >- * compressed/ directory where it may be 64-bit code, and thus needs
>> >> >- * to be 'pushfq' or 'popfq' in that case.
>> >> >+ * letting the compiler choose.
>> >> >  */
>> >> >-#ifdef __x86_64__
>> >> >-#define PUSHF "pushfq"
>> >> >-#define POPF "popfq"
>> >> >-#else
>> >> >-#define PUSHF "pushfl"
>> >> >-#define POPF "popfl"
>> >> >-#endif
>> >> >-
>> >> >-int has_eflag(unsigned long mask)
>> >> >+bool has_eflag(unsigned long mask)
>> >> > {
>> >> >       unsigned long f0, f1;
>> >> >
>> >> >-      asm volatile(PUSHF "    \n\t"
>> >> >-                   PUSHF "    \n\t"
>> >> >+      asm volatile("pushfl    \n\t"
>> >> >+                   "pushfl    \n\t"
>> >> >                    "pop %0    \n\t"
>> >> >                    "mov %0,%1 \n\t"
>> >> >                    "xor %2,%1 \n\t"
>> >> >                    "push %1   \n\t"
>> >> >-                   POPF "     \n\t"
>> >> >-                   PUSHF "    \n\t"
>> >> >+                   "popfl     \n\t"
>> >> >+                   "pushfl    \n\t"
>> >> >                    "pop %1    \n\t"
>> >> >-                   POPF
>> >> >+                   "popfl"
>> >> >                    : "=&r" (f0), "=&r" (f1)
>> >> >                    : "ri" (mask));
>> >> >
>> >> >       return !!((f0^f1) & mask);
>> >> > }
>> >> >+#endif
>> >> >
>> >> > void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d)
>> >> > {
>> >> >diff --git a/arch/x86/boot/cpuflags.h b/arch/x86/boot/cpuflags.h
>> >> >index fdcc2aa4c3c4..a398d9204ad0 100644
>> >> >--- a/arch/x86/boot/cpuflags.h
>> >> >+++ b/arch/x86/boot/cpuflags.h
>> >> >@@ -15,7 +15,11 @@ struct cpu_features {
>> >> > extern struct cpu_features cpu;
>> >> > extern u32 cpu_vendor[3];
>> >> >
>> >> >-int has_eflag(unsigned long mask);
>> >> >+#ifdef CONFIG_X86_32
>> >> >+bool has_eflag(unsigned long mask);
>> >> >+#else
>> >> >+static inline bool has_eflag(unsigned long mask) { return true; }
>> >> >+#endif
>> >> > void get_cpuflags(void);
>> >> > void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d);
>> >> > bool has_cpuflag(int flag);
>> >>
>> >> PUSF et al → pushf
>> >>
>> >> The -l and -q suffixes have been optional for a long time.
>> >
>> >No, not in this case. Please see the comment:
>> >
>> >/*
>> >* For building the 16-bit code we want to explicitly specify 32-bit
>> >* push/pop operations, rather than just saying 'pushf' or 'popf' and
>> >* letting the compiler choose.
>> >*/
>> >
>> >We are building 16-bit code here, and we want PUSHFL, the one with
>> >operand size prefix 0x66.
>> >
>> >Please consider the following code:
>> >
>> >    .code16
>> >    pushf
>> >    pushfl
>> >
>> >as -o push.o push.s
>> >
>> >objdump -dr -Mdata16 push.o
>> >
>> >0000000000000000 <.text>:
>> >  0:   9c                      pushf
>> >  1:   66 9c                   pushfl
>> >
>> >Uros.
>> >
>> 
>> *plonk* I should have remembered (.code16gcc is different then 
>> .code16 though.) I wrote the damned things after all...
>
>So I'll read this as a tentative:
>
>  Acked-by: H. Peter Anvin <hpa@xxxxxxxxx>
>
>Let me know if I'm not reading the room right. :-)
>
>	Ingo

Indeed.