Message-ID: <b97df985-8545-9f8b-719b-3f8b6a19f2f5@linux.intel.com>
Date: Fri, 21 Mar 2025 16:01:17 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Mario Limonciello <superm1@...nel.org>
cc: Hans de Goede <hdegoede@...hat.com>, 
    Mario Limonciello <mario.limonciello@....com>, 
    Perry Yuan <perry.yuan@....com>, Thomas Gleixner <tglx@...utronix.de>, 
    Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
    Dave Hansen <dave.hansen@...ux.intel.com>, 
    "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>, 
    "H . Peter Anvin" <hpa@...or.com>, Jonathan Corbet <corbet@....net>, 
    Huang Rui <ray.huang@....com>, 
    "Gautham R . Shenoy" <gautham.shenoy@....com>, 
    "Rafael J . Wysocki" <rafael@...nel.org>, 
    Viresh Kumar <viresh.kumar@...aro.org>, 
    "open list:AMD HETERO CORE HARDWARE FEEDBACK DRIVER" <platform-driver-x86@...r.kernel.org>, 
    "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <linux-kernel@...r.kernel.org>, 
    "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>, 
    "open list:AMD PSTATE DRIVER" <linux-pm@...r.kernel.org>, 
    Shyam Sundar S K <Shyam-sundar.S-k@....com>
Subject: Re: [PATCH v8 04/13] platform/x86: hfi: Introduce AMD Hardware
 Feedback Interface Driver

On Thu, 20 Mar 2025, Mario Limonciello wrote:

