linux-kernel - Re: [PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.DEB.2.21.1803111048360.3841@nanos.tec.linutronix.de>
Date:   Sun, 11 Mar 2018 11:01:35 -0700 (PDT)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Jason Vas Dias <jason.vas.dias@...il.com>
cc:     x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
        andi <andi@...stfloor.org>, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle
 CLOCK_MONOTONIC_RAW

On Sun, 11 Mar 2018, Jason Vas Dias wrote:

This looks better now. Though running that patch through checkpatch.pl
results in:

total: 28 errors, 20 warnings, 139 lines checked

....

> +notrace static u64 vread_tsc_raw(void)

Why do you need a separate function? I asked you to use vread_tsc(). So you
might have reasons for doing that, but please then explain WHY and not just
throw the stuff in my direction w/o any comment.

> +{
> +        u64 tsc, last=gtod->raw_cycle_last;
> +        if( likely( gtod->has_rdtscp ) ) {
> +                u32     tsc_lo, tsc_hi,
> +                        tsc_cpu __attribute__((unused));
> +                asm volatile
> +                ( "rdtscp"
> +                /* ^- has built-in cancellation point / pipeline stall "barrier" */
> +                : "=a" (tsc_lo)
> +                , "=d" (tsc_hi)
> +                , "=c" (tsc_cpu)
> +                ); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx                 
> +                tsc      = ((((u64)tsc_hi) & 0xffffffffUL) << 32) | (((u64)tsc_lo) & 0xffffffffUL);

This is not required to make the vdso accessor for monotonic raw work.

If at all then the rdtscp support wants to be in a separate patch with a
proper explanation.

Aside of that the code for rdtscp wants to be in a proper inline helper in
the relevant header file and written according to the coding style the
kernel uses for asm inlines.

> +        } else {
> +                tsc      = rdtsc_ordered();
> +        }
> +	if (likely(tsc >= last))
> +		return tsc;        
> +        asm volatile ("");                                
> +        return last;
> +}

The rest looks ok.

Thanks,

	tglx