| 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: <20211130032029.GA1371@ranerica-svr.sc.intel.com>
Date: Mon, 29 Nov 2021 19:20:29 -0800
From: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Linux PM <linux-pm@...r.kernel.org>,
the arch/x86 maintainers <x86@...nel.org>,
"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
Len Brown <len.brown@...el.com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Aubrey Li <aubrey.li@...ux.intel.com>,
Amit Kucheria <amitk@...nel.org>,
Andi Kleen <ak@...ux.intel.com>,
Tim Chen <tim.c.chen@...ux.intel.com>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Ricardo Neri <ricardo.neri@...el.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/7] thermal: intel: hfi: Minimally initialize the
Hardware Feedback Interface
On Wed, Nov 24, 2021 at 03:09:20PM +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 6, 2021 at 2:34 AM Ricardo Neri
> <ricardo.neri-calderon@...ux.intel.com> wrote:
> >
> > The Intel Hardware Feedback Interface provides guidance to the operating
> > system about the performance and energy efficiency capabilities of each
> > CPU in the system. Capabilities are numbers between 0 and 255 where a
> > higher number represents a higher capability. For each CPU, energy
> > efficiency and performance are reported as separate capabilities.
> >
> > Hardware computes these capabilities based on the operating conditions of
> > the system such as power and thermal limits. These capabilities are shared
> > with the operating system in a table resident in memory. Each package in
> > the system has its own HFI instance. Every logical CPU in the package is
> > represented in the table. More than one logical CPUs may be represented in
> > a single table entry. When the hardware updates the table, it generates a
> > package-level thermal interrupt.
> >
> > The size and format of the HFI table depend on the supported features and
> > can only be determined at runtime. To minimally initialize the HFI, parse
> > its features and allocate one instance per package of a data structure with
> > the necessary parameters to read and navigate individual HFI tables.
> >
> > A subsequent changeset will provide per-CPU initialization and interrupt
> > handling.
> >
> > Cc: Andi Kleen <ak@...ux.intel.com>
> > Cc: Aubrey Li <aubrey.li@...ux.intel.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> > Cc: Tim Chen <tim.c.chen@...ux.intel.com>
> > Cc: "Ravi V. Shankar" <ravi.v.shankar@...el.com>
> > Reviewed-by: Len Brown <len.brown@...el.com>
> > Co-developed by: Aubrey Li <aubrey.li@...ux.intel.com>
> > Signed-off-by: Aubrey Li <aubrey.li@...ux.intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> > ---
> > drivers/thermal/intel/Kconfig | 12 +++
> > drivers/thermal/intel/Makefile | 1 +
> > drivers/thermal/intel/intel_hfi.c | 155 ++++++++++++++++++++++++++++
> > drivers/thermal/intel/intel_hfi.h | 34 ++++++
> > drivers/thermal/intel/therm_throt.c | 3 +
> > 5 files changed, 205 insertions(+)
> > create mode 100644 drivers/thermal/intel/intel_hfi.c
> > create mode 100644 drivers/thermal/intel/intel_hfi.h
> >
> > diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> > index c83ea5d04a1d..d4c6bdcacddb 100644
> > --- a/drivers/thermal/intel/Kconfig
> > +++ b/drivers/thermal/intel/Kconfig
> > @@ -99,3 +99,15 @@ config INTEL_MENLOW
> > Intel Menlow platform.
> >
> > If unsure, say N.
> > +
> > +config INTEL_HFI
> > + bool "Intel Hardware Feedback Interface"
> > + depends on CPU_SUP_INTEL
> > + depends on SCHED_MC && X86_THERMAL_VECTOR
> > + help
> > + Select this option to enable the Hardware Feedback Interface. If
> > + selected, hardware provides guidance to the operating system on
> > + the performance and energy efficiency capabilities of each CPU.
> > + These capabilities may change as a result of changes in the operating
> > + conditions of the system such power and thermal limits. If selected,
> > + the kernel relays updates in CPUs' capabilities to userspace.
> > diff --git a/drivers/thermal/intel/Makefile b/drivers/thermal/intel/Makefile
> > index 960b56268b4a..1a80bffcd699 100644
> > --- a/drivers/thermal/intel/Makefile
> > +++ b/drivers/thermal/intel/Makefile
> > @@ -13,3 +13,4 @@ obj-$(CONFIG_INTEL_PCH_THERMAL) += intel_pch_thermal.o
> > obj-$(CONFIG_INTEL_TCC_COOLING) += intel_tcc_cooling.o
> > obj-$(CONFIG_X86_THERMAL_VECTOR) += therm_throt.o
> > obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o
> > +obj-$(CONFIG_INTEL_HFI) += intel_hfi.o
> > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> > new file mode 100644
> > index 000000000000..edfe343507b3
> > --- /dev/null
> > +++ b/drivers/thermal/intel/intel_hfi.c
> > @@ -0,0 +1,155 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Hardware Feedback Interface Driver
> > + *
> > + * Copyright (c) 2021, Intel Corporation.
> > + *
> > + * Authors: Aubrey Li <aubrey.li@...ux.intel.com>
> > + * Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> > + *
> > + *
> > + * The Hardware Feedback Interface provides a performance and energy efficiency
> > + * capability information for each CPU in the system. Depending on the processor
> > + * model, hardware may periodically update these capabilities as a result of
> > + * changes in the operating conditions (e.g., power limits or thermal
> > + * constraints). On other processor models, there is a single HFI update
> > + * at boot.
> > + *
> > + * This file provides functionality to process HFI updates and relay these
> > + * updates to userspace.
> > + */
> > +
> > +#define pr_fmt(fmt) "intel-hfi: " fmt
> > +
> > +#include <linux/slab.h>
> > +
> > +#include "intel_hfi.h"
> > +
> > +/**
> > + * struct hfi_cpu_data - HFI capabilities per CPU
> > + * @perf_cap: Performance capability
> > + * @ee_cap: Energy efficiency capability
> > + *
> > + * Capabilities of a logical processor in the HFI table. These capabilities are
> > + * unitless.
> > + */
> > +struct hfi_cpu_data {
> > + u8 perf_cap;
> > + u8 ee_cap;
> > +} __packed;
> > +
> > +/**
> > + * struct hfi_hdr - Header of the HFI table
> > + * @perf_updated: Hardware updated performance capabilities
> > + * @ee_updated: Hardware updated energy efficiency capabilities
> > + *
> > + * Properties of the data in an HFI table.
> > + */
> > +struct hfi_hdr {
> > + u8 perf_updated;
> > + u8 ee_updated;
> > +} __packed;
> > +
> > +/**
> > + * struct hfi_instance - Representation of an HFI instance (i.e., a table)
> > + * @ts_counter: Time stamp of the last update of the table
> > + * @hdr: Base address of the table header
> > + * @data: Base address of the table data
> > + *
> > + * A set of parameters to parse and navigate a specific HFI table.
> > + */
> > +struct hfi_instance {
> > + u64 *ts_counter;
> > + void *hdr;
> > + void *data;
> > +};
> > +
> > +/**
> > + * struct hfi_features - Supported HFI features
> > + * @capabilities: Bitmask of supported capabilities
> > + * @nr_table_pages: Size of the HFI table in 4KB pages
> > + * @cpu_stride: Stride size to locate capability data of a logical
> > + * processor within the table (i.e., row stride)
> > + * @hdr_size: Size of table header
> > + * @parsed: True if HFI features have been parsed
> > + *
> > + * Parameters and supported features that are common to all HFI instances
> > + */
> > +struct hfi_features {
> > + unsigned long capabilities;
> > + unsigned int nr_table_pages;
> > + unsigned int cpu_stride;
> > + unsigned int hdr_size;
> > + bool parsed;
Thank you very much for your feedback, Rafael!
>
> I'm not sure why this field is needed.
>
> It looks like it is only checked by hfi_parse_features() which is only
> called by intel_hfi_init() which is invoked by
> thermal_throttle_init_device() which can happen only once if I'm not
> mistaken.
This is very true. It is not needed. I will remove it.
> > +};
> > +
> > +static int max_hfi_instances;
> > +static struct hfi_instance *hfi_instances;
> > +
> > +static struct hfi_features hfi_features;
> > +
> > +static __init int hfi_parse_features(void)
> > +{
> > + unsigned int nr_capabilities, reg;
> > +
> > + if (!boot_cpu_has(X86_FEATURE_INTEL_HFI))
> > + return -ENODEV;
> > +
> > + if (hfi_features.parsed)
> > + return 0;
> > +
> > + /*
> > + * If we are here we know that CPUID_HFI_LEAF exists. Parse the
> > + * supported capabilities and the size of the HFI table.
> > + */
> > + reg = cpuid_edx(CPUID_HFI_LEAF);
> > +
> > + hfi_features.capabilities = reg & HFI_CAPABILITIES_MASK;
> > + if (!(hfi_features.capabilities & HFI_CAPABILITIES_PERFORMANCE)) {
> > + pr_err("Performance reporting not supported! Not using HFI\n");
>
> This doesn't need to be pr_err().
Should it be a pr_warn() or perhaps pr_info()?
Thanks and BR,
Ricardo
Powered by blists - more mailing lists