x86/asm: Use alternative_input() in amd_clear_divider()

[PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by Uros Bizjak 9 months, 1 week ago

Use higher-level API to declare assembly with alternatives.

bloat-o-meter reports no code size changes.

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/include/asm/processor.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 5d2f7e5aff26..b4b5aa46f68b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -707,8 +707,9 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
  */
 static __always_inline void amd_clear_divider(void)
 {
-	asm volatile(ALTERNATIVE("", "div %2\n\t", X86_BUG_DIV0)
-		     :: "a" (0), "d" (0), "r" (1));
+	alternative_input("", "div %[den]",
+			  X86_BUG_DIV0,
+			  "a" (0), "d" (0), [den] "r" (1));
 }
 
 extern void amd_check_microcode(void);
-- 
2.48.1

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by H. Peter Anvin 9 months, 1 week ago

On March 14, 2025 1:14:38 AM PDT, Uros Bizjak <ubizjak@gmail.com> wrote:
>Use higher-level API to declare assembly with alternatives.
>
>bloat-o-meter reports no code size changes.
>
>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/include/asm/processor.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>index 5d2f7e5aff26..b4b5aa46f68b 100644
>--- a/arch/x86/include/asm/processor.h
>+++ b/arch/x86/include/asm/processor.h
>@@ -707,8 +707,9 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
>  */
> static __always_inline void amd_clear_divider(void)
> {
>-	asm volatile(ALTERNATIVE("", "div %2\n\t", X86_BUG_DIV0)
>-		     :: "a" (0), "d" (0), "r" (1));
>+	alternative_input("", "div %[den]",
>+			  X86_BUG_DIV0,
>+			  "a" (0), "d" (0), [den] "r" (1));
> }
> 
> extern void amd_check_microcode(void);

I realize I'm probably in the minority, but I find these "higher-level APIs" for alternatives much harder to read and understand.

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by Borislav Petkov 9 months, 1 week ago

On Fri, Mar 14, 2025 at 09:14:38AM +0100, Uros Bizjak wrote:
> Use higher-level API to declare assembly with alternatives.
> 
> bloat-o-meter reports no code size changes.
> 
> 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/include/asm/processor.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

I'm getting tired of patches which cause unnecessary code churn. Please stop
this. If it ain't broke, it doesn't need fixing!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by Ingo Molnar 9 months ago

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Mar 14, 2025 at 09:14:38AM +0100, Uros Bizjak wrote:
> > Use higher-level API to declare assembly with alternatives.
> > 
> > bloat-o-meter reports no code size changes.
> > 
> > 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/include/asm/processor.h | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> I'm getting tired of patches which cause unnecessary code churn. Please stop
> this. If it ain't broke, it doesn't need fixing!

So why does the higher level alternative_input() API exist? If it 
shouldn't exist then we should remove it. If it exists, we should use 
it consistently instead of open-coding its equivalent.

Cleanups like this, especially if they are clearly part of an effort to 
improve x86 code generation in this area, are not 'code churn', why 
would they be?

Thanks,

	Ingo

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by Borislav Petkov 9 months ago

On Sat, Mar 15, 2025 at 10:29:25PM +0100, Ingo Molnar wrote:
> So why does the higher level alternative_input() API exist? If it 
> shouldn't exist then we should remove it. If it exists, we should use 
> it consistently instead of open-coding its equivalent.
> 
> Cleanups like this, especially if they are clearly part of an effort to 
> improve x86 code generation in this area, are not 'code churn', why 
> would they be?

Because this is not improving anything, IMO. It is simply writing it
differently, perhaps obscuring it more in the process.

And I, just like hpa, would need to go look at alternative_input() to figure
out what really happens there.

Dunno, maybe we should really remove alternative_input() instead...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by H. Peter Anvin 9 months ago

On March 15, 2025 2:36:55 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Sat, Mar 15, 2025 at 10:29:25PM +0100, Ingo Molnar wrote:
>> So why does the higher level alternative_input() API exist? If it 
>> shouldn't exist then we should remove it. If it exists, we should use 
>> it consistently instead of open-coding its equivalent.
>> 
>> Cleanups like this, especially if they are clearly part of an effort to 
>> improve x86 code generation in this area, are not 'code churn', why 
>> would they be?
>
>Because this is not improving anything, IMO. It is simply writing it
>differently, perhaps obscuring it more in the process.
>
>And I, just like hpa, would need to go look at alternative_input() to figure
>out what really happens there.
>
>Dunno, maybe we should really remove alternative_input() instead...
>

My main beef with the "higher level" macros is that they both obscure what is going on and they have a syntax which is very different from the typical asm statement, all of which makes them harder to read. Furthermore, they don't cover all the possible ways that one can use the alternatives API – and because cpp doesn't lend itself well to handling the colon-separated groups of asm syntax – that really can't be fixed easily.

Re: [PATCH] x86/asm: Use alternative_input() in amd_clear_divider()

Posted by Uros Bizjak 9 months ago

On Sat, Mar 15, 2025 at 10:37 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sat, Mar 15, 2025 at 10:29:25PM +0100, Ingo Molnar wrote:
> > So why does the higher level alternative_input() API exist? If it
> > shouldn't exist then we should remove it. If it exists, we should use
> > it consistently instead of open-coding its equivalent.
> >
> > Cleanups like this, especially if they are clearly part of an effort to
> > improve x86 code generation in this area, are not 'code churn', why
> > would they be?
>
> Because this is not improving anything, IMO. It is simply writing it
> differently, perhaps obscuring it more in the process.
>
> And I, just like hpa, would need to go look at alternative_input() to figure
> out what really happens there.
>
> Dunno, maybe we should really remove alternative_input() instead...

Please note that all other functions involving ALTERNATIVE in
asm/processor.h use "alternative" API.

Uros.