* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-20 19:08 [PATCH v4] hwmon: (dell-smm) Improve assembly code Armin Wolf
@ 2022-02-22 16:54 ` Guenter Roeck
2022-02-22 17:51 ` Pali Rohár
2022-03-02 18:07 ` Guenter Roeck
2022-06-20 15:21 ` Guenter Roeck
2 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2022-02-22 16:54 UTC (permalink / raw)
To: Armin Wolf
Cc: pali, jdelvare, David.Laight, linux-hwmon, linux-assembly, linux-kernel
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;
> }
>
> /*
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-22 16:54 ` Guenter Roeck
@ 2022-02-22 17:51 ` Pali Rohár
2022-02-22 19:46 ` David Laight
0 siblings, 1 reply; 11+ messages in thread
From: Pali Rohár @ 2022-02-22 17:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Armin Wolf, jdelvare, David.Laight, linux-hwmon, linux-assembly,
linux-kernel
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;
> > }
> >
> > /*
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-22 17:51 ` Pali Rohár
@ 2022-02-22 19:46 ` David Laight
0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2022-02-22 19:46 UTC (permalink / raw)
To: 'Pali Rohár', Guenter Roeck
Cc: Armin Wolf, jdelvare, linux-hwmon, linux-assembly, linux-kernel
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)
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code
@ 2022-02-22 19:46 ` David Laight
0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2022-02-22 19:46 UTC (permalink / raw)
To: 'Pali Rohár', Guenter Roeck
Cc: Armin Wolf, jdelvare, linux-hwmon, linux-assembly, linux-kernel
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)
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-22 19:46 ` David Laight
@ 2022-02-23 21:39 ` Armin Wolf
-1 siblings, 0 replies; 11+ messages in thread
From: Armin Wolf @ 2022-02-23 21:39 UTC (permalink / raw)
To: David Laight, 'Pali Rohár', Guenter Roeck
Cc: jdelvare, linux-hwmon, linux-assembly, linux-kernel
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)
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
@ 2022-02-23 21:39 ` Armin Wolf
0 siblings, 0 replies; 11+ messages in thread
From: Armin Wolf @ 2022-02-23 21:39 UTC (permalink / raw)
To: David Laight, 'Pali Rohár', Guenter Roeck
Cc: jdelvare, linux-hwmon, linux-assembly, linux-kernel
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)
>
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-23 21:39 ` Armin Wolf
(?)
@ 2022-02-23 22:35 ` David Laight
2022-02-24 0:45 ` Guenter Roeck
-1 siblings, 1 reply; 11+ messages in thread
From: David Laight @ 2022-02-23 22:35 UTC (permalink / raw)
To: 'Armin Wolf', 'Pali Rohár', Guenter Roeck
Cc: jdelvare, linux-hwmon, linux-assembly, linux-kernel
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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-23 22:35 ` David Laight
@ 2022-02-24 0:45 ` Guenter Roeck
0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2022-02-24 0:45 UTC (permalink / raw)
To: David Laight, 'Armin Wolf', 'Pali Rohár'
Cc: jdelvare, linux-hwmon, linux-assembly, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-20 19:08 [PATCH v4] hwmon: (dell-smm) Improve assembly code Armin Wolf
2022-02-22 16:54 ` Guenter Roeck
@ 2022-03-02 18:07 ` Guenter Roeck
2022-06-20 15:21 ` Guenter Roeck
2 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2022-03-02 18:07 UTC (permalink / raw)
To: Armin Wolf
Cc: pali, jdelvare, David.Laight, linux-hwmon, linux-assembly, linux-kernel
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;
> }
>
> /*
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code
2022-02-20 19:08 [PATCH v4] hwmon: (dell-smm) Improve assembly code Armin Wolf
2022-02-22 16:54 ` Guenter Roeck
2022-03-02 18:07 ` Guenter Roeck
@ 2022-06-20 15:21 ` Guenter Roeck
2 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2022-06-20 15:21 UTC (permalink / raw)
To: Armin Wolf
Cc: pali, jdelvare, David.Laight, linux-hwmon, linux-assembly, linux-kernel
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;
> }
>
> /*
^ permalink raw reply [flat|nested] 11+ messages in thread