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

[Ingo Molnar <mingo@kernel.org>]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On March 7, 2025 4:15:42 AM PST, Uros Bizjak <ubizjak@gmail.com> wrote:
> >On Fri, Mar 7, 2025 at 12:58 PM H. Peter Anvin <hpa@zytor.com> wrote:
> >>
> >> On March 7, 2025 1:10:03 AM PST, Uros Bizjak <ubizjak@gmail.com> 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@gmail.com>
> >> >Cc: Thomas Gleixner <tglx@linutronix.de>
> >> >Cc: Ingo Molnar <mingo@kernel.org>
> >> >Cc: Borislav Petkov <bp@alien8.de>
> >> >Cc: Dave Hansen <dave.hansen@linux.intel.com>
> >> >Cc: "H. Peter Anvin" <hpa@zytor.com>
> >> >---
> >> > 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@zytor.com>

Let me know if I'm not reading the room right. :-)

	Ingo