Message-ID: <22b443fd-b415-9550-76d3-a897dde9805e@redhat.com>
Date:   Wed, 1 Mar 2023 15:37:16 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 06/12] platform/x86: ISST: Enumerate TPMI SST and create
 framework
Content-Language: en-US, nl
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        markgross@kernel.org
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230211063257.311746-1-srinivas.pandruvada@linux.intel.com>
 <20230211063257.311746-7-srinivas.pandruvada@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230211063257.311746-7-srinivas.pandruvada@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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 ?

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.

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