* [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c @ 2024-07-18 6:32 Uros Bizjak 2024-07-18 6:35 ` H. Peter Anvin 0 siblings, 1 reply; 10+ messages in thread From: Uros Bizjak @ 2024-07-18 6:32 UTC (permalink / raw) To: x86, linux-kernel Cc: Uros Bizjak, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. 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 | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c index d75237ba7ce9..aacabe431fd5 100644 --- a/arch/x86/boot/cpuflags.c +++ b/arch/x86/boot/cpuflags.c @@ -2,6 +2,7 @@ #include <linux/types.h> #include "bitops.h" +#include <asm/asm.h> #include <asm/processor-flags.h> #include <asm/required-features.h> #include <asm/msr-index.h> @@ -36,13 +37,8 @@ static int has_fpu(void) * compressed/ directory where it may be 64-bit code, and thus needs * to be 'pushfq' or 'popfq' in that case. */ -#ifdef __x86_64__ -#define PUSHF "pushfq" -#define POPF "popfq" -#else -#define PUSHF "pushfl" -#define POPF "popfl" -#endif +#define PUSHF __ASM_SIZE(pushf) +#define POPF __ASM_SIZE(popf) int has_eflag(unsigned long mask) { -- 2.45.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 6:32 [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c Uros Bizjak @ 2024-07-18 6:35 ` H. Peter Anvin 2024-07-18 6:46 ` Uros Bizjak 2024-07-18 8:52 ` Uros Bizjak 0 siblings, 2 replies; 10+ messages in thread From: H. Peter Anvin @ 2024-07-18 6:35 UTC (permalink / raw) To: Uros Bizjak, x86, linux-kernel Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. > >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 | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c >index d75237ba7ce9..aacabe431fd5 100644 >--- a/arch/x86/boot/cpuflags.c >+++ b/arch/x86/boot/cpuflags.c >@@ -2,6 +2,7 @@ > #include <linux/types.h> > #include "bitops.h" > >+#include <asm/asm.h> > #include <asm/processor-flags.h> > #include <asm/required-features.h> > #include <asm/msr-index.h> >@@ -36,13 +37,8 @@ static int has_fpu(void) > * compressed/ directory where it may be 64-bit code, and thus needs > * to be 'pushfq' or 'popfq' in that case. > */ >-#ifdef __x86_64__ >-#define PUSHF "pushfq" >-#define POPF "popfq" >-#else >-#define PUSHF "pushfl" >-#define POPF "popfl" >-#endif >+#define PUSHF __ASM_SIZE(pushf) >+#define POPF __ASM_SIZE(popf) > > int has_eflag(unsigned long mask) > { Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 6:35 ` H. Peter Anvin @ 2024-07-18 6:46 ` Uros Bizjak 2024-07-18 6:58 ` H. Peter Anvin 2024-07-18 8:52 ` Uros Bizjak 1 sibling, 1 reply; 10+ messages in thread From: Uros Bizjak @ 2024-07-18 6:46 UTC (permalink / raw) To: H. Peter Anvin Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: > > On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: > >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. > > > >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 | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c > >index d75237ba7ce9..aacabe431fd5 100644 > >--- a/arch/x86/boot/cpuflags.c > >+++ b/arch/x86/boot/cpuflags.c > >@@ -2,6 +2,7 @@ > > #include <linux/types.h> > > #include "bitops.h" > > > >+#include <asm/asm.h> > > #include <asm/processor-flags.h> > > #include <asm/required-features.h> > > #include <asm/msr-index.h> > >@@ -36,13 +37,8 @@ static int has_fpu(void) > > * compressed/ directory where it may be 64-bit code, and thus needs > > * to be 'pushfq' or 'popfq' in that case. > > */ > >-#ifdef __x86_64__ > >-#define PUSHF "pushfq" > >-#define POPF "popfq" > >-#else > >-#define PUSHF "pushfl" > >-#define POPF "popfl" > >-#endif > >+#define PUSHF __ASM_SIZE(pushf) > >+#define POPF __ASM_SIZE(popf) > > > > int has_eflag(unsigned long mask) > > { > > Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. Yes, this works, too. So I guess we can also remove the comment explaining the reason for explicit suffixes? Thanks, Uros. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 6:46 ` Uros Bizjak @ 2024-07-18 6:58 ` H. Peter Anvin 2024-07-18 7:27 ` Uros Bizjak 0 siblings, 1 reply; 10+ messages in thread From: H. Peter Anvin @ 2024-07-18 6:58 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On July 17, 2024 11:46:57 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: >> >> On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >> >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. >> > >> >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 | 10 +++------- >> > 1 file changed, 3 insertions(+), 7 deletions(-) >> > >> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c >> >index d75237ba7ce9..aacabe431fd5 100644 >> >--- a/arch/x86/boot/cpuflags.c >> >+++ b/arch/x86/boot/cpuflags.c >> >@@ -2,6 +2,7 @@ >> > #include <linux/types.h> >> > #include "bitops.h" >> > >> >+#include <asm/asm.h> >> > #include <asm/processor-flags.h> >> > #include <asm/required-features.h> >> > #include <asm/msr-index.h> >> >@@ -36,13 +37,8 @@ static int has_fpu(void) >> > * compressed/ directory where it may be 64-bit code, and thus needs >> > * to be 'pushfq' or 'popfq' in that case. >> > */ >> >-#ifdef __x86_64__ >> >-#define PUSHF "pushfq" >> >-#define POPF "popfq" >> >-#else >> >-#define PUSHF "pushfl" >> >-#define POPF "popfl" >> >-#endif >> >+#define PUSHF __ASM_SIZE(pushf) >> >+#define POPF __ASM_SIZE(popf) >> > >> > int has_eflag(unsigned long mask) >> > { >> >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. > >Yes, this works, too. So I guess we can also remove the comment >explaining the reason for explicit suffixes? > >Thanks, >Uros. > Yeah. You may want to check the version of binutils that fixed it and put that in the comments. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 6:58 ` H. Peter Anvin @ 2024-07-18 7:27 ` Uros Bizjak 2024-07-18 7:28 ` H. Peter Anvin 0 siblings, 1 reply; 10+ messages in thread From: Uros Bizjak @ 2024-07-18 7:27 UTC (permalink / raw) To: H. Peter Anvin Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On Thu, Jul 18, 2024 at 8:59 AM H. Peter Anvin <hpa@zytor.com> wrote: > > >> >-#ifdef __x86_64__ > >> >-#define PUSHF "pushfq" > >> >-#define POPF "popfq" > >> >-#else > >> >-#define PUSHF "pushfl" > >> >-#define POPF "popfl" > >> >-#endif > >> >+#define PUSHF __ASM_SIZE(pushf) > >> >+#define POPF __ASM_SIZE(popf) > >> > > >> > int has_eflag(unsigned long mask) > >> > { > >> > >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. > > > >Yes, this works, too. So I guess we can also remove the comment > >explaining the reason for explicit suffixes? > > > >Thanks, > >Uros. > > > > Yeah. You may want to check the version of binutils that fixed it and put that in the comments. I have checked that the build works with 9 year old binutils-2.25 (minimal required version), so the fix was applied to an even earlier version. I guess we don't want to burden unsuspecting readers with historic toolchain oddities, so I propose to just remove the comment for good. Uros. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 7:27 ` Uros Bizjak @ 2024-07-18 7:28 ` H. Peter Anvin 0 siblings, 0 replies; 10+ messages in thread From: H. Peter Anvin @ 2024-07-18 7:28 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On July 18, 2024 12:27:04 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >On Thu, Jul 18, 2024 at 8:59 AM H. Peter Anvin <hpa@zytor.com> wrote: >> >> >> >-#ifdef __x86_64__ >> >> >-#define PUSHF "pushfq" >> >> >-#define POPF "popfq" >> >> >-#else >> >> >-#define PUSHF "pushfl" >> >> >-#define POPF "popfl" >> >> >-#endif >> >> >+#define PUSHF __ASM_SIZE(pushf) >> >> >+#define POPF __ASM_SIZE(popf) >> >> > >> >> > int has_eflag(unsigned long mask) >> >> > { >> >> >> >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. >> > >> >Yes, this works, too. So I guess we can also remove the comment >> >explaining the reason for explicit suffixes? >> > >> >Thanks, >> >Uros. >> > >> >> Yeah. You may want to check the version of binutils that fixed it and put that in the comments. > >I have checked that the build works with 9 year old binutils-2.25 >(minimal required version), so the fix was applied to an even earlier >version. I guess we don't want to burden unsuspecting readers with >historic toolchain oddities, so I propose to just remove the comment >for good. > >Uros. > Yeah, I meant the commit message not comments, sorry. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 6:35 ` H. Peter Anvin 2024-07-18 6:46 ` Uros Bizjak @ 2024-07-18 8:52 ` Uros Bizjak 2024-07-18 8:55 ` H. Peter Anvin 1 sibling, 1 reply; 10+ messages in thread From: Uros Bizjak @ 2024-07-18 8:52 UTC (permalink / raw) To: H. Peter Anvin Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: > > On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: > >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. > > > >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 | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c > >index d75237ba7ce9..aacabe431fd5 100644 > >--- a/arch/x86/boot/cpuflags.c > >+++ b/arch/x86/boot/cpuflags.c > >@@ -2,6 +2,7 @@ > > #include <linux/types.h> > > #include "bitops.h" > > > >+#include <asm/asm.h> > > #include <asm/processor-flags.h> > > #include <asm/required-features.h> > > #include <asm/msr-index.h> > >@@ -36,13 +37,8 @@ static int has_fpu(void) > > * compressed/ directory where it may be 64-bit code, and thus needs > > * to be 'pushfq' or 'popfq' in that case. > > */ > >-#ifdef __x86_64__ > >-#define PUSHF "pushfq" > >-#define POPF "popfq" > >-#else > >-#define PUSHF "pushfl" > >-#define POPF "popfl" > >-#endif > >+#define PUSHF __ASM_SIZE(pushf) > >+#define POPF __ASM_SIZE(popf) > > > > int has_eflag(unsigned long mask) > > { > > Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. Unfortunately, clang does not do the right thing when pushf/popf without suffix are used. arch/x86/boot/cpuflags.c compiles to: 00000000 <has_eflag>: 0: 9c pushf 1: 9c pushf 2: 66 5a pop %edx 4: 66 89 d1 mov %edx,%ecx 7: 66 31 c1 xor %eax,%ecx a: 66 51 push %ecx c: 9d popf d: 9c pushf e: 66 59 pop %ecx 10: 9d popf 11: 66 31 ca xor %ecx,%edx 14: 66 31 c9 xor %ecx,%ecx 17: 66 85 c2 test %eax,%edx 1a: 0f 95 c1 setne %cl 1d: 66 89 c8 mov %ecx,%eax 20: 66 c3 retl instead of: 00000000 <has_eflag>: 0: 66 9c pushfl 2: 66 9c pushfl 4: 66 5a pop %edx 6: 66 89 d1 mov %edx,%ecx 9: 66 31 c1 xor %eax,%ecx c: 66 51 push %ecx e: 66 9d popfl 10: 66 9c pushfl 12: 66 59 pop %ecx 14: 66 9d popfl 16: 66 31 ca xor %ecx,%edx 19: 66 31 c9 xor %ecx,%ecx 1c: 66 85 c2 test %eax,%edx 1f: 0f 95 c1 setne %cl 22: 66 89 c8 mov %ecx,%eax 25: 66 c3 retl Please note missing 0x66 operand size override prefixes with pushfl and popfl. This is 16bit code, operand prefixes are mandatory to push 32-bit EFLAGS register (ID flag lives in bit 21). So, the original patch is the way to go. Uros. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 8:52 ` Uros Bizjak @ 2024-07-18 8:55 ` H. Peter Anvin 2024-07-18 9:12 ` Uros Bizjak 0 siblings, 1 reply; 10+ messages in thread From: H. Peter Anvin @ 2024-07-18 8:55 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On July 18, 2024 1:52:17 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: >> >> On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >> >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. >> > >> >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 | 10 +++------- >> > 1 file changed, 3 insertions(+), 7 deletions(-) >> > >> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c >> >index d75237ba7ce9..aacabe431fd5 100644 >> >--- a/arch/x86/boot/cpuflags.c >> >+++ b/arch/x86/boot/cpuflags.c >> >@@ -2,6 +2,7 @@ >> > #include <linux/types.h> >> > #include "bitops.h" >> > >> >+#include <asm/asm.h> >> > #include <asm/processor-flags.h> >> > #include <asm/required-features.h> >> > #include <asm/msr-index.h> >> >@@ -36,13 +37,8 @@ static int has_fpu(void) >> > * compressed/ directory where it may be 64-bit code, and thus needs >> > * to be 'pushfq' or 'popfq' in that case. >> > */ >> >-#ifdef __x86_64__ >> >-#define PUSHF "pushfq" >> >-#define POPF "popfq" >> >-#else >> >-#define PUSHF "pushfl" >> >-#define POPF "popfl" >> >-#endif >> >+#define PUSHF __ASM_SIZE(pushf) >> >+#define POPF __ASM_SIZE(popf) >> > >> > int has_eflag(unsigned long mask) >> > { >> >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. > >Unfortunately, clang does not do the right thing when pushf/popf >without suffix are used. > >arch/x86/boot/cpuflags.c compiles to: > >00000000 <has_eflag>: > 0: 9c pushf > 1: 9c pushf > 2: 66 5a pop %edx > 4: 66 89 d1 mov %edx,%ecx > 7: 66 31 c1 xor %eax,%ecx > a: 66 51 push %ecx > c: 9d popf > d: 9c pushf > e: 66 59 pop %ecx > 10: 9d popf > 11: 66 31 ca xor %ecx,%edx > 14: 66 31 c9 xor %ecx,%ecx > 17: 66 85 c2 test %eax,%edx > 1a: 0f 95 c1 setne %cl > 1d: 66 89 c8 mov %ecx,%eax > 20: 66 c3 retl > >instead of: > >00000000 <has_eflag>: > 0: 66 9c pushfl > 2: 66 9c pushfl > 4: 66 5a pop %edx > 6: 66 89 d1 mov %edx,%ecx > 9: 66 31 c1 xor %eax,%ecx > c: 66 51 push %ecx > e: 66 9d popfl > 10: 66 9c pushfl > 12: 66 59 pop %ecx > 14: 66 9d popfl > 16: 66 31 ca xor %ecx,%edx > 19: 66 31 c9 xor %ecx,%ecx > 1c: 66 85 c2 test %eax,%edx > 1f: 0f 95 c1 setne %cl > 22: 66 89 c8 mov %ecx,%eax > 25: 66 c3 retl > >Please note missing 0x66 operand size override prefixes with pushfl >and popfl. This is 16bit code, operand prefixes are mandatory to push >32-bit EFLAGS register (ID flag lives in bit 21). > >So, the original patch is the way to go. > >Uros. > You do know that has_eflag can be completely elided on x86-64, or you can use %z with one of the register operands. One more reason why clang really needs to shape up. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 8:55 ` H. Peter Anvin @ 2024-07-18 9:12 ` Uros Bizjak 2024-07-18 20:53 ` H. Peter Anvin 0 siblings, 1 reply; 10+ messages in thread From: Uros Bizjak @ 2024-07-18 9:12 UTC (permalink / raw) To: H. Peter Anvin Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On Thu, Jul 18, 2024 at 10:56 AM H. Peter Anvin <hpa@zytor.com> wrote: > > On July 18, 2024 1:52:17 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: > >On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: > >> > >> On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: > >> >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. > >> > > >> >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 | 10 +++------- > >> > 1 file changed, 3 insertions(+), 7 deletions(-) > >> > > >> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c > >> >index d75237ba7ce9..aacabe431fd5 100644 > >> >--- a/arch/x86/boot/cpuflags.c > >> >+++ b/arch/x86/boot/cpuflags.c > >> >@@ -2,6 +2,7 @@ > >> > #include <linux/types.h> > >> > #include "bitops.h" > >> > > >> >+#include <asm/asm.h> > >> > #include <asm/processor-flags.h> > >> > #include <asm/required-features.h> > >> > #include <asm/msr-index.h> > >> >@@ -36,13 +37,8 @@ static int has_fpu(void) > >> > * compressed/ directory where it may be 64-bit code, and thus needs > >> > * to be 'pushfq' or 'popfq' in that case. > >> > */ > >> >-#ifdef __x86_64__ > >> >-#define PUSHF "pushfq" > >> >-#define POPF "popfq" > >> >-#else > >> >-#define PUSHF "pushfl" > >> >-#define POPF "popfl" > >> >-#endif > >> >+#define PUSHF __ASM_SIZE(pushf) > >> >+#define POPF __ASM_SIZE(popf) > >> > > >> > int has_eflag(unsigned long mask) > >> > { > >> > >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. > > > >Unfortunately, clang does not do the right thing when pushf/popf > >without suffix are used. > > > >arch/x86/boot/cpuflags.c compiles to: > > > >00000000 <has_eflag>: > > 0: 9c pushf > > 1: 9c pushf > > 2: 66 5a pop %edx > > 4: 66 89 d1 mov %edx,%ecx > > 7: 66 31 c1 xor %eax,%ecx > > a: 66 51 push %ecx > > c: 9d popf > > d: 9c pushf > > e: 66 59 pop %ecx > > 10: 9d popf > > 11: 66 31 ca xor %ecx,%edx > > 14: 66 31 c9 xor %ecx,%ecx > > 17: 66 85 c2 test %eax,%edx > > 1a: 0f 95 c1 setne %cl > > 1d: 66 89 c8 mov %ecx,%eax > > 20: 66 c3 retl > > > >instead of: > > > >00000000 <has_eflag>: > > 0: 66 9c pushfl > > 2: 66 9c pushfl > > 4: 66 5a pop %edx > > 6: 66 89 d1 mov %edx,%ecx > > 9: 66 31 c1 xor %eax,%ecx > > c: 66 51 push %ecx > > e: 66 9d popfl > > 10: 66 9c pushfl > > 12: 66 59 pop %ecx > > 14: 66 9d popfl > > 16: 66 31 ca xor %ecx,%edx > > 19: 66 31 c9 xor %ecx,%ecx > > 1c: 66 85 c2 test %eax,%edx > > 1f: 0f 95 c1 setne %cl > > 22: 66 89 c8 mov %ecx,%eax > > 25: 66 c3 retl > > > >Please note missing 0x66 operand size override prefixes with pushfl > >and popfl. This is 16bit code, operand prefixes are mandatory to push > >32-bit EFLAGS register (ID flag lives in bit 21). > > > >So, the original patch is the way to go. > > > >Uros. > > > > You do know that has_eflag can be completely elided on x86-64, or you can use %z with one of the register operands. It is 32-bit PUSHFL insn that requires the "L" suffix in 16-bit code. This is x86_32 issue and clang was just lucky that the instruction was always defined with explicit L suffix. Please note that PUSHF has no register operand, so %z can't be used. > One more reason why clang really needs to shape up. Indeed. Thanks, Uros. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c 2024-07-18 9:12 ` Uros Bizjak @ 2024-07-18 20:53 ` H. Peter Anvin 0 siblings, 0 replies; 10+ messages in thread From: H. Peter Anvin @ 2024-07-18 20:53 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen On July 18, 2024 2:12:07 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >On Thu, Jul 18, 2024 at 10:56 AM H. Peter Anvin <hpa@zytor.com> wrote: >> >> On July 18, 2024 1:52:17 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >> >On Thu, Jul 18, 2024 at 8:36 AM H. Peter Anvin <hpa@zytor.com> wrote: >> >> >> >> On July 17, 2024 11:32:18 PM PDT, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> >Use __ASM_SIZE() macro to add correct insn suffix to pushf/popf. >> >> > >> >> >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 | 10 +++------- >> >> > 1 file changed, 3 insertions(+), 7 deletions(-) >> >> > >> >> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c >> >> >index d75237ba7ce9..aacabe431fd5 100644 >> >> >--- a/arch/x86/boot/cpuflags.c >> >> >+++ b/arch/x86/boot/cpuflags.c >> >> >@@ -2,6 +2,7 @@ >> >> > #include <linux/types.h> >> >> > #include "bitops.h" >> >> > >> >> >+#include <asm/asm.h> >> >> > #include <asm/processor-flags.h> >> >> > #include <asm/required-features.h> >> >> > #include <asm/msr-index.h> >> >> >@@ -36,13 +37,8 @@ static int has_fpu(void) >> >> > * compressed/ directory where it may be 64-bit code, and thus needs >> >> > * to be 'pushfq' or 'popfq' in that case. >> >> > */ >> >> >-#ifdef __x86_64__ >> >> >-#define PUSHF "pushfq" >> >> >-#define POPF "popfq" >> >> >-#else >> >> >-#define PUSHF "pushfl" >> >> >-#define POPF "popfl" >> >> >-#endif >> >> >+#define PUSHF __ASM_SIZE(pushf) >> >> >+#define POPF __ASM_SIZE(popf) >> >> > >> >> > int has_eflag(unsigned long mask) >> >> > { >> >> >> >> Just use pushf/popf. gas hasn't needed that suffix for a long time as far as I know. >> > >> >Unfortunately, clang does not do the right thing when pushf/popf >> >without suffix are used. >> > >> >arch/x86/boot/cpuflags.c compiles to: >> > >> >00000000 <has_eflag>: >> > 0: 9c pushf >> > 1: 9c pushf >> > 2: 66 5a pop %edx >> > 4: 66 89 d1 mov %edx,%ecx >> > 7: 66 31 c1 xor %eax,%ecx >> > a: 66 51 push %ecx >> > c: 9d popf >> > d: 9c pushf >> > e: 66 59 pop %ecx >> > 10: 9d popf >> > 11: 66 31 ca xor %ecx,%edx >> > 14: 66 31 c9 xor %ecx,%ecx >> > 17: 66 85 c2 test %eax,%edx >> > 1a: 0f 95 c1 setne %cl >> > 1d: 66 89 c8 mov %ecx,%eax >> > 20: 66 c3 retl >> > >> >instead of: >> > >> >00000000 <has_eflag>: >> > 0: 66 9c pushfl >> > 2: 66 9c pushfl >> > 4: 66 5a pop %edx >> > 6: 66 89 d1 mov %edx,%ecx >> > 9: 66 31 c1 xor %eax,%ecx >> > c: 66 51 push %ecx >> > e: 66 9d popfl >> > 10: 66 9c pushfl >> > 12: 66 59 pop %ecx >> > 14: 66 9d popfl >> > 16: 66 31 ca xor %ecx,%edx >> > 19: 66 31 c9 xor %ecx,%ecx >> > 1c: 66 85 c2 test %eax,%edx >> > 1f: 0f 95 c1 setne %cl >> > 22: 66 89 c8 mov %ecx,%eax >> > 25: 66 c3 retl >> > >> >Please note missing 0x66 operand size override prefixes with pushfl >> >and popfl. This is 16bit code, operand prefixes are mandatory to push >> >32-bit EFLAGS register (ID flag lives in bit 21). >> > >> >So, the original patch is the way to go. >> > >> >Uros. >> > >> >> You do know that has_eflag can be completely elided on x86-64, or you can use %z with one of the register operands. > >It is 32-bit PUSHFL insn that requires the "L" suffix in 16-bit code. >This is x86_32 issue and clang was just lucky that the instruction was >always defined with explicit L suffix. Please note that PUSHF has no >register operand, so %z can't be used. > >> One more reason why clang really needs to shape up. > >Indeed. > >Thanks, >Uros. > Oh, I see – it is a bug in clang -m16 (.code16gcc or its equivalent.) However, there you can always do pushfl/popfl since that doesn't apply to 64-bit mode: both AC and ID always exist in 64-bit mode and any future features should be detected by cpuid rather than by frobbing random bits in xFLAGS. I suggest have_eflag_ac() and have_eflag_id() and just have them return true on 64 bits. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-07-18 20:53 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-18 6:32 [PATCH] x86/boot: Use __ASM_SIZE() to reduce ifdeffery in cpuflags.c Uros Bizjak 2024-07-18 6:35 ` H. Peter Anvin 2024-07-18 6:46 ` Uros Bizjak 2024-07-18 6:58 ` H. Peter Anvin 2024-07-18 7:27 ` Uros Bizjak 2024-07-18 7:28 ` H. Peter Anvin 2024-07-18 8:52 ` Uros Bizjak 2024-07-18 8:55 ` H. Peter Anvin 2024-07-18 9:12 ` Uros Bizjak 2024-07-18 20:53 ` H. Peter Anvin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.