Message-ID: <3aaa5dae-48d7-4562-8735-a96abda92df4@amd.com>
Date: Mon, 14 Oct 2024 14:46:47 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Borislav Petkov <bp@...en8.de>, Hans de Goede <hdegoede@...hat.com>,
 x86@...nel.org, "Gautham R . Shenoy" <gautham.shenoy@....com>,
 Perry Yuan <perry.yuan@....com>, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-pm@...r.kernel.org,
 platform-driver-x86@...r.kernel.org,
 Shyam Sundar S K <Shyam-sundar.S-k@....com>
Subject: Re: [PATCH v2 05/13] platform/x86: hfi: Introduce AMD Hardware
 Feedback Interface Driver

On 10/14/2024 04:10, Ilpo Järvinen wrote:
> On Thu, 10 Oct 2024, 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 BIOS 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
>> Signed-off-by: Perry Yuan <perry.yuan@....com>
>> Co-developed-by: Mario Limonciello <mario.limonciello@....com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> v2:
>>   * Drop unnecessary select
>>   * Make tristate instead of bool
>>   * Drop error messages
>>   * Drop unnecessary function declarations for init
>>   * Fix cleanup for amd_hfi_exit()
>>   * Drop unnecessary variables for upcoming features
>> ---
>>   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    | 169 ++++++++++++++++++++++++++
>>   5 files changed, 198 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 f88682d36447..c3f69dbe3037 100644
>> --- a/drivers/platform/x86/amd/Kconfig
>> +++ b/drivers/platform/x86/amd/Kconfig
>> @@ -5,6 +5,7 @@
>>   
>>   source "drivers/platform/x86/amd/pmf/Kconfig"
>>   source "drivers/platform/x86/amd/pmc/Kconfig"
>> +source "drivers/platform/x86/amd/hfi/Kconfig"
>>   
>>   config AMD_HSMP
>>   	tristate "AMD HSMP Driver"
>> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
>> index dcec0a46f8af..2676fc81fee5 100644
>> --- a/drivers/platform/x86/amd/Makefile
>> +++ b/drivers/platform/x86/amd/Makefile
>> @@ -9,3 +9,4 @@ amd_hsmp-y			:= hsmp.o
>>   obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
>>   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 000000000000..08051cd4f74d
>> --- /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, 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.
>> +
>> +
>> diff --git a/drivers/platform/x86/amd/hfi/Makefile b/drivers/platform/x86/amd/hfi/Makefile
>> new file mode 100644
>> index 000000000000..672c6ac106e9
>> --- /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 000000000000..da2e667107e8
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/hfi/hfi.c
>> @@ -0,0 +1,169 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * AMD Hardware Feedback Interface Driver
>> + *
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
>> + *
>> + * Author: Perry Yuan <Perry.Yuan@....com>
>> + *
> 
> Extra line.
> 
>> + */
>> +
>> +#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/printk.h>
>> +#include <linux/smp.h>
>> +#include <linux/string.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;
>> +};
>> +
>> +struct amd_hfi_classes {
>> +	u32	perf;
>> +	u32	eff;
>> +} __packed;
> 
> Unnecessary packed.
> 
>> +
>> +/**
>> + * struct amd_hfi_cpuinfo - HFI workload class info per CPU
>> + * @cpu:		cpu index
>> + * @cpus:		mask of cpus associated with amd_hfi_cpuinfo
>> + * @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
>> + */
>> +struct amd_hfi_cpuinfo {
>> +	int		cpu;
>> +	cpumask_var_t	cpus;
>> +	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) {
>> +		dev_warn(dev, "failed to get supported class number from CPUID %d\n",
>> +				AMD_HETERO_CPUID_27);
>> +		return -EINVAL;
>> +	}
>> +
>> +	for_each_possible_cpu(idx) {
>> +		hfi_cpuinfo = per_cpu_ptr(&amd_hfi_cpuinfo, idx);
>> +		hfi_cpuinfo->amd_hfi_classes = devm_kmalloc(dev, nr_class_id *
>> +				sizeof(struct amd_hfi_classes), GFP_KERNEL);
> 
> I recommend splitting this line differently.
> 
> Why it's not using kzalloc?
> 
>> +		if (!hfi_cpuinfo->amd_hfi_classes)
>> +			return -ENOMEM;
>> +
>> +		hfi_cpuinfo->nr_class = nr_class_id;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void amd_hfi_remove(struct platform_device *pdev)
>> +{
>> +	struct amd_hfi_data *dev = platform_get_drvdata(pdev);
>> +
>> +	mutex_destroy(&dev->lock);
>> +}
>> +
>> +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;
>> +
>> +	mutex_init(&amd_hfi_data->lock);
> 
> No idea why the empty line is put between these initializations of the
> data structure fields.
> 
>> +	platform_set_drvdata(pdev, amd_hfi_data);
>> +
>> +	/* alloc data array for hardware feedback class data */
> 
> Unnecessary comment? Isn't the function name enough to tell it?
> 
>> +	ret = amd_hfi_alloc_class_data(pdev);
>> +	if (ret)
>> +		goto out;
>> +
>> +out:
> 
> I'm guessing this might be to reduce churn in some patch after this,
> if not, this is enough:

Thanks for all the feedback.

This was intended to avoid churn, but some of that change isn't part of 
this series and will be a future follow up that can adjust at that time.

I'll adjust to churn for "this series alone".

> 
> 	return amd_hfi_alloc_class_data(pdev);
> 
>> +	return ret;
>> +}
>> +
>> +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,
>> +	.remove_new = amd_hfi_remove,
>> +};
>> +
>> +static int __init amd_hfi_init(void)
>> +{
>> +	int ret;
>> +
>> +	if (acpi_disabled ||
>> +	    !boot_cpu_has(X86_FEATURE_HETERO_CORE_TOPOLOGY) ||
>> +	    !boot_cpu_has(X86_FEATURE_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");
> 
> I assume HFI is the correct capitalization given I see some comments with
> that. Please correct any user visible print outs to use the capitalized
> form.
> 
>> +		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);
>> +}
>> +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