Re: [PATCH v2 06/13] platform/x86: hfi: parse CPU core ranking data from shared memory

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/14/2024 05:14, Ilpo Järvinen wrote:
> On Thu, 10 Oct 2024, Mario Limonciello wrote:
> 
>> From: Perry Yuan <Perry.Yuan@amd.com>
>>
>> When `amd_hfi` driver is loaded, it will use PCCT subspace type 4 table
>> to retrieve the shared memory address which contains the CPU core ranking
>> table. This table includes a header that specifies the number of ranking
>> data entries to be parsed and rank each CPU core with the Performance and
>> Energy Efficiency capability as implemented by the CPU power management
>> firmware.
>>
>> Once the table has been parsed, each CPU is assigned a ranking score
>> within its class. Subsequently, when the scheduler selects cores, it
>> chooses from the ranking list based on the assigned scores in each class,
>> thereby ensuring the optimal selection of CPU cores according to their
>> predefined classifications and priorities.
>>
>> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v2:
>>   * Rework amd_hfi_fill_metatadata to directly use structure instead of
>>     pointer math.
>> ---
>>   drivers/platform/x86/amd/hfi/hfi.c | 215 ++++++++++++++++++++++++++++-
>>   1 file changed, 212 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hfi/hfi.c b/drivers/platform/x86/amd/hfi/hfi.c
>> index da2e667107e8..10651399cf75 100644
>> --- a/drivers/platform/x86/amd/hfi/hfi.c
>> +++ b/drivers/platform/x86/amd/hfi/hfi.c
>> @@ -18,22 +18,78 @@
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>> +#include <linux/mailbox_client.h>
>>   #include <linux/mutex.h>
>> +#include <linux/percpu-defs.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/printk.h>
>>   #include <linux/smp.h>
>>   #include <linux/string.h>
>> +#include <linux/topology.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include <asm/cpu_device_id.h>
>> +
>> +#include <acpi/pcc.h>
>> +#include <acpi/cppc_acpi.h>
>>   
>>   #define AMD_HFI_DRIVER		"amd_hfi"
>> +#define AMD_HFI_MAILBOX_COUNT	1
>> +#define AMD_HETERO_RANKING_TABLE_VER	2
>> +
>>   #define AMD_HETERO_CPUID_27	0x80000027
>> +
>>   static struct platform_device *device;
>>   
>> +/**
>> + * struct amd_shmem_info - Shared memory table for AMD HFI
>> + *
>> + * @signature:	The PCC signature. The signature of a subspace is computed by
>> + *		a bitwise of the value 0x50434300 with the subspace ID.
>> + * @flags:	Notify on completion
>> + * @length:	Length of payload being transmitted including command field
>> + * @command:	Command being sent over the subspace
>> + * @version_number:		Version number of the table
>> + * @n_logical_processors:	Number of logical processors
>> + * @n_capabilities:		Number of ranking dimensions (performance, efficiency, etc)
>> + * @table_update_context:	Command being sent over the subspace
>> + * @n_bitmaps:			Number of 32-bit bitmaps to enumerate all the APIC IDs
>> + *				This is based on the maximum APIC ID enumerated in the system
>> + * @reserved:			24 bit spare
>> + * @table_data:			Bit Map(s) of enabled logical processors
>> + *				Followed by the ranking data for each logical processor
>> + */
>> +struct amd_shmem_info {
>> +	struct acpi_pcct_ext_pcc_shared_memory header;
>> +	u32	version_number		:8,
>> +		n_logical_processors	:8,
>> +		n_capabilities		:8,
>> +		table_update_context	:8;
>> +	u32	n_bitmaps		:8,
>> +		reserved		:24;
>> +	u32	table_data[];
>> +} __packed;
>> +
>>   struct amd_hfi_data {
>>   	const char	*name;
>>   	struct device	*dev;
>>   	struct mutex	lock;
>> +
>> +	/* PCCT table related*/
>> +	struct pcc_mbox_chan	*pcc_chan;
>> +	void __iomem		*pcc_comm_addr;
>> +	struct acpi_subtable_header	*pcct_entry;
>> +	struct amd_shmem_info	*shmem;
>>   };
>>   
>> +/**
>> + * struct amd_hfi_classes - HFI class capabilities per CPU
>> + * @perf:	Performance capability
>> + * @eff:	Power efficiency capability
>> + *
>> + * Capabilities of a logical processor in the ranking table. These capabilities
>> + * are unitless and specific to each HFI class.
>> + */
>>   struct amd_hfi_classes {
>>   	u32	perf;
>>   	u32	eff;
>> @@ -42,23 +98,105 @@ struct amd_hfi_classes {
>>   /**
>>    * struct amd_hfi_cpuinfo - HFI workload class info per CPU
>>    * @cpu:		cpu index
>> + * @apic_id:		apic id of the current cpu
>>    * @cpus:		mask of cpus associated with amd_hfi_cpuinfo
>>    * @class_index:	workload class ID index
>>    * @nr_class:		max number of workload class supported
>> + * @ipcc_scores:	ipcc scores for each class
>>    * @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;
>> +	u32		apic_id;
>>   	cpumask_var_t	cpus;
>>   	s16		class_index;
>>   	u8		nr_class;
>> +	int		*ipcc_scores;
>>   	struct amd_hfi_classes	*amd_hfi_classes;
>>   };
>>   
>>   static DEFINE_PER_CPU(struct amd_hfi_cpuinfo, amd_hfi_cpuinfo) = {.class_index = -1};
>>   
>> +static int find_cpu_index_by_apicid(unsigned int target_apicid)
>> +{
>> +	int cpu_index;
>> +
>> +	for_each_possible_cpu(cpu_index) {
>> +		struct cpuinfo_x86 *info = &cpu_data(cpu_index);
>> +
>> +		if (info->topo.apicid == target_apicid) {
>> +			pr_debug("match APIC id %d for CPU index: %d",
> 
> Missing \n
Ack
> 
>> +				 info->topo.apicid, cpu_index);
>> +			return cpu_index;
>> +		}
>> +	}
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static int amd_hfi_fill_metadata(struct amd_hfi_data *amd_hfi_data)
>> +{
>> +	struct acpi_pcct_ext_pcc_slave *pcct_ext =
>> +		(struct acpi_pcct_ext_pcc_slave *)amd_hfi_data->pcct_entry;
>> +	void __iomem *pcc_comm_addr;
>> +
>> +	pcc_comm_addr = acpi_os_ioremap(amd_hfi_data->pcc_chan->shmem_base_addr,
>> +					amd_hfi_data->pcc_chan->shmem_size);
>> +	if (!pcc_comm_addr) {
>> +		pr_err("failed to ioremap PCC common region mem\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	memcpy_fromio(amd_hfi_data->shmem, pcc_comm_addr, pcct_ext->length);
>> +	iounmap(pcc_comm_addr);
>> +
>> +	if (amd_hfi_data->shmem->header.signature != PCC_SIGNATURE) {
>> +		pr_err("Invalid signature in shared memory\n");
>> +		return -EINVAL;
>> +	}
>> +	if (amd_hfi_data->shmem->version_number != AMD_HETERO_RANKING_TABLE_VER) {
>> +		pr_err("Invalid veresion %d\n", amd_hfi_data->shmem->version_number);
> 
> version
Ack
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (u32 i = 0; i < amd_hfi_data->shmem->n_bitmaps; i++) {
>> +		u32 bitmap = amd_hfi_data->shmem->table_data[i];
>> +
>> +		for (u32 j = 0; j < BITS_PER_TYPE(u32); j++) {
> 
> Are these u32 really the types you want to use for the loop vars, why?

I was going off the type of amd_hfi_data->shmem->n_bitmaps which is u32.
In practice I think an unsigned int should be fine though too.

> 
>> +			struct amd_hfi_cpuinfo *info;
>> +			int apic_id = i * BITS_PER_TYPE(u32) + j;
>> +			int cpu_index;
>> +
>> +			if (!(bitmap & BIT(j)))
>> +				continue;
>> +
>> +			cpu_index = find_cpu_index_by_apicid(apic_id);
>> +			if (cpu_index < 0) {
>> +				pr_warn("APIC ID %d not found\n", apic_id);
>> +				continue;
>> +			}
>> +
>> +			info = per_cpu_ptr(&amd_hfi_cpuinfo, cpu_index);
>> +			info->apic_id = apic_id;
>> +
>> +			/* Fill the ranking data for each logical processor */
>> +			info = per_cpu_ptr(&amd_hfi_cpuinfo, cpu_index);
>> +			for (int k = 0; k < info->nr_class; k++) {
> 
> unsigned int
> 
>> +				u32 *table = amd_hfi_data->shmem->table_data +
>> +					     amd_hfi_data->shmem->n_bitmaps +
>> +					     i * info->nr_class;
>> +
>> +				info->amd_hfi_classes[k].eff = table[apic_id + 2 * k];
>> +				info->amd_hfi_classes[k].perf = table[apic_id + 2 * k + 1];
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int amd_hfi_alloc_class_data(struct platform_device *pdev)
>>   {
>>   	struct amd_hfi_cpuinfo *hfi_cpuinfo;
>> @@ -68,8 +206,7 @@ static int amd_hfi_alloc_class_data(struct platform_device *pdev)
>>   
>>   	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);
>> +		dev_warn(dev, "failed to get number of supported classes\n");
> 
> This message was added in the previous patch and now immediately changed.

Will drop this change.

> 
>>   		return -EINVAL;
>>   	}
>>   
>> @@ -79,7 +216,10 @@ static int amd_hfi_alloc_class_data(struct platform_device *pdev)
>>   				sizeof(struct amd_hfi_classes), GFP_KERNEL);
>>   		if (!hfi_cpuinfo->amd_hfi_classes)
>>   			return -ENOMEM;
>> -
>> +		hfi_cpuinfo->ipcc_scores = devm_kcalloc(dev, nr_class_id,
>> +							sizeof(int), GFP_KERNEL);
>> +		if (!hfi_cpuinfo->ipcc_scores)
>> +			return -ENOMEM;
>>   		hfi_cpuinfo->nr_class = nr_class_id;
>>   	}
>>   
>> @@ -93,6 +233,70 @@ static void amd_hfi_remove(struct platform_device *pdev)
>>   	mutex_destroy(&dev->lock);
>>   }
>>   
>> +static int amd_hfi_metadata_parser(struct platform_device *pdev,
>> +				   struct amd_hfi_data *amd_hfi_data)
>> +{
>> +	struct acpi_pcct_ext_pcc_slave *pcct_ext;
>> +	struct acpi_subtable_header *pcct_entry;
>> +	struct mbox_chan *pcc_mbox_channels;
>> +	struct acpi_table_header *pcct_tbl;
>> +	struct pcc_mbox_chan *pcc_chan;
>> +	acpi_status status;
>> +	int ret;
>> +
>> +	pcc_mbox_channels = devm_kcalloc(&pdev->dev, AMD_HFI_MAILBOX_COUNT,
>> +					 sizeof(*pcc_mbox_channels), GFP_KERNEL);
>> +	if (!pcc_mbox_channels) {
>> +		ret = -ENOMEM;
>> +		goto out;
> 
> Please return directly if there is nothing to rollback.
Ack
> 
>> +	}
>> +
>> +	pcc_chan = devm_kcalloc(&pdev->dev, AMD_HFI_MAILBOX_COUNT,
>> +				sizeof(*pcc_chan), GFP_KERNEL);
>> +	if (!pcc_chan) {
>> +		ret = -ENOMEM;
>> +		goto out;
> 
> Ditto.
> 
>> +	}
>> +
>> +	status = acpi_get_table(ACPI_SIG_PCCT, 0, &pcct_tbl);
>> +	if (ACPI_FAILURE(status) || !pcct_tbl) {
>> +		ret = -ENODEV;
>> +		goto out;
> 
> Ditto.
> 
>> +	}
>> +
>> +	/* get pointer to the first PCC subspace entry */
>> +	pcct_entry = (struct acpi_subtable_header *) (
>> +			(unsigned long)pcct_tbl + sizeof(struct acpi_table_pcct));
>> +
>> +	pcc_chan->mchan = &pcc_mbox_channels[0];
>> +
>> +	amd_hfi_data->pcc_chan = pcc_chan;
>> +	amd_hfi_data->pcct_entry = pcct_entry;
>> +	pcct_ext = (struct acpi_pcct_ext_pcc_slave *)pcct_entry;
>> +
>> +	if (pcct_ext->length <= 0) {
>> +		ret = -EINVAL;
>> +		goto out;
> 
> Ditto.
> 
>> +	}
>> +
>> +	amd_hfi_data->shmem = devm_kmalloc(amd_hfi_data->dev, pcct_ext->length, GFP_KERNEL);
> 
> Why kmalloc ?
> 
>> +	if (!amd_hfi_data->shmem) {
>> +		ret = -ENOMEM;
>> +		goto out;
> 
> Return directly.
> 
>> +	}
>> +
>> +	pcc_chan->shmem_base_addr = pcct_ext->base_address;
>> +	pcc_chan->shmem_size = pcct_ext->length;
>> +
>> +	/* parse the shared memory info from the pcct table */
>> +	ret = amd_hfi_fill_metadata(amd_hfi_data);
>> +
>> +	acpi_put_table(pcct_tbl);
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>>   static const struct acpi_device_id amd_hfi_platform_match[] = {
>>   	{ "AMDI0104", 0},
>>   	{ }
>> @@ -121,6 +325,11 @@ static int amd_hfi_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto out;
> 
> This should do return ret; directly, not jump to out label which does
> nothing but return.
> 
>>   
>> +	/* parse PCCT table */
>> +	ret = amd_hfi_metadata_parser(pdev, amd_hfi_data);
>> +	if (ret)
>> +		goto out;
>> +
>>   out:
>>   	return ret;
> 
> Might again be there for churn avoidance, otherwise, please consider:
> 
> 	return amd_hfi_metadata_parser(pdev, amd_hfi_data);
> 
> That goto out should again just return ret directly.
> 
>>   }
>>
>