[PATCH v4] hwmon: (dell-smm) Improve assembly code

[PATCH v4] hwmon: (dell-smm) Improve assembly code

[Armin Wolf]

The new assembly code works on both 32 bit and 64 bit
cpus and allows for more compiler optimisations.
Since clang runs out of registers on 32 bit x86 when
using CC_OUT, we need to execute "setc" ourself.
Also modify the debug message so we can still see
the result (eax) when the carry flag was set.

Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Changes in v4:
- reword commit message

Changes in v3:
- make carry an unsigned char
- use "+a", ... for output registers
- drop "cc" from clobbered list

Changes in v2:
- fix clang running out of registers on 32 bit x86
- modify debug message
---
 drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
 1 file changed, 18 insertions(+), 60 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c5939e68586d..38d23a8e83f2 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -119,7 +119,7 @@ struct smm_regs {
 	unsigned int edx;
 	unsigned int esi;
 	unsigned int edi;
-} __packed;
+};

 static const char * const temp_labels[] = {
 	"CPU",
@@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
 	struct smm_regs *regs = par;
 	int eax = regs->eax;
 	int ebx = regs->ebx;
+	unsigned char carry;
 	long long duration;
-	int rc;

 	/* SMM requires CPU 0 */
 	if (smp_processor_id() != 0)
 		return -EBUSY;

-#if defined(CONFIG_X86_64)
-	asm volatile("pushq %%rax\n\t"
-		"movl 0(%%rax),%%edx\n\t"
-		"pushq %%rdx\n\t"
-		"movl 4(%%rax),%%ebx\n\t"
-		"movl 8(%%rax),%%ecx\n\t"
-		"movl 12(%%rax),%%edx\n\t"
-		"movl 16(%%rax),%%esi\n\t"
-		"movl 20(%%rax),%%edi\n\t"
-		"popq %%rax\n\t"
-		"out %%al,$0xb2\n\t"
-		"out %%al,$0x84\n\t"
-		"xchgq %%rax,(%%rsp)\n\t"
-		"movl %%ebx,4(%%rax)\n\t"
-		"movl %%ecx,8(%%rax)\n\t"
-		"movl %%edx,12(%%rax)\n\t"
-		"movl %%esi,16(%%rax)\n\t"
-		"movl %%edi,20(%%rax)\n\t"
-		"popq %%rdx\n\t"
-		"movl %%edx,0(%%rax)\n\t"
-		"pushfq\n\t"
-		"popq %%rax\n\t"
-		"andl $1,%%eax\n"
-		: "=a"(rc)
-		:    "a"(regs)
-		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#else
-	asm volatile("pushl %%eax\n\t"
-	    "movl 0(%%eax),%%edx\n\t"
-	    "push %%edx\n\t"
-	    "movl 4(%%eax),%%ebx\n\t"
-	    "movl 8(%%eax),%%ecx\n\t"
-	    "movl 12(%%eax),%%edx\n\t"
-	    "movl 16(%%eax),%%esi\n\t"
-	    "movl 20(%%eax),%%edi\n\t"
-	    "popl %%eax\n\t"
-	    "out %%al,$0xb2\n\t"
-	    "out %%al,$0x84\n\t"
-	    "xchgl %%eax,(%%esp)\n\t"
-	    "movl %%ebx,4(%%eax)\n\t"
-	    "movl %%ecx,8(%%eax)\n\t"
-	    "movl %%edx,12(%%eax)\n\t"
-	    "movl %%esi,16(%%eax)\n\t"
-	    "movl %%edi,20(%%eax)\n\t"
-	    "popl %%edx\n\t"
-	    "movl %%edx,0(%%eax)\n\t"
-	    "lahf\n\t"
-	    "shrl $8,%%eax\n\t"
-	    "andl $1,%%eax\n"
-	    : "=a"(rc)
-	    :    "a"(regs)
-	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#endif
-	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
-		rc = -EINVAL;
+	asm volatile("out %%al,$0xb2\n\t"
+		     "out %%al,$0x84\n\t"
+		     "setc %0\n"
+		     : "=mr" (carry),
+		       "+a" (regs->eax),
+		       "+b" (regs->ebx),
+		       "+c" (regs->ecx),
+		       "+d" (regs->edx),
+		       "+S" (regs->esi),
+		       "+D" (regs->edi));

 	duration = ktime_us_delta(ktime_get(), calltime);
-	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
-		 (rc ? 0xffff : regs->eax & 0xffff), duration);
+	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
+		 eax, ebx, regs->eax & 0xffff, carry, duration);

-	return rc;
+	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		return -EINVAL;
+
+	return 0;
 }

 /*
--
2.30.2

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Guenter Roeck]

On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> The new assembly code works on both 32 bit and 64 bit
> cpus and allows for more compiler optimisations.
> Since clang runs out of registers on 32 bit x86 when
> using CC_OUT, we need to execute "setc" ourself.
> Also modify the debug message so we can still see
> the result (eax) when the carry flag was set.
> 
> Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

It would be great if I can get some Tested-by/Acked-by/Reviewed-by
tags for this patch.

Thanks,
Guenter

> ---
> Changes in v4:
> - reword commit message
> 
> Changes in v3:
> - make carry an unsigned char
> - use "+a", ... for output registers
> - drop "cc" from clobbered list
> 
> Changes in v2:
> - fix clang running out of registers on 32 bit x86
> - modify debug message
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
>  1 file changed, 18 insertions(+), 60 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index c5939e68586d..38d23a8e83f2 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -119,7 +119,7 @@ struct smm_regs {
>  	unsigned int edx;
>  	unsigned int esi;
>  	unsigned int edi;
> -} __packed;
> +};
> 
>  static const char * const temp_labels[] = {
>  	"CPU",
> @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
>  	struct smm_regs *regs = par;
>  	int eax = regs->eax;
>  	int ebx = regs->ebx;
> +	unsigned char carry;
>  	long long duration;
> -	int rc;
> 
>  	/* SMM requires CPU 0 */
>  	if (smp_processor_id() != 0)
>  		return -EBUSY;
> 
> -#if defined(CONFIG_X86_64)
> -	asm volatile("pushq %%rax\n\t"
> -		"movl 0(%%rax),%%edx\n\t"
> -		"pushq %%rdx\n\t"
> -		"movl 4(%%rax),%%ebx\n\t"
> -		"movl 8(%%rax),%%ecx\n\t"
> -		"movl 12(%%rax),%%edx\n\t"
> -		"movl 16(%%rax),%%esi\n\t"
> -		"movl 20(%%rax),%%edi\n\t"
> -		"popq %%rax\n\t"
> -		"out %%al,$0xb2\n\t"
> -		"out %%al,$0x84\n\t"
> -		"xchgq %%rax,(%%rsp)\n\t"
> -		"movl %%ebx,4(%%rax)\n\t"
> -		"movl %%ecx,8(%%rax)\n\t"
> -		"movl %%edx,12(%%rax)\n\t"
> -		"movl %%esi,16(%%rax)\n\t"
> -		"movl %%edi,20(%%rax)\n\t"
> -		"popq %%rdx\n\t"
> -		"movl %%edx,0(%%rax)\n\t"
> -		"pushfq\n\t"
> -		"popq %%rax\n\t"
> -		"andl $1,%%eax\n"
> -		: "=a"(rc)
> -		:    "a"(regs)
> -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#else
> -	asm volatile("pushl %%eax\n\t"
> -	    "movl 0(%%eax),%%edx\n\t"
> -	    "push %%edx\n\t"
> -	    "movl 4(%%eax),%%ebx\n\t"
> -	    "movl 8(%%eax),%%ecx\n\t"
> -	    "movl 12(%%eax),%%edx\n\t"
> -	    "movl 16(%%eax),%%esi\n\t"
> -	    "movl 20(%%eax),%%edi\n\t"
> -	    "popl %%eax\n\t"
> -	    "out %%al,$0xb2\n\t"
> -	    "out %%al,$0x84\n\t"
> -	    "xchgl %%eax,(%%esp)\n\t"
> -	    "movl %%ebx,4(%%eax)\n\t"
> -	    "movl %%ecx,8(%%eax)\n\t"
> -	    "movl %%edx,12(%%eax)\n\t"
> -	    "movl %%esi,16(%%eax)\n\t"
> -	    "movl %%edi,20(%%eax)\n\t"
> -	    "popl %%edx\n\t"
> -	    "movl %%edx,0(%%eax)\n\t"
> -	    "lahf\n\t"
> -	    "shrl $8,%%eax\n\t"
> -	    "andl $1,%%eax\n"
> -	    : "=a"(rc)
> -	    :    "a"(regs)
> -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#endif
> -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> -		rc = -EINVAL;
> +	asm volatile("out %%al,$0xb2\n\t"
> +		     "out %%al,$0x84\n\t"
> +		     "setc %0\n"
> +		     : "=mr" (carry),
> +		       "+a" (regs->eax),
> +		       "+b" (regs->ebx),
> +		       "+c" (regs->ecx),
> +		       "+d" (regs->edx),
> +		       "+S" (regs->esi),
> +		       "+D" (regs->edi));
> 
>  	duration = ktime_us_delta(ktime_get(), calltime);
> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> 
> -	return rc;
> +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> +		return -EINVAL;
> +
> +	return 0;
>  }
> 
>  /*

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Pali Rohár]

On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
> On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> > The new assembly code works on both 32 bit and 64 bit
> > cpus and allows for more compiler optimisations.
> > Since clang runs out of registers on 32 bit x86 when
> > using CC_OUT, we need to execute "setc" ourself.
> > Also modify the debug message so we can still see
> > the result (eax) when the carry flag was set.
> > 
> > Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> > 
> > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> 
> It would be great if I can get some Tested-by/Acked-by/Reviewed-by
> tags for this patch.

Well, I know about this driver asm code for a long time and it since
beginning it was suspicious for me, why there is such huge code with
stack and registers manipulation and why it cannot be implemented easily
via just two "out" instructions. This patch is exactly doing it.
But question reminds why it was written in this simple way since
beginning.

If this change is correct then I have no problem with it.

But I would rather see review of this asm change by skilled x86 asm
developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
stack manipulation in inline gcc asm which sounds like a trap. And I'm
not feeling skilled for reviewing this change.

May I ask somebody to review this change? Is there some linux x86 ML?

> Thanks,
> Guenter
> 
> > ---
> > Changes in v4:
> > - reword commit message
> > 
> > Changes in v3:
> > - make carry an unsigned char
> > - use "+a", ... for output registers
> > - drop "cc" from clobbered list
> > 
> > Changes in v2:
> > - fix clang running out of registers on 32 bit x86
> > - modify debug message
> > ---
> >  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
> >  1 file changed, 18 insertions(+), 60 deletions(-)
> > 
> > --
> > 2.30.2
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index c5939e68586d..38d23a8e83f2 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -119,7 +119,7 @@ struct smm_regs {
> >  	unsigned int edx;
> >  	unsigned int esi;
> >  	unsigned int edi;
> > -} __packed;
> > +};
> > 
> >  static const char * const temp_labels[] = {
> >  	"CPU",
> > @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
> >  	struct smm_regs *regs = par;
> >  	int eax = regs->eax;
> >  	int ebx = regs->ebx;
> > +	unsigned char carry;
> >  	long long duration;
> > -	int rc;
> > 
> >  	/* SMM requires CPU 0 */
> >  	if (smp_processor_id() != 0)
> >  		return -EBUSY;
> > 
> > -#if defined(CONFIG_X86_64)
> > -	asm volatile("pushq %%rax\n\t"
> > -		"movl 0(%%rax),%%edx\n\t"
> > -		"pushq %%rdx\n\t"
> > -		"movl 4(%%rax),%%ebx\n\t"
> > -		"movl 8(%%rax),%%ecx\n\t"
> > -		"movl 12(%%rax),%%edx\n\t"
> > -		"movl 16(%%rax),%%esi\n\t"
> > -		"movl 20(%%rax),%%edi\n\t"
> > -		"popq %%rax\n\t"
> > -		"out %%al,$0xb2\n\t"
> > -		"out %%al,$0x84\n\t"
> > -		"xchgq %%rax,(%%rsp)\n\t"
> > -		"movl %%ebx,4(%%rax)\n\t"
> > -		"movl %%ecx,8(%%rax)\n\t"
> > -		"movl %%edx,12(%%rax)\n\t"
> > -		"movl %%esi,16(%%rax)\n\t"
> > -		"movl %%edi,20(%%rax)\n\t"
> > -		"popq %%rdx\n\t"
> > -		"movl %%edx,0(%%rax)\n\t"
> > -		"pushfq\n\t"
> > -		"popq %%rax\n\t"
> > -		"andl $1,%%eax\n"
> > -		: "=a"(rc)
> > -		:    "a"(regs)
> > -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > -#else
> > -	asm volatile("pushl %%eax\n\t"
> > -	    "movl 0(%%eax),%%edx\n\t"
> > -	    "push %%edx\n\t"
> > -	    "movl 4(%%eax),%%ebx\n\t"
> > -	    "movl 8(%%eax),%%ecx\n\t"
> > -	    "movl 12(%%eax),%%edx\n\t"
> > -	    "movl 16(%%eax),%%esi\n\t"
> > -	    "movl 20(%%eax),%%edi\n\t"
> > -	    "popl %%eax\n\t"
> > -	    "out %%al,$0xb2\n\t"
> > -	    "out %%al,$0x84\n\t"
> > -	    "xchgl %%eax,(%%esp)\n\t"
> > -	    "movl %%ebx,4(%%eax)\n\t"
> > -	    "movl %%ecx,8(%%eax)\n\t"
> > -	    "movl %%edx,12(%%eax)\n\t"
> > -	    "movl %%esi,16(%%eax)\n\t"
> > -	    "movl %%edi,20(%%eax)\n\t"
> > -	    "popl %%edx\n\t"
> > -	    "movl %%edx,0(%%eax)\n\t"
> > -	    "lahf\n\t"
> > -	    "shrl $8,%%eax\n\t"
> > -	    "andl $1,%%eax\n"
> > -	    : "=a"(rc)
> > -	    :    "a"(regs)
> > -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > -#endif
> > -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > -		rc = -EINVAL;
> > +	asm volatile("out %%al,$0xb2\n\t"
> > +		     "out %%al,$0x84\n\t"
> > +		     "setc %0\n"
> > +		     : "=mr" (carry),
> > +		       "+a" (regs->eax),
> > +		       "+b" (regs->ebx),
> > +		       "+c" (regs->ecx),
> > +		       "+d" (regs->edx),
> > +		       "+S" (regs->esi),
> > +		       "+D" (regs->edi));
> > 
> >  	duration = ktime_us_delta(ktime_get(), calltime);
> > -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> > -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> > +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> > +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> > 
> > -	return rc;
> > +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > +		return -EINVAL;
> > +
> > +	return 0;
> >  }
> > 
> >  /*

RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[David Laight]

From: Pali Rohár
> Sent: 22 February 2022 17:52
> 
> On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
> > On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> > > The new assembly code works on both 32 bit and 64 bit
> > > cpus and allows for more compiler optimisations.
> > > Since clang runs out of registers on 32 bit x86 when
> > > using CC_OUT, we need to execute "setc" ourself.
> > > Also modify the debug message so we can still see
> > > the result (eax) when the carry flag was set.
> > >
> > > Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> > >
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> >
> > It would be great if I can get some Tested-by/Acked-by/Reviewed-by
> > tags for this patch.
> 
> Well, I know about this driver asm code for a long time and it since
> beginning it was suspicious for me, why there is such huge code with
> stack and registers manipulation and why it cannot be implemented easily
> via just two "out" instructions. This patch is exactly doing it.
> But question reminds why it was written in this simple way since
> beginning.
> 
> If this change is correct then I have no problem with it.
> 
> But I would rather see review of this asm change by skilled x86 asm
> developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
> stack manipulation in inline gcc asm which sounds like a trap. And I'm
> not feeling skilled for reviewing this change.
> 
> May I ask somebody to review this change? Is there some linux x86 ML?

On the face of it the new asm code is basically the same as the old
asm code - and both are probably equivalent to the reverse engineered
windows code.

The real problem isn't the new code, it is more 'WTF is actually going on'.
Somehow the pair of outb are generating a synchronous (or synchronous
enough) trap that the trap handler (in the smm code in the bios)
can change the registers before the code following the outb saves
them back to memory.

SMM mode is a semi-hidden cpu mode usually entered by an interrupt.
It is horrid and nasty because it has access to the cpu memory and
registers, but is hidden and unknown (I think in the bios).

Normal bios calls can (usually) be executed using a cpu emulator
(especially useful for doing real-mode call from 64bit mode) so
the kernel can limit what they can do.

But SMM mode is nearly as bad as the ME processor on newer systems.
If you worry about security you really want it disabled - but you can't.

As far as this patch goes I think I'd add a 'stc' (set carry)
instruction before the first 'outb'.
Then if the 'something magic happens here' doesn't happen (because
you aren't on the right kind of motherboard) the action fails.

	David

> 
> > Thanks,
> > Guenter
> >
> > > ---
> > > Changes in v4:
> > > - reword commit message
> > >
> > > Changes in v3:
> > > - make carry an unsigned char
> > > - use "+a", ... for output registers
> > > - drop "cc" from clobbered list
> > >
> > > Changes in v2:
> > > - fix clang running out of registers on 32 bit x86
> > > - modify debug message
> > > ---
> > >  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
> > >  1 file changed, 18 insertions(+), 60 deletions(-)
> > >
> > > --
> > > 2.30.2
> > >
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index c5939e68586d..38d23a8e83f2 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -119,7 +119,7 @@ struct smm_regs {
> > >  	unsigned int edx;
> > >  	unsigned int esi;
> > >  	unsigned int edi;
> > > -} __packed;
> > > +};
> > >
> > >  static const char * const temp_labels[] = {
> > >  	"CPU",
> > > @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
> > >  	struct smm_regs *regs = par;
> > >  	int eax = regs->eax;
> > >  	int ebx = regs->ebx;
> > > +	unsigned char carry;
> > >  	long long duration;
> > > -	int rc;
> > >
> > >  	/* SMM requires CPU 0 */
> > >  	if (smp_processor_id() != 0)
> > >  		return -EBUSY;
> > >
> > > -#if defined(CONFIG_X86_64)
> > > -	asm volatile("pushq %%rax\n\t"
> > > -		"movl 0(%%rax),%%edx\n\t"
> > > -		"pushq %%rdx\n\t"
> > > -		"movl 4(%%rax),%%ebx\n\t"
> > > -		"movl 8(%%rax),%%ecx\n\t"
> > > -		"movl 12(%%rax),%%edx\n\t"
> > > -		"movl 16(%%rax),%%esi\n\t"
> > > -		"movl 20(%%rax),%%edi\n\t"
> > > -		"popq %%rax\n\t"
> > > -		"out %%al,$0xb2\n\t"
> > > -		"out %%al,$0x84\n\t"
> > > -		"xchgq %%rax,(%%rsp)\n\t"
> > > -		"movl %%ebx,4(%%rax)\n\t"
> > > -		"movl %%ecx,8(%%rax)\n\t"
> > > -		"movl %%edx,12(%%rax)\n\t"
> > > -		"movl %%esi,16(%%rax)\n\t"
> > > -		"movl %%edi,20(%%rax)\n\t"
> > > -		"popq %%rdx\n\t"
> > > -		"movl %%edx,0(%%rax)\n\t"
> > > -		"pushfq\n\t"
> > > -		"popq %%rax\n\t"
> > > -		"andl $1,%%eax\n"
> > > -		: "=a"(rc)
> > > -		:    "a"(regs)
> > > -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > > -#else
> > > -	asm volatile("pushl %%eax\n\t"
> > > -	    "movl 0(%%eax),%%edx\n\t"
> > > -	    "push %%edx\n\t"
> > > -	    "movl 4(%%eax),%%ebx\n\t"
> > > -	    "movl 8(%%eax),%%ecx\n\t"
> > > -	    "movl 12(%%eax),%%edx\n\t"
> > > -	    "movl 16(%%eax),%%esi\n\t"
> > > -	    "movl 20(%%eax),%%edi\n\t"
> > > -	    "popl %%eax\n\t"
> > > -	    "out %%al,$0xb2\n\t"
> > > -	    "out %%al,$0x84\n\t"
> > > -	    "xchgl %%eax,(%%esp)\n\t"
> > > -	    "movl %%ebx,4(%%eax)\n\t"
> > > -	    "movl %%ecx,8(%%eax)\n\t"
> > > -	    "movl %%edx,12(%%eax)\n\t"
> > > -	    "movl %%esi,16(%%eax)\n\t"
> > > -	    "movl %%edi,20(%%eax)\n\t"
> > > -	    "popl %%edx\n\t"
> > > -	    "movl %%edx,0(%%eax)\n\t"
> > > -	    "lahf\n\t"
> > > -	    "shrl $8,%%eax\n\t"
> > > -	    "andl $1,%%eax\n"
> > > -	    : "=a"(rc)
> > > -	    :    "a"(regs)
> > > -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > > -#endif
> > > -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > > -		rc = -EINVAL;
> > > +	asm volatile("out %%al,$0xb2\n\t"
> > > +		     "out %%al,$0x84\n\t"
> > > +		     "setc %0\n"
> > > +		     : "=mr" (carry),
> > > +		       "+a" (regs->eax),
> > > +		       "+b" (regs->ebx),
> > > +		       "+c" (regs->ecx),
> > > +		       "+d" (regs->edx),
> > > +		       "+S" (regs->esi),
> > > +		       "+D" (regs->edi));
> > >
> > >  	duration = ktime_us_delta(ktime_get(), calltime);
> > > -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> > > -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> > > +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> > > +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> > >
> > > -	return rc;
> > > +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  /*

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[David Laight]

From: Pali Rohár
> Sent: 22 February 2022 17:52
> 
> On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
> > On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> > > The new assembly code works on both 32 bit and 64 bit
> > > cpus and allows for more compiler optimisations.
> > > Since clang runs out of registers on 32 bit x86 when
> > > using CC_OUT, we need to execute "setc" ourself.
> > > Also modify the debug message so we can still see
> > > the result (eax) when the carry flag was set.
> > >
> > > Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> > >
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> >
> > It would be great if I can get some Tested-by/Acked-by/Reviewed-by
> > tags for this patch.
> 
> Well, I know about this driver asm code for a long time and it since
> beginning it was suspicious for me, why there is such huge code with
> stack and registers manipulation and why it cannot be implemented easily
> via just two "out" instructions. This patch is exactly doing it.
> But question reminds why it was written in this simple way since
> beginning.
> 
> If this change is correct then I have no problem with it.
> 
> But I would rather see review of this asm change by skilled x86 asm
> developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
> stack manipulation in inline gcc asm which sounds like a trap. And I'm
> not feeling skilled for reviewing this change.
> 
> May I ask somebody to review this change? Is there some linux x86 ML?

On the face of it the new asm code is basically the same as the old
asm code - and both are probably equivalent to the reverse engineered
windows code.

The real problem isn't the new code, it is more 'WTF is actually going on'.
Somehow the pair of outb are generating a synchronous (or synchronous
enough) trap that the trap handler (in the smm code in the bios)
can change the registers before the code following the outb saves
them back to memory.

SMM mode is a semi-hidden cpu mode usually entered by an interrupt.
It is horrid and nasty because it has access to the cpu memory and
registers, but is hidden and unknown (I think in the bios).

Normal bios calls can (usually) be executed using a cpu emulator
(especially useful for doing real-mode call from 64bit mode) so
the kernel can limit what they can do.

But SMM mode is nearly as bad as the ME processor on newer systems.
If you worry about security you really want it disabled - but you can't.

As far as this patch goes I think I'd add a 'stc' (set carry)
instruction before the first 'outb'.
Then if the 'something magic happens here' doesn't happen (because
you aren't on the right kind of motherboard) the action fails.

	David

> 
> > Thanks,
> > Guenter
> >
> > > ---
> > > Changes in v4:
> > > - reword commit message
> > >
> > > Changes in v3:
> > > - make carry an unsigned char
> > > - use "+a", ... for output registers
> > > - drop "cc" from clobbered list
> > >
> > > Changes in v2:
> > > - fix clang running out of registers on 32 bit x86
> > > - modify debug message
> > > ---
> > >  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
> > >  1 file changed, 18 insertions(+), 60 deletions(-)
> > >
> > > --
> > > 2.30.2
> > >
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index c5939e68586d..38d23a8e83f2 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -119,7 +119,7 @@ struct smm_regs {
> > >  	unsigned int edx;
> > >  	unsigned int esi;
> > >  	unsigned int edi;
> > > -} __packed;
> > > +};
> > >
> > >  static const char * const temp_labels[] = {
> > >  	"CPU",
> > > @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
> > >  	struct smm_regs *regs = par;
> > >  	int eax = regs->eax;
> > >  	int ebx = regs->ebx;
> > > +	unsigned char carry;
> > >  	long long duration;
> > > -	int rc;
> > >
> > >  	/* SMM requires CPU 0 */
> > >  	if (smp_processor_id() != 0)
> > >  		return -EBUSY;
> > >
> > > -#if defined(CONFIG_X86_64)
> > > -	asm volatile("pushq %%rax\n\t"
> > > -		"movl 0(%%rax),%%edx\n\t"
> > > -		"pushq %%rdx\n\t"
> > > -		"movl 4(%%rax),%%ebx\n\t"
> > > -		"movl 8(%%rax),%%ecx\n\t"
> > > -		"movl 12(%%rax),%%edx\n\t"
> > > -		"movl 16(%%rax),%%esi\n\t"
> > > -		"movl 20(%%rax),%%edi\n\t"
> > > -		"popq %%rax\n\t"
> > > -		"out %%al,$0xb2\n\t"
> > > -		"out %%al,$0x84\n\t"
> > > -		"xchgq %%rax,(%%rsp)\n\t"
> > > -		"movl %%ebx,4(%%rax)\n\t"
> > > -		"movl %%ecx,8(%%rax)\n\t"
> > > -		"movl %%edx,12(%%rax)\n\t"
> > > -		"movl %%esi,16(%%rax)\n\t"
> > > -		"movl %%edi,20(%%rax)\n\t"
> > > -		"popq %%rdx\n\t"
> > > -		"movl %%edx,0(%%rax)\n\t"
> > > -		"pushfq\n\t"
> > > -		"popq %%rax\n\t"
> > > -		"andl $1,%%eax\n"
> > > -		: "=a"(rc)
> > > -		:    "a"(regs)
> > > -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > > -#else
> > > -	asm volatile("pushl %%eax\n\t"
> > > -	    "movl 0(%%eax),%%edx\n\t"
> > > -	    "push %%edx\n\t"
> > > -	    "movl 4(%%eax),%%ebx\n\t"
> > > -	    "movl 8(%%eax),%%ecx\n\t"
> > > -	    "movl 12(%%eax),%%edx\n\t"
> > > -	    "movl 16(%%eax),%%esi\n\t"
> > > -	    "movl 20(%%eax),%%edi\n\t"
> > > -	    "popl %%eax\n\t"
> > > -	    "out %%al,$0xb2\n\t"
> > > -	    "out %%al,$0x84\n\t"
> > > -	    "xchgl %%eax,(%%esp)\n\t"
> > > -	    "movl %%ebx,4(%%eax)\n\t"
> > > -	    "movl %%ecx,8(%%eax)\n\t"
> > > -	    "movl %%edx,12(%%eax)\n\t"
> > > -	    "movl %%esi,16(%%eax)\n\t"
> > > -	    "movl %%edi,20(%%eax)\n\t"
> > > -	    "popl %%edx\n\t"
> > > -	    "movl %%edx,0(%%eax)\n\t"
> > > -	    "lahf\n\t"
> > > -	    "shrl $8,%%eax\n\t"
> > > -	    "andl $1,%%eax\n"
> > > -	    : "=a"(rc)
> > > -	    :    "a"(regs)
> > > -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> > > -#endif
> > > -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > > -		rc = -EINVAL;
> > > +	asm volatile("out %%al,$0xb2\n\t"
> > > +		     "out %%al,$0x84\n\t"
> > > +		     "setc %0\n"
> > > +		     : "=mr" (carry),
> > > +		       "+a" (regs->eax),
> > > +		       "+b" (regs->ebx),
> > > +		       "+c" (regs->ecx),
> > > +		       "+d" (regs->edx),
> > > +		       "+S" (regs->esi),
> > > +		       "+D" (regs->edi));
> > >
> > >  	duration = ktime_us_delta(ktime_get(), calltime);
> > > -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> > > -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> > > +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> > > +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> > >
> > > -	return rc;
> > > +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  /*

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Armin Wolf]

Am 22.02.22 um 20:46 schrieb David Laight:

> From: Pali Rohár
>> Sent: 22 February 2022 17:52
>>
>> On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
>>> On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
>>>> The new assembly code works on both 32 bit and 64 bit
>>>> cpus and allows for more compiler optimisations.
>>>> Since clang runs out of registers on 32 bit x86 when
>>>> using CC_OUT, we need to execute "setc" ourself.
>>>> Also modify the debug message so we can still see
>>>> the result (eax) when the carry flag was set.
>>>>
>>>> Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> It would be great if I can get some Tested-by/Acked-by/Reviewed-by
>>> tags for this patch.
>> Well, I know about this driver asm code for a long time and it since
>> beginning it was suspicious for me, why there is such huge code with
>> stack and registers manipulation and why it cannot be implemented easily
>> via just two "out" instructions. This patch is exactly doing it.
>> But question reminds why it was written in this simple way since
>> beginning.
>>
>> If this change is correct then I have no problem with it.
>>
>> But I would rather see review of this asm change by skilled x86 asm
>> developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
>> stack manipulation in inline gcc asm which sounds like a trap. And I'm
>> not feeling skilled for reviewing this change.
>>
>> May I ask somebody to review this change? Is there some linux x86 ML?
> On the face of it the new asm code is basically the same as the old
> asm code - and both are probably equivalent to the reverse engineered
> windows code.
>
> The real problem isn't the new code, it is more 'WTF is actually going on'.
> Somehow the pair of outb are generating a synchronous (or synchronous
> enough) trap that the trap handler (in the smm code in the bios)
> can change the registers before the code following the outb saves
> them back to memory.
>
> SMM mode is a semi-hidden cpu mode usually entered by an interrupt.
> It is horrid and nasty because it has access to the cpu memory and
> registers, but is hidden and unknown (I think in the bios).
>
> Normal bios calls can (usually) be executed using a cpu emulator
> (especially useful for doing real-mode call from 64bit mode) so
> the kernel can limit what they can do.
>
> But SMM mode is nearly as bad as the ME processor on newer systems.
> If you worry about security you really want it disabled - but you can't.
>
> As far as this patch goes I think I'd add a 'stc' (set carry)
> instruction before the first 'outb'.
> Then if the 'something magic happens here' doesn't happen (because
> you aren't on the right kind of motherboard) the action fails.
>
> 	David

We already check for such scenarios by checking if eax changed.
I would not set the carry flag before doing a SMM call since im
not sure if the firmware code always clears the carry flag when
the call was successful.
If for example the firmware code only sets the carry flag on
error and otherwise ignores the carry flag, we might create some
false negatives when a successful SMM call leaves the carry flag set.

>>> Thanks,
>>> Guenter
>>>
>>>> ---
>>>> Changes in v4:
>>>> - reword commit message
>>>>
>>>> Changes in v3:
>>>> - make carry an unsigned char
>>>> - use "+a", ... for output registers
>>>> - drop "cc" from clobbered list
>>>>
>>>> Changes in v2:
>>>> - fix clang running out of registers on 32 bit x86
>>>> - modify debug message
>>>> ---
>>>>   drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
>>>>   1 file changed, 18 insertions(+), 60 deletions(-)
>>>>
>>>> --
>>>> 2.30.2
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>>> index c5939e68586d..38d23a8e83f2 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -119,7 +119,7 @@ struct smm_regs {
>>>>   	unsigned int edx;
>>>>   	unsigned int esi;
>>>>   	unsigned int edi;
>>>> -} __packed;
>>>> +};
>>>>
>>>>   static const char * const temp_labels[] = {
>>>>   	"CPU",
>>>> @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
>>>>   	struct smm_regs *regs = par;
>>>>   	int eax = regs->eax;
>>>>   	int ebx = regs->ebx;
>>>> +	unsigned char carry;
>>>>   	long long duration;
>>>> -	int rc;
>>>>
>>>>   	/* SMM requires CPU 0 */
>>>>   	if (smp_processor_id() != 0)
>>>>   		return -EBUSY;
>>>>
>>>> -#if defined(CONFIG_X86_64)
>>>> -	asm volatile("pushq %%rax\n\t"
>>>> -		"movl 0(%%rax),%%edx\n\t"
>>>> -		"pushq %%rdx\n\t"
>>>> -		"movl 4(%%rax),%%ebx\n\t"
>>>> -		"movl 8(%%rax),%%ecx\n\t"
>>>> -		"movl 12(%%rax),%%edx\n\t"
>>>> -		"movl 16(%%rax),%%esi\n\t"
>>>> -		"movl 20(%%rax),%%edi\n\t"
>>>> -		"popq %%rax\n\t"
>>>> -		"out %%al,$0xb2\n\t"
>>>> -		"out %%al,$0x84\n\t"
>>>> -		"xchgq %%rax,(%%rsp)\n\t"
>>>> -		"movl %%ebx,4(%%rax)\n\t"
>>>> -		"movl %%ecx,8(%%rax)\n\t"
>>>> -		"movl %%edx,12(%%rax)\n\t"
>>>> -		"movl %%esi,16(%%rax)\n\t"
>>>> -		"movl %%edi,20(%%rax)\n\t"
>>>> -		"popq %%rdx\n\t"
>>>> -		"movl %%edx,0(%%rax)\n\t"
>>>> -		"pushfq\n\t"
>>>> -		"popq %%rax\n\t"
>>>> -		"andl $1,%%eax\n"
>>>> -		: "=a"(rc)
>>>> -		:    "a"(regs)
>>>> -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
>>>> -#else
>>>> -	asm volatile("pushl %%eax\n\t"
>>>> -	    "movl 0(%%eax),%%edx\n\t"
>>>> -	    "push %%edx\n\t"
>>>> -	    "movl 4(%%eax),%%ebx\n\t"
>>>> -	    "movl 8(%%eax),%%ecx\n\t"
>>>> -	    "movl 12(%%eax),%%edx\n\t"
>>>> -	    "movl 16(%%eax),%%esi\n\t"
>>>> -	    "movl 20(%%eax),%%edi\n\t"
>>>> -	    "popl %%eax\n\t"
>>>> -	    "out %%al,$0xb2\n\t"
>>>> -	    "out %%al,$0x84\n\t"
>>>> -	    "xchgl %%eax,(%%esp)\n\t"
>>>> -	    "movl %%ebx,4(%%eax)\n\t"
>>>> -	    "movl %%ecx,8(%%eax)\n\t"
>>>> -	    "movl %%edx,12(%%eax)\n\t"
>>>> -	    "movl %%esi,16(%%eax)\n\t"
>>>> -	    "movl %%edi,20(%%eax)\n\t"
>>>> -	    "popl %%edx\n\t"
>>>> -	    "movl %%edx,0(%%eax)\n\t"
>>>> -	    "lahf\n\t"
>>>> -	    "shrl $8,%%eax\n\t"
>>>> -	    "andl $1,%%eax\n"
>>>> -	    : "=a"(rc)
>>>> -	    :    "a"(regs)
>>>> -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
>>>> -#endif
>>>> -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>>>> -		rc = -EINVAL;
>>>> +	asm volatile("out %%al,$0xb2\n\t"
>>>> +		     "out %%al,$0x84\n\t"
>>>> +		     "setc %0\n"
>>>> +		     : "=mr" (carry),
>>>> +		       "+a" (regs->eax),
>>>> +		       "+b" (regs->ebx),
>>>> +		       "+c" (regs->ecx),
>>>> +		       "+d" (regs->edx),
>>>> +		       "+S" (regs->esi),
>>>> +		       "+D" (regs->edi));
>>>>
>>>>   	duration = ktime_us_delta(ktime_get(), calltime);
>>>> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
>>>> -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
>>>> +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
>>>> +		 eax, ebx, regs->eax & 0xffff, carry, duration);
>>>>
>>>> -	return rc;
>>>> +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return 0;
>>>>   }
>>>>
>>>>   /*
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Armin Wolf]

Am 22.02.22 um 20:46 schrieb David Laight:

> From: Pali Rohár
>> Sent: 22 February 2022 17:52
>>
>> On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
>>> On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
>>>> The new assembly code works on both 32 bit and 64 bit
>>>> cpus and allows for more compiler optimisations.
>>>> Since clang runs out of registers on 32 bit x86 when
>>>> using CC_OUT, we need to execute "setc" ourself.
>>>> Also modify the debug message so we can still see
>>>> the result (eax) when the carry flag was set.
>>>>
>>>> Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> It would be great if I can get some Tested-by/Acked-by/Reviewed-by
>>> tags for this patch.
>> Well, I know about this driver asm code for a long time and it since
>> beginning it was suspicious for me, why there is such huge code with
>> stack and registers manipulation and why it cannot be implemented easily
>> via just two "out" instructions. This patch is exactly doing it.
>> But question reminds why it was written in this simple way since
>> beginning.
>>
>> If this change is correct then I have no problem with it.
>>
>> But I would rather see review of this asm change by skilled x86 asm
>> developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
>> stack manipulation in inline gcc asm which sounds like a trap. And I'm
>> not feeling skilled for reviewing this change.
>>
>> May I ask somebody to review this change? Is there some linux x86 ML?
> On the face of it the new asm code is basically the same as the old
> asm code - and both are probably equivalent to the reverse engineered
> windows code.
>
> The real problem isn't the new code, it is more 'WTF is actually going on'.
> Somehow the pair of outb are generating a synchronous (or synchronous
> enough) trap that the trap handler (in the smm code in the bios)
> can change the registers before the code following the outb saves
> them back to memory.
>
> SMM mode is a semi-hidden cpu mode usually entered by an interrupt.
> It is horrid and nasty because it has access to the cpu memory and
> registers, but is hidden and unknown (I think in the bios).
>
> Normal bios calls can (usually) be executed using a cpu emulator
> (especially useful for doing real-mode call from 64bit mode) so
> the kernel can limit what they can do.
>
> But SMM mode is nearly as bad as the ME processor on newer systems.
> If you worry about security you really want it disabled - but you can't.
>
> As far as this patch goes I think I'd add a 'stc' (set carry)
> instruction before the first 'outb'.
> Then if the 'something magic happens here' doesn't happen (because
> you aren't on the right kind of motherboard) the action fails.
>
> 	David

We already check for such scenarios by checking if eax changed.
I would not set the carry flag before doing a SMM call since im
not sure if the firmware code always clears the carry flag when
the call was successful.
If for example the firmware code only sets the carry flag on
error and otherwise ignores the carry flag, we might create some
false negatives when a successful SMM call leaves the carry flag set.

>>> Thanks,
>>> Guenter
>>>
>>>> ---
>>>> Changes in v4:
>>>> - reword commit message
>>>>
>>>> Changes in v3:
>>>> - make carry an unsigned char
>>>> - use "+a", ... for output registers
>>>> - drop "cc" from clobbered list
>>>>
>>>> Changes in v2:
>>>> - fix clang running out of registers on 32 bit x86
>>>> - modify debug message
>>>> ---
>>>>   drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
>>>>   1 file changed, 18 insertions(+), 60 deletions(-)
>>>>
>>>> --
>>>> 2.30.2
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>>> index c5939e68586d..38d23a8e83f2 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -119,7 +119,7 @@ struct smm_regs {
>>>>   	unsigned int edx;
>>>>   	unsigned int esi;
>>>>   	unsigned int edi;
>>>> -} __packed;
>>>> +};
>>>>
>>>>   static const char * const temp_labels[] = {
>>>>   	"CPU",
>>>> @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
>>>>   	struct smm_regs *regs = par;
>>>>   	int eax = regs->eax;
>>>>   	int ebx = regs->ebx;
>>>> +	unsigned char carry;
>>>>   	long long duration;
>>>> -	int rc;
>>>>
>>>>   	/* SMM requires CPU 0 */
>>>>   	if (smp_processor_id() != 0)
>>>>   		return -EBUSY;
>>>>
>>>> -#if defined(CONFIG_X86_64)
>>>> -	asm volatile("pushq %%rax\n\t"
>>>> -		"movl 0(%%rax),%%edx\n\t"
>>>> -		"pushq %%rdx\n\t"
>>>> -		"movl 4(%%rax),%%ebx\n\t"
>>>> -		"movl 8(%%rax),%%ecx\n\t"
>>>> -		"movl 12(%%rax),%%edx\n\t"
>>>> -		"movl 16(%%rax),%%esi\n\t"
>>>> -		"movl 20(%%rax),%%edi\n\t"
>>>> -		"popq %%rax\n\t"
>>>> -		"out %%al,$0xb2\n\t"
>>>> -		"out %%al,$0x84\n\t"
>>>> -		"xchgq %%rax,(%%rsp)\n\t"
>>>> -		"movl %%ebx,4(%%rax)\n\t"
>>>> -		"movl %%ecx,8(%%rax)\n\t"
>>>> -		"movl %%edx,12(%%rax)\n\t"
>>>> -		"movl %%esi,16(%%rax)\n\t"
>>>> -		"movl %%edi,20(%%rax)\n\t"
>>>> -		"popq %%rdx\n\t"
>>>> -		"movl %%edx,0(%%rax)\n\t"
>>>> -		"pushfq\n\t"
>>>> -		"popq %%rax\n\t"
>>>> -		"andl $1,%%eax\n"
>>>> -		: "=a"(rc)
>>>> -		:    "a"(regs)
>>>> -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
>>>> -#else
>>>> -	asm volatile("pushl %%eax\n\t"
>>>> -	    "movl 0(%%eax),%%edx\n\t"
>>>> -	    "push %%edx\n\t"
>>>> -	    "movl 4(%%eax),%%ebx\n\t"
>>>> -	    "movl 8(%%eax),%%ecx\n\t"
>>>> -	    "movl 12(%%eax),%%edx\n\t"
>>>> -	    "movl 16(%%eax),%%esi\n\t"
>>>> -	    "movl 20(%%eax),%%edi\n\t"
>>>> -	    "popl %%eax\n\t"
>>>> -	    "out %%al,$0xb2\n\t"
>>>> -	    "out %%al,$0x84\n\t"
>>>> -	    "xchgl %%eax,(%%esp)\n\t"
>>>> -	    "movl %%ebx,4(%%eax)\n\t"
>>>> -	    "movl %%ecx,8(%%eax)\n\t"
>>>> -	    "movl %%edx,12(%%eax)\n\t"
>>>> -	    "movl %%esi,16(%%eax)\n\t"
>>>> -	    "movl %%edi,20(%%eax)\n\t"
>>>> -	    "popl %%edx\n\t"
>>>> -	    "movl %%edx,0(%%eax)\n\t"
>>>> -	    "lahf\n\t"
>>>> -	    "shrl $8,%%eax\n\t"
>>>> -	    "andl $1,%%eax\n"
>>>> -	    : "=a"(rc)
>>>> -	    :    "a"(regs)
>>>> -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
>>>> -#endif
>>>> -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>>>> -		rc = -EINVAL;
>>>> +	asm volatile("out %%al,$0xb2\n\t"
>>>> +		     "out %%al,$0x84\n\t"
>>>> +		     "setc %0\n"
>>>> +		     : "=mr" (carry),
>>>> +		       "+a" (regs->eax),
>>>> +		       "+b" (regs->ebx),
>>>> +		       "+c" (regs->ecx),
>>>> +		       "+d" (regs->edx),
>>>> +		       "+S" (regs->esi),
>>>> +		       "+D" (regs->edi));
>>>>
>>>>   	duration = ktime_us_delta(ktime_get(), calltime);
>>>> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
>>>> -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
>>>> +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
>>>> +		 eax, ebx, regs->eax & 0xffff, carry, duration);
>>>>
>>>> -	return rc;
>>>> +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return 0;
>>>>   }
>>>>
>>>>   /*
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[David Laight]

From: Armin Wolf
> Sent: 23 February 2022 21:39
...
> > As far as this patch goes I think I'd add a 'stc' (set carry)
> > instruction before the first 'outb'.
> > Then if the 'something magic happens here' doesn't happen (because
> > you aren't on the right kind of motherboard) the action fails.
> >
> > 	David
> 
> We already check for such scenarios by checking if eax changed.
> I would not set the carry flag before doing a SMM call since im
> not sure if the firmware code always clears the carry flag when
> the call was successful.
> If for example the firmware code only sets the carry flag on
> error and otherwise ignores the carry flag, we might create some
> false negatives when a successful SMM call leaves the carry flag set.

If you are worried about that you should be clearing carry on entry.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Guenter Roeck]

On 2/23/22 14:35, David Laight wrote:
> From: Armin Wolf
>> Sent: 23 February 2022 21:39
> ...
>>> As far as this patch goes I think I'd add a 'stc' (set carry)
>>> instruction before the first 'outb'.
>>> Then if the 'something magic happens here' doesn't happen (because
>>> you aren't on the right kind of motherboard) the action fails.
>>>
>>> 	David
>>
>> We already check for such scenarios by checking if eax changed.
>> I would not set the carry flag before doing a SMM call since im
>> not sure if the firmware code always clears the carry flag when
>> the call was successful.
>> If for example the firmware code only sets the carry flag on
>> error and otherwise ignores the carry flag, we might create some
>> false negatives when a successful SMM call leaves the carry flag set.
> 
> If you are worried about that you should be clearing carry on entry.
> 

I agree, but now we have an argument for clearing carry (because the
SMM code might not do it) and for setting carry (because the SMM code
might not execute). I don't know which way to go, if any, but that
would be a functional change and should be submitted as separate patch.

Guenter

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Guenter Roeck]

On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> The new assembly code works on both 32 bit and 64 bit
> cpus and allows for more compiler optimisations.
> Since clang runs out of registers on 32 bit x86 when
> using CC_OUT, we need to execute "setc" ourself.
> Also modify the debug message so we can still see
> the result (eax) when the carry flag was set.
> 
> Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

I still did not get any Reviewed-by: or Tested-by: tags for this patch.
There was a suggested change (to either set or clear the carry flag
before executing the out instructions) but that would actually change
the behavior of the code and should be implemented as separate patch
to make it easy to revert without impact on this patch if needed.

Unless someone steps up, that leaves this patch (unfortunately) in limbo.

Guenter



> ---
> Changes in v4:
> - reword commit message
> 
> Changes in v3:
> - make carry an unsigned char
> - use "+a", ... for output registers
> - drop "cc" from clobbered list
> 
> Changes in v2:
> - fix clang running out of registers on 32 bit x86
> - modify debug message
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
>  1 file changed, 18 insertions(+), 60 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index c5939e68586d..38d23a8e83f2 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -119,7 +119,7 @@ struct smm_regs {
>  	unsigned int edx;
>  	unsigned int esi;
>  	unsigned int edi;
> -} __packed;
> +};
> 
>  static const char * const temp_labels[] = {
>  	"CPU",
> @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
>  	struct smm_regs *regs = par;
>  	int eax = regs->eax;
>  	int ebx = regs->ebx;
> +	unsigned char carry;
>  	long long duration;
> -	int rc;
> 
>  	/* SMM requires CPU 0 */
>  	if (smp_processor_id() != 0)
>  		return -EBUSY;
> 
> -#if defined(CONFIG_X86_64)
> -	asm volatile("pushq %%rax\n\t"
> -		"movl 0(%%rax),%%edx\n\t"
> -		"pushq %%rdx\n\t"
> -		"movl 4(%%rax),%%ebx\n\t"
> -		"movl 8(%%rax),%%ecx\n\t"
> -		"movl 12(%%rax),%%edx\n\t"
> -		"movl 16(%%rax),%%esi\n\t"
> -		"movl 20(%%rax),%%edi\n\t"
> -		"popq %%rax\n\t"
> -		"out %%al,$0xb2\n\t"
> -		"out %%al,$0x84\n\t"
> -		"xchgq %%rax,(%%rsp)\n\t"
> -		"movl %%ebx,4(%%rax)\n\t"
> -		"movl %%ecx,8(%%rax)\n\t"
> -		"movl %%edx,12(%%rax)\n\t"
> -		"movl %%esi,16(%%rax)\n\t"
> -		"movl %%edi,20(%%rax)\n\t"
> -		"popq %%rdx\n\t"
> -		"movl %%edx,0(%%rax)\n\t"
> -		"pushfq\n\t"
> -		"popq %%rax\n\t"
> -		"andl $1,%%eax\n"
> -		: "=a"(rc)
> -		:    "a"(regs)
> -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#else
> -	asm volatile("pushl %%eax\n\t"
> -	    "movl 0(%%eax),%%edx\n\t"
> -	    "push %%edx\n\t"
> -	    "movl 4(%%eax),%%ebx\n\t"
> -	    "movl 8(%%eax),%%ecx\n\t"
> -	    "movl 12(%%eax),%%edx\n\t"
> -	    "movl 16(%%eax),%%esi\n\t"
> -	    "movl 20(%%eax),%%edi\n\t"
> -	    "popl %%eax\n\t"
> -	    "out %%al,$0xb2\n\t"
> -	    "out %%al,$0x84\n\t"
> -	    "xchgl %%eax,(%%esp)\n\t"
> -	    "movl %%ebx,4(%%eax)\n\t"
> -	    "movl %%ecx,8(%%eax)\n\t"
> -	    "movl %%edx,12(%%eax)\n\t"
> -	    "movl %%esi,16(%%eax)\n\t"
> -	    "movl %%edi,20(%%eax)\n\t"
> -	    "popl %%edx\n\t"
> -	    "movl %%edx,0(%%eax)\n\t"
> -	    "lahf\n\t"
> -	    "shrl $8,%%eax\n\t"
> -	    "andl $1,%%eax\n"
> -	    : "=a"(rc)
> -	    :    "a"(regs)
> -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#endif
> -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> -		rc = -EINVAL;
> +	asm volatile("out %%al,$0xb2\n\t"
> +		     "out %%al,$0x84\n\t"
> +		     "setc %0\n"
> +		     : "=mr" (carry),
> +		       "+a" (regs->eax),
> +		       "+b" (regs->ebx),
> +		       "+c" (regs->ecx),
> +		       "+d" (regs->edx),
> +		       "+S" (regs->esi),
> +		       "+D" (regs->edi));
> 
>  	duration = ktime_us_delta(ktime_get(), calltime);
> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> 
> -	return rc;
> +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> +		return -EINVAL;
> +
> +	return 0;
>  }
> 
>  /*

Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Guenter Roeck]

On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
> The new assembly code works on both 32 bit and 64 bit
> cpus and allows for more compiler optimisations.
> Since clang runs out of registers on 32 bit x86 when
> using CC_OUT, we need to execute "setc" ourself.
> Also modify the debug message so we can still see
> the result (eax) when the carry flag was set.
> 
> Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

I still have this patch hanging around, with no one willing to tell
me if there are real problems with it. Given that, I decided to just
go ahead and apply it to linux-next. If there turns out to be problems,
we'll see it soon enough and can always revert it. If not, the change
was worth it, so let's just accept the risk.

Guenter

> ---
> Changes in v4:
> - reword commit message
> 
> Changes in v3:
> - make carry an unsigned char
> - use "+a", ... for output registers
> - drop "cc" from clobbered list
> 
> Changes in v2:
> - fix clang running out of registers on 32 bit x86
> - modify debug message
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
>  1 file changed, 18 insertions(+), 60 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index c5939e68586d..38d23a8e83f2 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -119,7 +119,7 @@ struct smm_regs {
>  	unsigned int edx;
>  	unsigned int esi;
>  	unsigned int edi;
> -} __packed;
> +};
> 
>  static const char * const temp_labels[] = {
>  	"CPU",
> @@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
>  	struct smm_regs *regs = par;
>  	int eax = regs->eax;
>  	int ebx = regs->ebx;
> +	unsigned char carry;
>  	long long duration;
> -	int rc;
> 
>  	/* SMM requires CPU 0 */
>  	if (smp_processor_id() != 0)
>  		return -EBUSY;
> 
> -#if defined(CONFIG_X86_64)
> -	asm volatile("pushq %%rax\n\t"
> -		"movl 0(%%rax),%%edx\n\t"
> -		"pushq %%rdx\n\t"
> -		"movl 4(%%rax),%%ebx\n\t"
> -		"movl 8(%%rax),%%ecx\n\t"
> -		"movl 12(%%rax),%%edx\n\t"
> -		"movl 16(%%rax),%%esi\n\t"
> -		"movl 20(%%rax),%%edi\n\t"
> -		"popq %%rax\n\t"
> -		"out %%al,$0xb2\n\t"
> -		"out %%al,$0x84\n\t"
> -		"xchgq %%rax,(%%rsp)\n\t"
> -		"movl %%ebx,4(%%rax)\n\t"
> -		"movl %%ecx,8(%%rax)\n\t"
> -		"movl %%edx,12(%%rax)\n\t"
> -		"movl %%esi,16(%%rax)\n\t"
> -		"movl %%edi,20(%%rax)\n\t"
> -		"popq %%rdx\n\t"
> -		"movl %%edx,0(%%rax)\n\t"
> -		"pushfq\n\t"
> -		"popq %%rax\n\t"
> -		"andl $1,%%eax\n"
> -		: "=a"(rc)
> -		:    "a"(regs)
> -		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#else
> -	asm volatile("pushl %%eax\n\t"
> -	    "movl 0(%%eax),%%edx\n\t"
> -	    "push %%edx\n\t"
> -	    "movl 4(%%eax),%%ebx\n\t"
> -	    "movl 8(%%eax),%%ecx\n\t"
> -	    "movl 12(%%eax),%%edx\n\t"
> -	    "movl 16(%%eax),%%esi\n\t"
> -	    "movl 20(%%eax),%%edi\n\t"
> -	    "popl %%eax\n\t"
> -	    "out %%al,$0xb2\n\t"
> -	    "out %%al,$0x84\n\t"
> -	    "xchgl %%eax,(%%esp)\n\t"
> -	    "movl %%ebx,4(%%eax)\n\t"
> -	    "movl %%ecx,8(%%eax)\n\t"
> -	    "movl %%edx,12(%%eax)\n\t"
> -	    "movl %%esi,16(%%eax)\n\t"
> -	    "movl %%edi,20(%%eax)\n\t"
> -	    "popl %%edx\n\t"
> -	    "movl %%edx,0(%%eax)\n\t"
> -	    "lahf\n\t"
> -	    "shrl $8,%%eax\n\t"
> -	    "andl $1,%%eax\n"
> -	    : "=a"(rc)
> -	    :    "a"(regs)
> -	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> -#endif
> -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> -		rc = -EINVAL;
> +	asm volatile("out %%al,$0xb2\n\t"
> +		     "out %%al,$0x84\n\t"
> +		     "setc %0\n"
> +		     : "=mr" (carry),
> +		       "+a" (regs->eax),
> +		       "+b" (regs->ebx),
> +		       "+c" (regs->ecx),
> +		       "+d" (regs->edx),
> +		       "+S" (regs->esi),
> +		       "+D" (regs->edi));
> 
>  	duration = ktime_us_delta(ktime_get(), calltime);
> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> -		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> +	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
> +		 eax, ebx, regs->eax & 0xffff, carry, duration);
> 
> -	return rc;
> +	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> +		return -EINVAL;
> +
> +	return 0;
>  }
> 
>  /*