> On 3/19/2025 09:03, Ilpo Järvinen wrote:
> > On Tue, 18 Feb 2025, Mario Limonciello wrote:
> > 
> > > From: Perry Yuan <Perry.Yuan@....com>
> > > 
> > > The AMD Heterogeneous core design and Hardware Feedback Interface (HFI)
> > > provide behavioral classification and a dynamically updated ranking table
> > > for the scheduler to use when choosing cores for tasks.
> > > 
> > > There are two CPU core types defined: `Classic Core` and `Dense Core`.
> > > "Classic" cores are the standard performance cores, while "Dense" cores
> > > are optimized for area and efficiency.
> > > 
> > > Heterogeneous compute refers to CPU implementations that are comprised
> > > of more than one architectural class, each with two capabilities. This
> > > means each CPU reports two separate capabilities: "perf" and "eff".
> > > 
> > > Each capability lists all core ranking numbers between 0 and 255, where
> > > a higher number represents a higher capability.
> > > 
> > > Heterogeneous systems can also extend to more than two architectural
> > > classes.
> > > 
> > > The purpose of the scheduling feedback mechanism is to provide information
> > > to the operating system scheduler in real time, allowing the scheduler to
> > > direct threads to the optimal core during task scheduling.
> > > 
> > > All core ranking data are provided by the PMFW via a shared memory ranking
> > > table, which the driver reads and uses to update core capabilities to the
> > > scheduler. When the hardware updates the table, it generates a platform
> > > interrupt to notify the OS to read the new ranking table.
> > > 
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>
> > > Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> > > Signed-off-by: Perry Yuan <perry.yuan@....com>
> > > Co-developed-by: Mario Limonciello <mario.limonciello@....com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> > > ---
> > > v8:
> > >   * s,devm_kzalloc,devm_kcalloc,
> > >   * fold newlines from patch 5 into this patch
> > >   * Drop ->cpu member, push to later patch
> > >   * s,for_each_present_cpu,for_each_possible_cpu,
> > > v7:
> > >   * Adjust Kconfig to 80 characters
> > > ---
> > >   drivers/platform/x86/amd/Kconfig      |   1 +
> > >   drivers/platform/x86/amd/Makefile     |   1 +
> > >   drivers/platform/x86/amd/hfi/Kconfig  |  20 ++++
> > >   drivers/platform/x86/amd/hfi/Makefile |   7 ++
> > >   drivers/platform/x86/amd/hfi/hfi.c    | 162 ++++++++++++++++++++++++++
> > >   5 files changed, 191 insertions(+)
> > >   create mode 100644 drivers/platform/x86/amd/hfi/Kconfig
> > >   create mode 100644 drivers/platform/x86/amd/hfi/Makefile
> > >   create mode 100644 drivers/platform/x86/amd/hfi/hfi.c
> > > 
> > > diff --git a/drivers/platform/x86/amd/Kconfig
> > > b/drivers/platform/x86/amd/Kconfig
> > > index c3e086ea64fc6..589d61ebf726b 100644
> > > --- a/drivers/platform/x86/amd/Kconfig
> > > +++ b/drivers/platform/x86/amd/Kconfig
> > > @@ -6,6 +6,7 @@
> > >   source "drivers/platform/x86/amd/hsmp/Kconfig"
> > >   source "drivers/platform/x86/amd/pmf/Kconfig"
> > >   source "drivers/platform/x86/amd/pmc/Kconfig"
> > > +source "drivers/platform/x86/amd/hfi/Kconfig"
> > >     config AMD_3D_VCACHE
> > >   	tristate "AMD 3D V-Cache Performance Optimizer Driver"
> > > diff --git a/drivers/platform/x86/amd/Makefile
> > > b/drivers/platform/x86/amd/Makefile
> > > index 56f62fc9c97b4..c50e93c3334cf 100644
> > > --- a/drivers/platform/x86/amd/Makefile
> > > +++ b/drivers/platform/x86/amd/Makefile
> > > @@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC)		+= pmc/
> > >   obj-$(CONFIG_AMD_HSMP)		+= hsmp/
> > >   obj-$(CONFIG_AMD_PMF)		+= pmf/
> > >   obj-$(CONFIG_AMD_WBRF)		+= wbrf.o
> > > +obj-$(CONFIG_AMD_HFI)		+= hfi/
> > > diff --git a/drivers/platform/x86/amd/hfi/Kconfig
> > > b/drivers/platform/x86/amd/hfi/Kconfig
> > > new file mode 100644
> > > index 0000000000000..532939eb08a6a
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/amd/hfi/Kconfig
> > > @@ -0,0 +1,20 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# AMD Hardware Feedback Interface Driver
> > > +#
> > > +
> > > +config AMD_HFI
> > > +	bool "AMD Hetero Core Hardware Feedback Driver"
> > > +	depends on ACPI
> > > +	depends on CPU_SUP_AMD
> > > +	help
> > > +	 Select this option to enable the AMD Heterogeneous Core Hardware
> > > +	 Feedback Interface. If selected, hardware provides runtime thread
> > > +	 classification guidance to the operating system on the performance
> > > and
> > > +	 energy efficiency capabilities of each heterogeneous CPU core. These
> > > +	 capabilities may vary due to the inherent differences in the core
> > > types
> > > +	 and can also change as a result of variations in the operating
> > > +	 conditions of the system such as power and thermal limits. If
> > > selected,
> > 
> > This says the capabilities can change but metadata is only read and scores
> > updated during probe?
> > 
> > > +	 the kernel relays updates in heterogeneous CPUs' capabilities to
> > > +	 userspace, allowing for more optimal task scheduling and resource
> > > +	 allocation, leveraging the diverse set of cores available.
> > 
> > How are the capabilities communicated to userspace as mentioned here? I'm
> > asking this because I only noted debugfs interface, and that commit
> > claimed the debug fs interface was to troubleshoot scheduler issues.
> 
> This is one of those cases that the split into multiple parts shows. Thinking
> through I feel it makes more sense to adjust for now and then we can change it
> again on the next part.
> 
> > 
> > > diff --git a/drivers/platform/x86/amd/hfi/Makefile
> > > b/drivers/platform/x86/amd/hfi/Makefile
> > > new file mode 100644
> > > index 0000000000000..672c6ac106e95
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/amd/hfi/Makefile
> > > @@ -0,0 +1,7 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# AMD Hardware Feedback Interface Driver
> > > +#
> > > +
> > > +obj-$(CONFIG_AMD_HFI) += amd_hfi.o
> > > +amd_hfi-objs := hfi.o
> > > diff --git a/drivers/platform/x86/amd/hfi/hfi.c
> > > b/drivers/platform/x86/amd/hfi/hfi.c
> > > new file mode 100644
> > > index 0000000000000..426f7e520b76c
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/amd/hfi/hfi.c
> > > @@ -0,0 +1,162 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * AMD Hardware Feedback Interface Driver
> > > + *
> > > + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> > 
> > 2025 ?
> 
> Ack.
> 
> Heh, earlier versions started in 2024!
>
> > > + *
> > > + * Authors: Perry Yuan <Perry.Yuan@....com>
> > > + *          Mario Limonciello <mario.limonciello@....com>
> > > + */
> > > +
> > > +#define pr_fmt(fmt)  "amd-hfi: " fmt
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/cpu.h>
> > > +#include <linux/cpumask.h>
> > > +#include <linux/gfp.h>
> > > +#include <linux/init.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/smp.h>
> > > +
> > > +#define AMD_HFI_DRIVER		"amd_hfi"
> > > +
> > > +#define AMD_HETERO_CPUID_27	0x80000027
> > > +
> > > +static struct platform_device *device;
> > > +
> > > +struct amd_hfi_data {
> > > +	const char	*name;
> > > +	struct device	*dev;
> > > +	struct mutex	lock;
> > 
> > Please mention what this protects.
> 
> As a comment at the end like this, right?
>
> struct mutex lock; /* lock the foo */

Yes.

-- 
 i.



> 
> > 
> > > +};
> > > +
> > > +struct amd_hfi_classes {
> > > +	u32	perf;
> > > +	u32	eff;
> > > +};
> > > +
> > > +/**
> > > + * struct amd_hfi_cpuinfo - HFI workload class info per CPU
> > > + * @cpu:		cpu index
> > > + * @class_index:	workload class ID index
> > > + * @nr_class:		max number of workload class supported
> > > + * @amd_hfi_classes:	current cpu workload class ranking data
> > > + *
> > > + * Parameters of a logical processor linked with hardware feedback class
> > 
> > missing .
> 
> Ack
> 
> > 
> > > + */
> > > +struct amd_hfi_cpuinfo {
> > > +	int		cpu;
> > > +	s16		class_index;
> > > +	u8		nr_class;
> > > +	struct amd_hfi_classes	*amd_hfi_classes;
> > > +};
> > > +
> > > +static DEFINE_PER_CPU(struct amd_hfi_cpuinfo, amd_hfi_cpuinfo) =
> > > {.class_index = -1};
> > > +
> > > +static int amd_hfi_alloc_class_data(struct platform_device *pdev)
> > > +{
> > > +	struct amd_hfi_cpuinfo *hfi_cpuinfo;
> > > +	struct device *dev = &pdev->dev;
> > > +	int idx;
> > > +	int nr_class_id;
> > > +
> > > +	nr_class_id = cpuid_eax(AMD_HETERO_CPUID_27);
> > > +	if (nr_class_id < 0 || nr_class_id > 255) {
> > 
> > Is the signed type correct for this?
> > 
> > > +		dev_err(dev, "failed to get number of supported classes:
> > > %d\n",
> > > +			nr_class_id);
> > 
> > I'd reword the error message as the number of classes was just too
> > large / outside the allowed range.
> 
> OK.
> 
> > 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	for_each_possible_cpu(idx) {
> > > +		struct amd_hfi_classes *classes;
> > > +
> > > +		classes = devm_kcalloc(dev,
> > > +				       nr_class_id,
> > > +				       sizeof(struct amd_hfi_classes),
> > > +				       GFP_KERNEL);
> > > +		if (!classes)
> > > +			return -ENOMEM;
> > > +		hfi_cpuinfo = per_cpu_ptr(&amd_hfi_cpuinfo, idx);
> > > +		hfi_cpuinfo->amd_hfi_classes = classes;
> > > +		hfi_cpuinfo->nr_class = nr_class_id;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct acpi_device_id amd_hfi_platform_match[] = {
> > > +	{"AMDI0104", 0},
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, amd_hfi_platform_match);
> > > +
> > > +static int amd_hfi_probe(struct platform_device *pdev)
> > > +{
> > > +	struct amd_hfi_data *amd_hfi_data;
> > > +	int ret;
> > > +
> > > +	if (!acpi_match_device(amd_hfi_platform_match, &pdev->dev))
> > > +		return -ENODEV;
> > > +
> > > +	amd_hfi_data = devm_kzalloc(&pdev->dev, sizeof(*amd_hfi_data),
> > > GFP_KERNEL);
> > > +	if (!amd_hfi_data)
> > > +		return -ENOMEM;
> > > +
> > > +	amd_hfi_data->dev = &pdev->dev;
> > > +	ret = devm_mutex_init(&pdev->dev, &amd_hfi_data->lock);
> > > +	if (ret)
> > > +		return ret;
> > > +	platform_set_drvdata(pdev, amd_hfi_data);
> > > +
> > > +	ret = amd_hfi_alloc_class_data(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver amd_hfi_driver = {
> > > +	.driver = {
> > > +		.name = AMD_HFI_DRIVER,
> > > +		.owner = THIS_MODULE,
> > > +		.acpi_match_table = ACPI_PTR(amd_hfi_platform_match),
> > > +	},
> > > +	.probe = amd_hfi_probe,
> > > +};
> > > +
> > > +static int __init amd_hfi_init(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (acpi_disabled ||
> > > +	    !cpu_feature_enabled(X86_FEATURE_AMD_HETEROGENEOUS_CORES) ||
> > > +	    !cpu_feature_enabled(X86_FEATURE_AMD_WORKLOAD_CLASS))
> > > +		return -ENODEV;
> > > +
> > > +	device = platform_device_register_simple(AMD_HFI_DRIVER, -1, NULL, 0);
> > > +	if (IS_ERR(device)) {
> > > +		pr_err("unable to register HFI platform device\n");
> > > +		return PTR_ERR(device);
> > > +	}
> > > +
> > > +	ret = platform_driver_register(&amd_hfi_driver);
> > > +	if (ret)
> > > +		pr_err("failed to register HFI driver\n");
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __exit void amd_hfi_exit(void)
> > > +{
> > > +	platform_device_unregister(device);
> > > +	platform_driver_unregister(&amd_hfi_driver);
> > 
> > Why are these not in the opposite order than in init?
> 
> Oversight.  Will fix it.
> 
> > 
> > > +}
> > > +module_init(amd_hfi_init);
> > > +module_exit(amd_hfi_exit);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("AMD Hardware Feedback Interface Driver");
> > > 
> > 
> 

Powered by blists - more mailing lists