Re: [PATCH 06/12] platform/x86: ISST: Enumerate TPMI SST and create framework
From: Hans de Goede
Date: Wed Mar 01 2023 - 09:48:57 EST
Hi,
On 3/1/23 15:45, srinivas pandruvada wrote:
> On Wed, 2023-03-01 at 15:37 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/11/23 07:32, Srinivas Pandruvada wrote:
>>> Enumerate TPMI SST driver and create basic framework to add more
>>> features.
>>>
>>> The basic user space interface is still same as the legacy using
>>> /dev/isst_interface. Users of "intel-speed-select" utility should
>>> be able to use same commands as prior gens without being aware
>>> of new underlying hardware interface.
>>>
>>> TPMI SST driver enumerates on device "intel_vsec.tpmi-sst". Since
>>> there
>>> can be multiple instances and there is one common SST core, split
>>> implementation into two parts: A common core part and an
>>> enumeration
>>> part. The enumeration driver is loaded for each device instance and
>>> register with the TPMI SST core driver.
>>>
>>> On very first enumeration the TPMI SST core driver register with
>>> SST
>>> core driver to get IOCTL callbacks. The api_version is incremented
>>> for IOCTL ISST_IF_GET_PLATFORM_INFO, so that user space can issue
>>> new IOCTLs.
>>>
>>> Each TPMI package contains multiple power domains. Each power
>>> domain
>>> has its own set of SST controls. For each domain map the MMIO
>>> memory
>>> and update per domain struct tpmi_per_power_domain_info. This
>>> information
>>> will be used to implement other SST interfaces.
>>>
>>> Implement first IOCTL commands to get number of TPMI SST instances
>>> and instance mask as some of the power domains may not have any
>>> SST controls.
>>>
>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>
>> But this is a somewhat larger patch and IMHO it would be good
>> to get an extra pair of eyes on this, can you get someone else
>> from Intel to also review this patch ?
>
> Five versions of this patchset was reviewed internally.
That is good to know.
>> This (getting someone else to review the patches) especially
>> applies to patches 7-11, where I don't really feel myself
>> qualifeed to review them. I have given them a quick lookover
>> and nothing stood out, but they really need a Reviewed-by or
>> at minimum an Ack from someone else @Intel.
>>
> When I submit the next version, I will add their reviewed-by.
> So once you are done your reviews, I will submit a new series.
I'm done with my review, I only had some remarks to patch 12/12.
Please submit a new version with Revieved-by(s) when you
have the new version ready.
Regards,
Hans
>>> ---
>>> .../x86/intel/speed_select_if/Kconfig | 4 +
>>> .../x86/intel/speed_select_if/Makefile | 2 +
>>> .../x86/intel/speed_select_if/isst_tpmi.c | 53 ++++
>>> .../intel/speed_select_if/isst_tpmi_core.c | 274
>>> ++++++++++++++++++
>>> .../intel/speed_select_if/isst_tpmi_core.h | 16 +
>>> include/uapi/linux/isst_if.h | 18 ++
>>> 6 files changed, 367 insertions(+)
>>> create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>>
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> b/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> index ce3e3dc076d2..4eb3ad299db0 100644
>>> --- a/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> +++ b/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> @@ -2,8 +2,12 @@ menu "Intel Speed Select Technology interface
>>> support"
>>> depends on PCI
>>> depends on X86_64 || COMPILE_TEST
>>>
>>> +config INTEL_SPEED_SELECT_TPMI
>>> + tristate
>>> +
>>> config INTEL_SPEED_SELECT_INTERFACE
>>> tristate "Intel(R) Speed Select Technology interface
>>> drivers"
>>> + select INTEL_SPEED_SELECT_TPMI if INTEL_TPMI
>>> help
>>> This config enables the Intel(R) Speed Select Technology
>>> interface
>>> drivers. The Intel(R) speed select technology features
>>> are non
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/Makefile
>>> b/drivers/platform/x86/intel/speed_select_if/Makefile
>>> index 856076206f35..1d878a36d0ab 100644
>>> --- a/drivers/platform/x86/intel/speed_select_if/Makefile
>>> +++ b/drivers/platform/x86/intel/speed_select_if/Makefile
>>> @@ -8,3 +8,5 @@ obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) +=
>>> isst_if_common.o
>>> obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mmio.o
>>> obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mbox_pci.o
>>> obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mbox_msr.o
>>> +obj-$(CONFIG_INTEL_SPEED_SELECT_TPMI) += isst_tpmi_core.o
>>> +obj-$(CONFIG_INTEL_SPEED_SELECT_TPMI) += isst_tpmi.o
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> new file mode 100644
>>> index 000000000000..7b4bdeefb8bc
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> @@ -0,0 +1,53 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * isst_tpmi.c: SST TPMI interface
>>> + *
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All Rights Reserved.
>>> + *
>>> + */
>>> +
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/module.h>
>>> +#include <linux/intel_tpmi.h>
>>> +
>>> +#include "isst_tpmi_core.h"
>>> +
>>> +static int intel_sst_probe(struct auxiliary_device *auxdev, const
>>> struct auxiliary_device_id *id)
>>> +{
>>> + int ret;
>>> +
>>> + ret = tpmi_sst_init();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = tpmi_sst_dev_add(auxdev);
>>> + if (ret)
>>> + tpmi_sst_exit();
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void intel_sst_remove(struct auxiliary_device *auxdev)
>>> +{
>>> + tpmi_sst_dev_remove(auxdev);
>>> + tpmi_sst_exit();
>>> +}
>>> +
>>> +static const struct auxiliary_device_id intel_sst_id_table[] = {
>>> + { .name = "intel_vsec.tpmi-sst" },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(auxiliary, intel_sst_id_table);
>>> +
>>> +static struct auxiliary_driver intel_sst_aux_driver = {
>>> + .id_table = intel_sst_id_table,
>>> + .remove = intel_sst_remove,
>>> + .probe = intel_sst_probe,
>>> +};
>>> +
>>> +module_auxiliary_driver(intel_sst_aux_driver);
>>> +
>>> +MODULE_IMPORT_NS(INTEL_TPMI_SST);
>>> +MODULE_DESCRIPTION("Intel TPMI SST Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git
>>> a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> new file mode 100644
>>> index 000000000000..6b37016c0417
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> @@ -0,0 +1,274 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * isst_tpmi.c: SST TPMI interface core
>>> + *
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All Rights Reserved.
>>> + *
>>> + * This information will be useful to understand flows:
>>> + * In the current generation of platforms, TPMI is supported via
>>> OOB
>>> + * PCI device. This PCI device has one instance per CPU package.
>>> + * There is a unique TPMI ID for SST. Each TPMI ID also has
>>> multiple
>>> + * entries, representing per power domain information.
>>> + *
>>> + * There is one dev file for complete SST information and control
>>> same as the
>>> + * prior generation of hardware. User spaces don't need to know
>>> how the
>>> + * information is presented by the hardware. The TPMI core module
>>> implements
>>> + * the hardware mapping.
>>> + */
>>> +
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/intel_tpmi.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <uapi/linux/isst_if.h>
>>> +
>>> +#include "isst_tpmi_core.h"
>>> +#include "isst_if_common.h"
>>> +
>>> +/**
>>> + * struct tpmi_per_power_domain_info - Store per power_domain SST
>>> info
>>> + * @package_id: Package id for this power_domain
>>> + * @power_domain_id: Power domain id, Each entry from the SST-
>>> TPMI instance is a power_domain.
>>> + * @sst_base: Mapped SST base IO memory
>>> + * @auxdev: Auxiliary device instance enumerated this
>>> instance
>>> + *
>>> + * This structure is used store complete SST information for a
>>> power_domain. This information
>>> + * is used to read/write request for any SST IOCTL. Each physical
>>> CPU package can have multiple
>>> + * power_domains. Each power domain describes its own SST
>>> information and has its own controls.
>>> + */
>>> +struct tpmi_per_power_domain_info {
>>> + int package_id;
>>> + int power_domain_id;
>>> + void __iomem *sst_base;
>>> + struct auxiliary_device *auxdev;
>>> +};
>>> +
>>> +/**
>>> + * struct tpmi_sst_struct - Store sst info for a package
>>> + * @package_id: Package id for this aux
>>> device instance
>>> + * @number_of_power_domains: Number of power_domains pointed by
>>> power_domain_info pointer
>>> + * @power_domain_info: Pointer to power domains
>>> information
>>> + *
>>> + * This structure is used store full SST information for a
>>> package.
>>> + * Each package has a unique OOB PCI device, which enumerates
>>> TPMI.
>>> + * Each Package will have multiple power_domains.
>>> + */
>>> +struct tpmi_sst_struct {
>>> + int package_id;
>>> + int number_of_power_domains;
>>> + struct tpmi_per_power_domain_info *power_domain_info;
>>> +};
>>> +
>>> +/**
>>> + * struct tpmi_sst_common_struct - Store all SST instances
>>> + * @max_index: Maximum instances currently present
>>> + * @sst_inst: Pointer to per package instance
>>> + *
>>> + * Stores every SST Package instance.
>>> + */
>>> +struct tpmi_sst_common_struct {
>>> + int max_index;
>>> + struct tpmi_sst_struct **sst_inst;
>>> +};
>>> +
>>> +/*
>>> + * Each IOCTL request is processed under this lock. Also used to
>>> protect
>>> + * registration functions and common data structures.
>>> + */
>>> +static DEFINE_MUTEX(isst_tpmi_dev_lock);
>>> +
>>> +/* Usage count to track, number of TPMI SST instances registered
>>> to this core. */
>>> +static int isst_core_usage_count;
>>> +
>>> +/* Stores complete SST information for every package and
>>> power_domain */
>>> +static struct tpmi_sst_common_struct isst_common;
>>> +
>>> +static int isst_if_get_tpmi_instance_count(void __user *argp)
>>> +{
>>> + struct isst_tpmi_instance_count tpmi_inst;
>>> + struct tpmi_sst_struct *sst_inst;
>>> + int i;
>>> +
>>> + if (copy_from_user(&tpmi_inst, argp, sizeof(tpmi_inst)))
>>> + return -EFAULT;
>>> +
>>> + if (tpmi_inst.socket_id >= topology_max_packages())
>>> + return -EINVAL;
>>> +
>>> + tpmi_inst.count =
>>> isst_common.sst_inst[tpmi_inst.socket_id]->number_of_power_domains;
>>> +
>>> + sst_inst = isst_common.sst_inst[tpmi_inst.socket_id];
>>> + tpmi_inst.valid_mask = 0;
>>> + for (i = 0; i < sst_inst->number_of_power_domains; ++i) {
>>> + struct tpmi_per_power_domain_info
>>> *power_domain_info;
>>> +
>>> + power_domain_info = &sst_inst-
>>>> power_domain_info[i];
>>> + if (power_domain_info->sst_base)
>>> + tpmi_inst.valid_mask |= BIT(i);
>>> + }
>>> +
>>> + if (copy_to_user(argp, &tpmi_inst, sizeof(tpmi_inst)))
>>> + return -EFAULT;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
>>> + unsigned long arg)
>>> +{
>>> + void __user *argp = (void __user *)arg;
>>> + long ret = -ENOTTY;
>>> +
>>> + mutex_lock(&isst_tpmi_dev_lock);
>>> + switch (cmd) {
>>> + case ISST_IF_COUNT_TPMI_INSTANCES:
>>> + ret = isst_if_get_tpmi_instance_count(argp);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> + mutex_unlock(&isst_tpmi_dev_lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
>>> +{
>>> + struct intel_tpmi_plat_info *plat_info;
>>> + struct tpmi_sst_struct *tpmi_sst;
>>> + int i, pkg = 0, inst = 0;
>>> + int num_resources;
>>> +
>>> + plat_info = tpmi_get_platform_data(auxdev);
>>> + if (!plat_info) {
>>> + dev_err(&auxdev->dev, "No platform info\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + pkg = plat_info->package_id;
>>> + if (pkg >= topology_max_packages()) {
>>> + dev_err(&auxdev->dev, "Invalid package id :%x\n",
>>> pkg);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (isst_common.sst_inst[pkg])
>>> + return -EEXIST;
>>> +
>>> + num_resources = tpmi_get_resource_count(auxdev);
>>> +
>>> + if (!num_resources)
>>> + return -EINVAL;
>>> +
>>> + tpmi_sst = devm_kzalloc(&auxdev->dev, sizeof(*tpmi_sst),
>>> GFP_KERNEL);
>>> + if (!tpmi_sst)
>>> + return -ENOMEM;
>>> +
>>> + tpmi_sst->power_domain_info = devm_kcalloc(&auxdev->dev,
>>> num_resources,
>>> +
>>> sizeof(*tpmi_sst->power_domain_info),
>>> + GFP_KERNEL);
>>> + if (!tpmi_sst->power_domain_info)
>>> + return -ENOMEM;
>>> +
>>> + tpmi_sst->number_of_power_domains = num_resources;
>>> +
>>> + for (i = 0; i < num_resources; ++i) {
>>> + struct resource *res;
>>> +
>>> + res = tpmi_get_resource_at_index(auxdev, i);
>>> + if (!res) {
>>> + tpmi_sst->power_domain_info[i].sst_base =
>>> NULL;
>>> + continue;
>>> + }
>>> +
>>> + tpmi_sst->power_domain_info[i].package_id = pkg;
>>> + tpmi_sst->power_domain_info[i].power_domain_id = i;
>>> + tpmi_sst->power_domain_info[i].auxdev = auxdev;
>>> + tpmi_sst->power_domain_info[i].sst_base =
>>> devm_ioremap_resource(&auxdev->dev, res);
>>> + if (IS_ERR(tpmi_sst-
>>>> power_domain_info[i].sst_base))
>>> + return PTR_ERR(tpmi_sst-
>>>> power_domain_info[i].sst_base);
>>> +
>>> + ++inst;
>>> + }
>>> +
>>> + if (!inst)
>>> + return -ENODEV;
>>> +
>>> + tpmi_sst->package_id = pkg;
>>> + auxiliary_set_drvdata(auxdev, tpmi_sst);
>>> +
>>> + mutex_lock(&isst_tpmi_dev_lock);
>>> + if (isst_common.max_index < pkg)
>>> + isst_common.max_index = pkg;
>>> + isst_common.sst_inst[pkg] = tpmi_sst;
>>> + mutex_unlock(&isst_tpmi_dev_lock);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_dev_add, INTEL_TPMI_SST);
>>> +
>>> +void tpmi_sst_dev_remove(struct auxiliary_device *auxdev)
>>> +{
>>> + struct tpmi_sst_struct *tpmi_sst =
>>> auxiliary_get_drvdata(auxdev);
>>> +
>>> + mutex_lock(&isst_tpmi_dev_lock);
>>> + isst_common.sst_inst[tpmi_sst->package_id] = NULL;
>>> + mutex_unlock(&isst_tpmi_dev_lock);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_dev_remove, INTEL_TPMI_SST);
>>> +
>>> +#define ISST_TPMI_API_VERSION 0x02
>>> +
>>> +int tpmi_sst_init(void)
>>> +{
>>> + struct isst_if_cmd_cb cb;
>>> + int ret = 0;
>>> +
>>> + mutex_lock(&isst_tpmi_dev_lock);
>>> +
>>> + if (isst_core_usage_count) {
>>> + ++isst_core_usage_count;
>>> + goto init_done;
>>> + }
>>> +
>>> + isst_common.sst_inst = kcalloc(topology_max_packages(),
>>> +
>>> sizeof(*isst_common.sst_inst),
>>> + GFP_KERNEL);
>>> + if (!isst_common.sst_inst)
>>> + return -ENOMEM;
>>> +
>>> + memset(&cb, 0, sizeof(cb));
>>> + cb.cmd_size = sizeof(struct isst_if_io_reg);
>>> + cb.offset = offsetof(struct isst_if_io_regs, io_reg);
>>> + cb.cmd_callback = NULL;
>>> + cb.api_version = ISST_TPMI_API_VERSION;
>>> + cb.def_ioctl = isst_if_def_ioctl;
>>> + cb.owner = THIS_MODULE;
>>> + ret = isst_if_cdev_register(ISST_IF_DEV_TPMI, &cb);
>>> + if (ret)
>>> + kfree(isst_common.sst_inst);
>>> +init_done:
>>> + mutex_unlock(&isst_tpmi_dev_lock);
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_init, INTEL_TPMI_SST);
>>> +
>>> +void tpmi_sst_exit(void)
>>> +{
>>> + mutex_lock(&isst_tpmi_dev_lock);
>>> + if (isst_core_usage_count)
>>> + --isst_core_usage_count;
>>> +
>>> + if (!isst_core_usage_count) {
>>> + isst_if_cdev_unregister(ISST_IF_DEV_TPMI);
>>> + kfree(isst_common.sst_inst);
>>> + }
>>> + mutex_unlock(&isst_tpmi_dev_lock);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_exit, INTEL_TPMI_SST);
>>> +
>>> +MODULE_IMPORT_NS(INTEL_TPMI);
>>> +MODULE_IMPORT_NS(INTEL_TPMI_POWER_DOMAIN);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> diff --git
>>> a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> new file mode 100644
>>> index 000000000000..356cb02273b1
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Intel Speed Select Interface: Drivers Internal defines
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All rights reserved.
>>> + *
>>> + */
>>> +
>>> +#ifndef _ISST_TPMI_CORE_H
>>> +#define _ISST_TPMI_CORE_H
>>> +
>>> +int tpmi_sst_init(void);
>>> +void tpmi_sst_exit(void);
>>> +int tpmi_sst_dev_add(struct auxiliary_device *auxdev);
>>> +void tpmi_sst_dev_remove(struct auxiliary_device *auxdev);
>>> +#endif
>>> diff --git a/include/uapi/linux/isst_if.h
>>> b/include/uapi/linux/isst_if.h
>>> index ba078f8e9add..bf32d959f6e8 100644
>>> --- a/include/uapi/linux/isst_if.h
>>> +++ b/include/uapi/linux/isst_if.h
>>> @@ -163,10 +163,28 @@ struct isst_if_msr_cmds {
>>> struct isst_if_msr_cmd msr_cmd[1];
>>> };
>>>
>>> +/**
>>> + * struct isst_tpmi_instance_count - Get number of TPMI instances
>>> per socket
>>> + * @socket_id: Socket/package id
>>> + * @count: Number of instances
>>> + * @valid_mask: Mask of instances as there can be holes
>>> + *
>>> + * Structure used to get TPMI instances information using
>>> + * IOCTL ISST_IF_COUNT_TPMI_INSTANCES.
>>> + */
>>> +struct isst_tpmi_instance_count {
>>> + __u8 socket_id;
>>> + __u8 count;
>>> + __u16 valid_mask;
>>> +};
>>> +
>>> #define ISST_IF_MAGIC 0xFE
>>> #define ISST_IF_GET_PLATFORM_INFO _IOR(ISST_IF_MAGIC, 0,
>>> struct isst_if_platform_info *)
>>> #define ISST_IF_GET_PHY_ID _IOWR(ISST_IF_MAGIC, 1,
>>> struct isst_if_cpu_map *)
>>> #define ISST_IF_IO_CMD _IOW(ISST_IF_MAGIC, 2, struct
>>> isst_if_io_regs *)
>>> #define ISST_IF_MBOX_COMMAND _IOWR(ISST_IF_MAGIC, 3, struct
>>> isst_if_mbox_cmds *)
>>> #define ISST_IF_MSR_COMMAND _IOWR(ISST_IF_MAGIC, 4, struct
>>> isst_if_msr_cmds *)
>>> +
>>> +#define ISST_IF_COUNT_TPMI_INSTANCES _IOR(ISST_IF_MAGIC, 5,
>>> struct isst_tpmi_instance_count *)
>>> +
>>> #endif
>>
>