linux-kernel - Re: [v2 PATCH] cpufreq: powernv: Correctly parse the sign of pstates on POWER8 vs POWER9

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]

Date:   Fri, 8 Dec 2017 14:44:40 +1100
From:   Balbir Singh <bsingharora@...il.com>
To:     "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
Cc:     Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        huntbag@...ux.vnet.ibm.com,
        Akshay Adiga <akshay.adiga@...ux.vnet.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
        "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" 
        <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        stable@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [v2 PATCH] cpufreq: powernv: Correctly parse the sign of pstates
 on POWER8 vs POWER9

On Thu, Dec 7, 2017 at 4:59 PM, Gautham R. Shenoy
<ego@...ux.vnet.ibm.com> wrote:
> From: "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
>
> On POWERNV platform, Pstates are 8-bit values. On POWER8 they are
> negatively numbered while on POWER9 they are positively
> numbered. Thus, on POWER9, the maximum number of pstates could be as
> high as 256.
>
> The current code interprets pstates as a signed 8-bit value. This
> causes a problem on POWER9 platforms which have more than 128 pstates.
> On such systems, on a CPU that is in a lower pstate whose number is
> greater than 128, querying the current pstate returns a "pstate X is
> out of bound" error message and the current pstate is reported as the
> nominal pstate.
>
> This patch fixes the aforementioned issue by correctly differentiating
> the sign whenever a pstate value read, depending on whether the
> pstates are positively numbered or negatively numbered.

Yikes! Is there no better way of fixing this?

>
> Fixes: commit 09ca4c9b5958 ("cpufreq: powernv: Replacing pstate_id with frequency table index")
> Cc: <stable@...r.kernel.org> #v4.8
> Signed-off-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com>
> Tested-and-reviewed-by: Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
> Acked-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 43 ++++++++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index b6d7c4c..bb7586e 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -41,11 +41,14 @@
>  #define POWERNV_MAX_PSTATES    256
>  #define PMSR_PSAFE_ENABLE      (1UL << 30)
>  #define PMSR_SPR_EM_DISABLE    (1UL << 31)
> -#define PMSR_MAX(x)            ((x >> 32) & 0xFF)
> +#define EXTRACT_BYTE(x, shift) (((x) >> shift) & 0xFF)
> +#define MAX_SHIFT              32
>  #define LPSTATE_SHIFT          48
>  #define GPSTATE_SHIFT          56
> -#define GET_LPSTATE(x)         (((x) >> LPSTATE_SHIFT) & 0xFF)
> -#define GET_GPSTATE(x)         (((x) >> GPSTATE_SHIFT) & 0xFF)
> +#define GET_PMSR_MAX(x)                EXTRACT_BYTE(x, MAX_SHIFT)
> +#define GET_LPSTATE(x)         EXTRACT_BYTE(x, LPSTATE_SHIFT)
> +#define GET_GPSTATE(x)         EXTRACT_BYTE(x, GPSTATE_SHIFT)
> +

Can you hide all of this in pstate_to_idx(), do the casting inside? I
was reviewing this
code earlier before being distracted with something else, this did
come across as
strange and I was looking at using abs values to simplify the code,
but I did not get
to it.

Balbir Singh.