linux-kernel - Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180129084650.stj2x65zplou6vma@pali>
Date:   Mon, 29 Jan 2018 09:46:50 +0100
From:   Pali Rohár <pali.rohar@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Jean Delvare <jdelvare@...e.com>,
        Oleksandr Natalenko <oleksandr@...alenko.name>,
        Jonathan Buzzard <jonathan@...zard.org.uk>,
        Mario Limonciello <mario.limonciello@...l.com>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around
 inlined asm

On Saturday 27 January 2018 09:51:45 Guenter Roeck wrote:
> On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote:
> > Measure only inlined asm code, not other functions to have as precise as
> > possible measured time.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> > ---
> >  drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index bf3bb7e1adab..e001afd53f46 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
> >  	int ebx = regs->ebx;
> >  	unsigned long duration;
> >  	ktime_t calltime, delta, rettime;
> > -
> > -	calltime = ktime_get();
> >  #endif
> >  
> >  	/* SMM requires CPU 0 */
> >  	if (smp_processor_id() != 0)
> >  		return -EBUSY;
> >  
> > +#ifdef DEBUG
> > +	calltime = ktime_get();
> > +#endif
> > +
> >  #if defined(CONFIG_X86_64)
> >  	asm volatile("pushq %%rax\n\t"
> >  		"movl 0(%%rax),%%edx\n\t"
> > @@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
> >  	    :    "a"(regs)
> >  	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
> >  #endif
> > -	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > -		rc = -EINVAL;
> >  
> >  #ifdef DEBUG
> >  	rettime = ktime_get();
> >  	delta = ktime_sub(rettime, calltime);
> >  	duration = ktime_to_ns(delta) >> 10;
> > +#endif
> > +
> > +	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
> > +		rc = -EINVAL;
> > +
> > +#ifdef DEBUG
> 
> FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is
> much worse than the improvements from moving the calls around. Using
> specific numbers, the current code reports 500 mS as 488,281 uS.
> So why bother ?

Ah, I have not noticed this yet :-(

> I would have suggested to use ktime_us_delta(ktime_get(), calltime)
> instead to make the results more accurate. Sure, you get the offset from
> the additional divide operation, but at least that would be a constant
> and not a systematic error.
> 
> I'll hold this patch off for a bit. Please confirm that you want it
> applied as-is. I applied the other patches from the series.

Ok, I will fix this patch and resend just new version of this one.

Anyway, if 2/4 is targeting stable, then 3/4 should too. But now I see
that I added Cc only to patch 2/4.

-- 
Pali Rohár
pali.rohar@...il.com