linux-kernel - Re: [PATCH 3/3] perf/x86/intel/uncore: Add Skylake server uncore support

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:	Sat, 13 Aug 2016 09:22:51 -0500
From:	Nilay Vaish <nilayvaish@...il.com>
To:	Kan Liang <kan.liang@...el.com>
Cc:	peterz@...radead.org, tglx@...utronix.de, mingo@...hat.com,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	eranian@...gle.com, Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH 3/3] perf/x86/intel/uncore: Add Skylake server uncore support

On 12 August 2016 at 16:30, Kan Liang <kan.liang@...el.com> wrote:
> From: Kan Liang <kan.liang@...el.com>
>
> This patch implements the uncore monitoring driver for Skylake server.
> The uncore subsystem in Skylake server is similar to previous
> server. There are some differences in config register encoding and pci
> device IDs. Besides, Skylake introduces many new boxes to reflect the
> MESH architecture changes.
>
> The control registers for IIO and UPI have been extended to 64 bit. This
> patch also introduces event_mask_ext to handle the high 32 bit mask.
>
> The CHA box number could vary for different machines. This patch gets
> the CHA box number by counting the CHA register space during
> initialization at runtime.
>

The code in this patch contains a lot of constants related to the
Skylake server.  Is it possible to mention in the description the
source from where these values have been picked?  May be Intel
provides some manual that has these values.  It would be good if we
can mention the name of the manual (or a URL) and its version.

> Signed-off-by: Kan Liang <kan.liang@...el.com>
> ---
>  arch/x86/events/intel/uncore.c       |   9 +-
>  arch/x86/events/intel/uncore.h       |   3 +
>  arch/x86/events/intel/uncore_snbep.c | 589 +++++++++++++++++++++++++++++++++++
>  3 files changed, 600 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 3719af5..55a081e 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> +
> +static int skx_count_chabox(void)
> +{
> +       struct pci_dev *chabox_dev = NULL;
> +       int bus, count = 0;
> +
> +       while (1) {
> +               chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d, chabox_dev);
> +               if (!chabox_dev)
> +                       break;
> +               if (count == 0)
> +                       bus = chabox_dev->bus->number;
> +               if (bus != chabox_dev->bus->number)
> +                       break;
> +               count++;
> +       }
> +
> +       pci_dev_put(chabox_dev);
> +       return count;
> +}

Kan,  do we not need to call pci_dev_put() each time we call pci_get_device()?


--
Nilay