linux-kernel - Re: [PATCH v2 3/4] platform/x86: intel_pmc

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: <CAHp75VcLN6g-6FJsVgxPZRV675N6YQzxsbm19BWgKhpiYfmG3w@mail.gmail.com>
Date:   Fri, 19 Oct 2018 15:34:07 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     rajneesh.bhardwaj@...ux.intel.com
Cc:     Platform Driver <platform-driver-x86@...r.kernel.org>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>
Subject: Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non
 Snoop LTR

On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
<rajneesh.bhardwaj@...ux.intel.com> wrote:
>
> The LTR values follow PCIE LTR encoding format and can be decoded as per
> https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
>
> This adds support to translate the raw LTR values as read from the PMC
> to meaningful values in nanosecond units of time.

While I have pushed this to my review and testing queue, it needs a
bit more work. See my comments below.

> +static u32 convert_ltr_scale(u32 val)
> +{

> +       u32 scale = 0;

Redundant, see below.

> +       /*
> +        * As per PCIE specification supporting document
> +        * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
> +        * Tolerance Reporting data payload is encoded in a
> +        * 3 bit scale and 10 bit value fields. Values are
> +        * multiplied by the indicated scale to yield an absolute time
> +        * value, expressible in a range from 1 nanosecond to
> +        * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
> +        *
> +        * scale encoding is as follows:
> +        *
> +        * ----------------------------------------------
> +        * |scale factor        |       Multiplier (ns) |
> +        * ----------------------------------------------
> +        * |    0               |       1               |
> +        * |    1               |       32              |
> +        * |    2               |       1024            |
> +        * |    3               |       32768           |
> +        * |    4               |       1048576         |
> +        * |    5               |       33554432        |
> +        * |    6               |       Invalid         |
> +        * |    7               |       Invalid         |
> +        * ----------------------------------------------
> +        */

> +       if (val > 5)

> +               pr_warn("Invalid LTR scale factor.\n");

if (...) {
 pr_warn(...); // Btw, Does it recoverable state? What user will get
with returned 0 as a multiplier?
 return 0; // Btw, is 0 fits better than ~0? How hw would behave with
this value?
}

> +       else
> +               scale = 1U << (5 * (val));
> +
> +       return scale;

return 1U << (5 * val);

> +}

>         for (index = 0; map[index].name ; index++) {
> -               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
> -                          map[index].name,
> -                          pmc_core_reg_read(pmcdev, map[index].bit_mask));

We use 32 characters for the names. Here are two minor issues:
- inconsistency with the rest
- ping-pong style of programming (you changed 32 to 24 in the same
series where you introduced 32 in the first place).


> +               decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
> +               ltr_raw_data = pmc_core_reg_read(pmcdev,
> +                                                map[index].bit_mask);
> +               snoop_ltr = ltr_raw_data & ~MTPMC_MASK;
> +               nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK;
> +
> +               if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) {
> +                       scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr);
> +                       val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr);
> +                       decoded_non_snoop_ltr = val * convert_ltr_scale(scale);
> +               }
> +
> +               if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) {
> +                       scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr);
> +                       val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr);
> +                       decoded_snoop_ltr = val * convert_ltr_scale(scale);
> +               }
> +

> +               seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",

Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x
much better.
After you remove the index, it would give you 4 more characters,
though it 4 less than 8 you got from reducing 32 to 24.

OTOH, those long texts perhaps may be compressed somehow, at least
remove LTR duplicating from the last two. Remove spaces after '\t' as
well.

> +                          index, map[index].name, ltr_raw_data,
> +                          decoded_non_snoop_ltr,
> +                          decoded_snoop_ltr);
>         }
>         return 0;
>  }

> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -177,6 +177,11 @@ enum ppfear_regs {

It might be good idea to include linux/bits.h here.

> +#define LTR_REQ_NONSNOOP                       BIT(31)
> +#define LTR_REQ_SNOOP                          BIT(15)
> +#define LTR_DECODED_VAL                                GENMASK(9, 0)
> +#define LTR_DECODED_SCALE                      GENMASK(12, 10)

-- 
With Best Regards,
Andy Shevchenko