From: "Liang, Kan" <kan.liang@linux.intel.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>,
Arnaldo Carvalho de Melo <acme@redhat.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>,
Stephane Eranian <eranian@google.com>,
Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH V2 1/5] perf/x86/intel/uncore: Parse uncore discovery tables
Date: Fri, 19 Mar 2021 16:28:59 -0400 [thread overview]
Message-ID: <8dadd0d7-8f18-0e94-6b5d-76bc59561ffb@linux.intel.com> (raw)
In-Reply-To: <CAM9d7ci9fpds3_EbgDp7heJGn-Xc_6NjR7KZ__GYBfaVLftdeg@mail.gmail.com>
On 3/18/2021 9:10 PM, Namhyung Kim wrote:
> Hi Kan,
>
> On Thu, Mar 18, 2021 at 3:05 AM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.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@linux.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'.
>
I'd like the feature is enabled by default. The default value of a
static is 0. So I use the current name. It's just a personal preference.
>> +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? :)
>
The ctl and ctr are consistent with the variable name in the struct
intel_uncore_type.
The counter or counter control are only in the comments.
I guess the naming should be OK. :)
Thanks,
Kan
> 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
>>
next prev parent reply other threads:[~2021-03-19 20:29 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-17 17:59 [PATCH V2 0/5] Uncore PMON discovery mechanism support kan.liang
2021-03-17 17:59 ` [PATCH V2 1/5] perf/x86/intel/uncore: Parse uncore discovery tables kan.liang
2021-03-19 1:10 ` Namhyung Kim
2021-03-19 20:28 ` Liang, Kan [this message]
2021-04-02 8:12 ` [tip: perf/core] " tip-bot2 for Kan Liang
2022-07-22 12:55 ` [PATCH V2 1/5] " Lucas De Marchi
2022-07-22 13:04 ` Liang, Kan
2022-07-23 18:56 ` Lucas De Marchi
2022-07-25 14:51 ` Liang, Kan
2022-08-02 14:22 ` Lucas De Marchi
2022-08-02 15:43 ` Liang, Kan
2022-08-02 16:02 ` Lucas De Marchi
2022-08-02 17:23 ` Liang, Kan
2021-03-17 17:59 ` [PATCH V2 2/5] perf/x86/intel/uncore: Generic support for the MSR type of uncore blocks kan.liang
2021-04-02 8:12 ` [tip: perf/core] " tip-bot2 for Kan Liang
2021-03-17 17:59 ` [PATCH V2 3/5] perf/x86/intel/uncore: Rename uncore_notifier to uncore_pci_sub_notifier kan.liang
2021-04-02 8:12 ` [tip: perf/core] " tip-bot2 for Kan Liang
2021-03-17 17:59 ` [PATCH V2 4/5] perf/x86/intel/uncore: Generic support for the PCI type of uncore blocks kan.liang
2021-04-02 8:12 ` [tip: perf/core] " tip-bot2 for Kan Liang
2021-03-17 17:59 ` [PATCH V2 5/5] perf/x86/intel/uncore: Generic support for the MMIO " kan.liang
2021-04-02 8:12 ` [tip: perf/core] " tip-bot2 for Kan Liang
2022-09-20 18:25 ` [PATCH V2 0/5] Uncore PMON discovery mechanism support Kin Cho
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8dadd0d7-8f18-0e94-6b5d-76bc59561ffb@linux.intel.com \
--to=kan.liang@linux.intel.com \
--cc=acme@redhat.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=eranian@google.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).