On Thu, 10 Oct 2024, Mario Limonciello wrote:
> From: Perry Yuan <Perry.Yuan@xxxxxxx>
>
> 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@xxxxxxx>
> Co-developed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> 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
> + 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
> + 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?
> + 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.
> 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.
> + }
> +
> + 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.
> }
>
--
i.
