| 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
| ||
|
Message-ID: <CAM9d7ci9fpds3_EbgDp7heJGn-Xc_6NjR7KZ__GYBfaVLftdeg@mail.gmail.com>
Date: Fri, 19 Mar 2021 10:10:28 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Kan Liang <kan.liang@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>,
Stephane Eranian <eranian@...gle.com>,
Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH V2 1/5] perf/x86/intel/uncore: Parse uncore discovery tables
Hi Kan,
On Thu, Mar 18, 2021 at 3:05 AM <kan.liang@...ux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@...ux.intel.com>
>
> A self-describing mechanism for the uncore PerfMon hardware has been
> introduced with the latest Intel platforms. By reading through an MMIO
> page worth of information, perf can 'discover' all the standard uncore
> PerfMon registers in a machine.
>
> The discovery mechanism relies on BIOS's support. With a proper BIOS,
> a PCI device with the unique capability ID 0x23 can be found on each
> die. Perf can retrieve the information of all available uncore PerfMons
> from the device via MMIO. The information is composed of one global
> discovery table and several unit discovery tables.
> - The global discovery table includes global uncore information of the
> die, e.g., the address of the global control register, the offset of
> the global status register, the number of uncore units, the offset of
> unit discovery tables, etc.
> - The unit discovery table includes generic uncore unit information,
> e.g., the access type, the counter width, the address of counters,
> the address of the counter control, the unit ID, the unit type, etc.
> The unit is also called "box" in the code.
> Perf can provide basic uncore support based on this information
> with the following patches.
>
> To locate the PCI device with the discovery tables, check the generic
> PCI ID first. If it doesn't match, go through the entire PCI device tree
> and locate the device with the unique capability ID.
>
> The uncore information is similar among dies. To save parsing time and
> space, only completely parse and store the discovery tables on the first
> die and the first box of each die. The parsed information is stored in
> an
> RB tree structure, intel_uncore_discovery_type. The size of the stored
> discovery tables varies among platforms. It's around 4KB for a Sapphire
> Rapids server.
>
> If a BIOS doesn't support the 'discovery' mechanism, the uncore driver
> will exit with -ENODEV. There is nothing changed.
>
> Add a module parameter to disable the discovery feature. If a BIOS gets
> the discovery tables wrong, users can have an option to disable the
> feature. For the current patchset, the uncore driver will exit with
> -ENODEV. In the future, it may fall back to the hardcode uncore driver
> on a known platform.
>
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> ---
> arch/x86/events/intel/Makefile | 2 +-
> arch/x86/events/intel/uncore.c | 31 ++-
> arch/x86/events/intel/uncore_discovery.c | 318 +++++++++++++++++++++++++++++++
> arch/x86/events/intel/uncore_discovery.h | 105 ++++++++++
> 4 files changed, 448 insertions(+), 8 deletions(-)
> create mode 100644 arch/x86/events/intel/uncore_discovery.c
> create mode 100644 arch/x86/events/intel/uncore_discovery.h
>
> diff --git a/arch/x86/events/intel/Makefile b/arch/x86/events/intel/Makefile
> index e67a588..10bde6c 100644
> --- a/arch/x86/events/intel/Makefile
> +++ b/arch/x86/events/intel/Makefile
> @@ -3,6 +3,6 @@ obj-$(CONFIG_CPU_SUP_INTEL) += core.o bts.o
> obj-$(CONFIG_CPU_SUP_INTEL) += ds.o knc.o
> obj-$(CONFIG_CPU_SUP_INTEL) += lbr.o p4.o p6.o pt.o
> obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE) += intel-uncore.o
> -intel-uncore-objs := uncore.o uncore_nhmex.o uncore_snb.o uncore_snbep.o
> +intel-uncore-objs := uncore.o uncore_nhmex.o uncore_snb.o uncore_snbep.o uncore_discovery.o
> obj-$(CONFIG_PERF_EVENTS_INTEL_CSTATE) += intel-cstate.o
> intel-cstate-objs := cstate.o
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 33c8180..d111370 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -4,7 +4,12 @@
> #include <asm/cpu_device_id.h>
> #include <asm/intel-family.h>
> #include "uncore.h"
> +#include "uncore_discovery.h"
>
> +static bool uncore_no_discover;
> +module_param(uncore_no_discover, bool, 0);
Wouldn't it be better to use a positive form like 'uncore_discover = true'?
To disable, the module param can be set to 'uncore_discover = false'.
> +MODULE_PARM_DESC(uncore_no_discover, "Don't enable the Intel uncore PerfMon discovery mechanism "
> + "(default: enable the discovery mechanism).");
> static struct intel_uncore_type *empty_uncore[] = { NULL, };
> struct intel_uncore_type **uncore_msr_uncores = empty_uncore;
> struct intel_uncore_type **uncore_pci_uncores = empty_uncore;
[SNIP]
> +enum uncore_access_type {
> + UNCORE_ACCESS_MSR = 0,
> + UNCORE_ACCESS_MMIO,
> + UNCORE_ACCESS_PCI,
> +
> + UNCORE_ACCESS_MAX,
> +};
> +
> +struct uncore_global_discovery {
> + union {
> + u64 table1;
> + struct {
> + u64 type : 8,
> + stride : 8,
> + max_units : 10,
> + __reserved_1 : 36,
> + access_type : 2;
> + };
> + };
> +
> + u64 ctl; /* Global Control Address */
> +
> + union {
> + u64 table3;
> + struct {
> + u64 status_offset : 8,
> + num_status : 16,
> + __reserved_2 : 40;
> + };
> + };
> +};
> +
> +struct uncore_unit_discovery {
> + union {
> + u64 table1;
> + struct {
> + u64 num_regs : 8,
> + ctl_offset : 8,
> + bit_width : 8,
> + ctr_offset : 8,
> + status_offset : 8,
> + __reserved_1 : 22,
> + access_type : 2;
> + };
> + };
> +
> + u64 ctl; /* Unit Control Address */
> +
> + union {
> + u64 table3;
> + struct {
> + u64 box_type : 16,
> + box_id : 16,
> + __reserved_2 : 32;
> + };
> + };
> +};
> +
> +struct intel_uncore_discovery_type {
> + struct rb_node node;
> + enum uncore_access_type access_type;
> + u64 box_ctrl; /* Unit ctrl addr of the first box */
> + u64 *box_ctrl_die; /* Unit ctrl addr of the first box of each die */
> + u16 type; /* Type ID of the uncore block */
> + u8 num_counters;
> + u8 counter_width;
> + u8 ctl_offset; /* Counter Control 0 offset */
> + u8 ctr_offset; /* Counter 0 offset */
I find it confusing and easy to miss - ctl and ctr. Some places you used
ctrl or counter. Why not be consistent? :)
Thanks,
Namhyung
> + u16 num_boxes; /* number of boxes for the uncore block */
> + unsigned int *ids; /* Box IDs */
> + unsigned int *box_offset; /* Box offset */
> +};
> +
> +bool intel_uncore_has_discovery_tables(void);
> +void intel_uncore_clear_discovery_tables(void);
> --
> 2.7.4
>
Powered by blists - more mailing lists