[PATCH v2 0/5] TPMI update for permissions

[PATCH v2 0/5] TPMI update for permissions

[Srinivas Pandruvada]

Process read/write and enabled state for feature drivers. When a feature
is disabled, don't create a device to load a feature driver. When a read
is blocked then don't load feature drivers. When write is blocked continue
to function in read only mode.

v2:
	Dropped patch platform/x86/intel/tpmi: Add additional TPMI header fields
	Addressed other review comments, details are in each patch

Srinivas Pandruvada (5):
  platform/x86/intel/tpmi: Don't create devices for disabled features
  platform/x86/intel/tpmi: Modify external interface to get read/write
    state
  platform/x86/intel/tpmi: Move TPMI ID definition
  platform/x86: ISST: Process read/write blocked feature status
  platform/x86/intel-uncore-freq: Process read/write blocked feature
    status

 .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++
 drivers/platform/x86/intel/tpmi.c             | 35 +++++++++----------
 .../uncore-frequency/uncore-frequency-tpmi.c  | 15 ++++++++
 include/linux/intel_tpmi.h                    | 18 ++++++++--
 4 files changed, 72 insertions(+), 21 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] platform/x86/intel/tpmi: Don't create devices for disabled features

[Srinivas Pandruvada]

If some TPMI features are disabled, don't create auxiliary devices. In
this way feature drivers will not load.

While creating auxiliary devices, call tpmi_read_feature_status() to
check feature state and return if the feature is disabled without
creating a device.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
v2
- Add comment for returning -EOPNOTSUPP

 drivers/platform/x86/intel/tpmi.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index c89aa4d14bea..868067ff966e 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -604,9 +604,21 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
 	struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
 	char feature_id_name[TPMI_FEATURE_NAME_LEN];
 	struct intel_vsec_device *feature_vsec_dev;
+	struct tpmi_feature_state feature_state;
 	struct resource *res, *tmp;
 	const char *name;
-	int i;
+	int i, ret;
+
+	ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state);
+	if (ret)
+		return ret;
+
+	/*
+	 * If not enabled, continue to look at other features in the PFS, so return -EOPNOTSUPP.
+	 * This will not cause failure of loading of this driver.
+	 */
+	if (!feature_state.enabled)
+		return -EOPNOTSUPP;
 
 	name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
 	if (!name)
-- 
2.41.0

[PATCH v2 2/5] platform/x86/intel/tpmi: Modify external interface to get read/write state

[Srinivas Pandruvada]

Modify the external interface tpmi_get_feature_status() to get read
and write blocked instead of locked and disabled. Since auxiliary device
is not created when disabled, no use of returning disabled state. Also
locked state is not useful as feature driver can't use locked state
in a meaningful way.

Using read and write state, feature driver can decide which operations
to restrict for that feature.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v2:
- Change read_blocked/write_blocked to bool *

 drivers/platform/x86/intel/tpmi.c | 8 ++++----
 include/linux/intel_tpmi.h        | 5 ++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index 868067ff966e..ec64565baddc 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -351,8 +351,8 @@ static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int featu
 	return ret;
 }
 
-int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
-			    int *locked, int *disabled)
+int tpmi_get_feature_status(struct auxiliary_device *auxdev,
+			    int feature_id, bool *read_blocked, bool *write_blocked)
 {
 	struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(auxdev->dev.parent);
 	struct intel_tpmi_info *tpmi_info = auxiliary_get_drvdata(&intel_vsec_dev->auxdev);
@@ -363,8 +363,8 @@ int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
 	if (ret)
 		return ret;
 
-	*locked = feature_state.locked;
-	*disabled = !feature_state.enabled;
+	*read_blocked = feature_state.read_blocked;
+	*write_blocked = feature_state.write_blocked;
 
 	return 0;
 }
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index 939663bb095f..7dff3d96a200 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -38,7 +38,6 @@ struct intel_tpmi_plat_info {
 struct intel_tpmi_plat_info *tpmi_get_platform_data(struct auxiliary_device *auxdev);
 struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int index);
 int tpmi_get_resource_count(struct auxiliary_device *auxdev);
-
-int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, int *locked,
-			    int *disabled);
+int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, bool *read_blocked,
+			    bool *write_blocked);
 #endif
-- 
2.41.0

[PATCH v2 3/5] platform/x86/intel/tpmi: Move TPMI ID definition

[Srinivas Pandruvada]

Move TPMI ID definitions to common include file. In this way other
feature drivers don't have to redefine.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v2
- Aligned comments for TPMI IDs

 drivers/platform/x86/intel/tpmi.c | 13 -------------
 include/linux/intel_tpmi.h        | 13 +++++++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index ec64565baddc..7ea4cbf5b3a3 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -176,19 +176,6 @@ struct tpmi_feature_state {
 	u32 locked:1;
 } __packed;
 
-/*
- * List of supported TMPI IDs.
- * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
- */
-enum intel_tpmi_id {
-	TPMI_ID_RAPL = 0, /* Running Average Power Limit */
-	TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */
-	TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */
-	TPMI_ID_SST = 5, /* Speed Select Technology */
-	TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */
-	TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */
-};
-
 /*
  * The size from hardware is in u32 units. This size is from a trusted hardware,
  * but better to verify for pre silicon platforms. Set size to 0, when invalid.
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index 7dff3d96a200..612b6a215e81 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -12,6 +12,19 @@
 #define TPMI_MINOR_VERSION(val)	FIELD_GET(GENMASK(4, 0), val)
 #define TPMI_MAJOR_VERSION(val)	FIELD_GET(GENMASK(7, 5), val)
 
+/*
+ * List of supported TMPI IDs.
+ * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
+ */
+enum intel_tpmi_id {
+	TPMI_ID_RAPL = 0,	/* Running Average Power Limit */
+	TPMI_ID_PEM = 1,	/* Power and Perf excursion Monitor */
+	TPMI_ID_UNCORE = 2,	/* Uncore Frequency Scaling */
+	TPMI_ID_SST = 5,	/* Speed Select Technology */
+	TPMI_CONTROL_ID = 0x80,	/* Special ID for getting feature status */
+	TPMI_INFO_ID = 0x81,	/* Special ID for PCI BDF and Package ID information */
+};
+
 /**
  * struct intel_tpmi_plat_info - Platform information for a TPMI device instance
  * @cdie_mask:	Mask of all compute dies in the partition
-- 
2.41.0

[PATCH v2 4/5] platform/x86: ISST: Process read/write blocked feature status

[Srinivas Pandruvada]

When a feature is read blocked, don't continue to read SST information
and register with SST core.

When the feature is write blocked, continue to offer read interface for
SST parameters, but don't allow any operation to change state. A state
change results from SST level change, feature change or class of service
change.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
v2
- Change read_blocked, write_blocked to bool
- Move the check for power_domain_info->write_blocked for SST-CP
to only write operations

 .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

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
index 0b6d2c864437..2662fbbddf0c 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
@@ -234,6 +234,7 @@ struct perf_level {
  * @saved_clos_configs:	Save SST-CP CLOS configuration to store restore for suspend/resume
  * @saved_clos_assocs:	Save SST-CP CLOS association to store restore for suspend/resume
  * @saved_pp_control:	Save SST-PP control information to store restore for suspend/resume
+ * @write_blocked:	Write operation is blocked, so can't change SST state
  *
  * 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
@@ -259,6 +260,7 @@ struct tpmi_per_power_domain_info {
 	u64 saved_clos_configs[4];
 	u64 saved_clos_assocs[4];
 	u64 saved_pp_control;
+	bool write_blocked;
 };
 
 /**
@@ -515,6 +517,9 @@ static long isst_if_clos_param(void __user *argp)
 		return -EINVAL;
 
 	if (clos_param.get_set) {
+		if (power_domain_info->write_blocked)
+			return -EPERM;
+
 		_write_cp_info("clos.min_freq", clos_param.min_freq_mhz,
 			       (SST_CLOS_CONFIG_0_OFFSET + clos_param.clos * SST_REG_SIZE),
 			       SST_CLOS_CONFIG_MIN_START, SST_CLOS_CONFIG_MIN_WIDTH,
@@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user *argp)
 
 		power_domain_info = &sst_inst->power_domain_info[punit_id];
 
+		if (assoc_cmds.get_set && power_domain_info->write_blocked)
+			return -EPERM;
+
 		offset = SST_CLOS_ASSOC_0_OFFSET +
 				(punit_cpu_no / SST_CLOS_ASSOC_CPUS_PER_REG) * SST_REG_SIZE;
 		shift = punit_cpu_no % SST_CLOS_ASSOC_CPUS_PER_REG;
@@ -752,6 +760,9 @@ static int isst_if_set_perf_level(void __user *argp)
 	if (!power_domain_info)
 		return -EINVAL;
 
+	if (power_domain_info->write_blocked)
+		return -EPERM;
+
 	if (!(power_domain_info->pp_header.allowed_level_mask & BIT(perf_level.level)))
 		return -EINVAL;
 
@@ -809,6 +820,9 @@ static int isst_if_set_perf_feature(void __user *argp)
 	if (!power_domain_info)
 		return -EINVAL;
 
+	if (power_domain_info->write_blocked)
+		return -EPERM;
+
 	_write_pp_info("perf_feature", perf_feature.feature, SST_PP_CONTROL_OFFSET,
 		       SST_PP_FEATURE_STATE_START, SST_PP_FEATURE_STATE_WIDTH,
 		       SST_MUL_FACTOR_NONE)
@@ -1257,11 +1271,21 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
 
 int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
 {
+	bool read_blocked = 0, write_blocked = 0;
 	struct intel_tpmi_plat_info *plat_info;
 	struct tpmi_sst_struct *tpmi_sst;
 	int i, ret, pkg = 0, inst = 0;
 	int num_resources;
 
+	ret = tpmi_get_feature_status(auxdev, TPMI_ID_SST, &read_blocked, &write_blocked);
+	if (ret)
+		dev_info(&auxdev->dev, "Can't read feature status: ignoring read/write blocked status\n");
+
+	if (read_blocked) {
+		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
+		return -ENODEV;
+	}
+
 	plat_info = tpmi_get_platform_data(auxdev);
 	if (!plat_info) {
 		dev_err(&auxdev->dev, "No platform info\n");
@@ -1306,6 +1330,7 @@ int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
 		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].write_blocked = write_blocked;
 		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);
-- 
2.41.0

[PATCH v2 5/5] platform/x86/intel-uncore-freq: Process read/write blocked feature status

[Srinivas Pandruvada]

When a feature is read blocked, don't continue to read uncore information
and register with uncore core.

When the feature is write blocked, continue to offer read interface but
block setting uncore limits.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v2
- Change read_blocked, write_blocked to bool

 .../uncore-frequency/uncore-frequency-tpmi.c      | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index 4fb790552c47..bd75d61ff8a6 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -66,6 +66,7 @@ struct tpmi_uncore_struct {
 	int min_ratio;
 	struct tpmi_uncore_power_domain_info *pd_info;
 	struct tpmi_uncore_cluster_info root_cluster;
+	bool write_blocked;
 };
 
 #define UNCORE_GENMASK_MIN_RATIO	GENMASK_ULL(21, 15)
@@ -157,6 +158,9 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
 	uncore_root = cluster_info->uncore_root;
 
+	if (uncore_root->write_blocked)
+		return -EPERM;
+
 	/* Update each cluster in a package */
 	if (cluster_info->root_domain) {
 		struct tpmi_uncore_struct *uncore_root = cluster_info->uncore_root;
@@ -233,11 +237,21 @@ static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
 
 static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
 {
+	bool read_blocked = 0, write_blocked = 0;
 	struct intel_tpmi_plat_info *plat_info;
 	struct tpmi_uncore_struct *tpmi_uncore;
 	int ret, i, pkg = 0;
 	int num_resources;
 
+	ret = tpmi_get_feature_status(auxdev, TPMI_ID_UNCORE, &read_blocked, &write_blocked);
+	if (ret)
+		dev_info(&auxdev->dev, "Can't read feature status: ignoring blocked status\n");
+
+	if (read_blocked) {
+		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
+		return -ENODEV;
+	}
+
 	/* Get number of power domains, which is equal to number of resources */
 	num_resources = tpmi_get_resource_count(auxdev);
 	if (!num_resources)
@@ -266,6 +280,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
 	}
 
 	tpmi_uncore->power_domain_count = num_resources;
+	tpmi_uncore->write_blocked = write_blocked;
 
 	/* Get the package ID from the TPMI core */
 	plat_info = tpmi_get_platform_data(auxdev);
-- 
2.41.0

Re: [PATCH v2 0/5] TPMI update for permissions

[srinivas pandruvada]

On Thu, 2023-11-30 at 13:47 -0800, Srinivas Pandruvada wrote:
> Process read/write and enabled state for feature drivers. When a
> feature
> is disabled, don't create a device to load a feature driver. When a
> read
> is blocked then don't load feature drivers. When write is blocked
> continue
> to function in read only mode.
> 
> v2:
>         Dropped patch platform/x86/intel/tpmi: Add additional TPMI
> header fields
>         Addressed other review comments, details are in each patch
> 
lkp@intel.com is complaining that these patches can't be applied. I
will fix any issue after review and post v3 if required.

Thanks,
Srinivas


> Srinivas Pandruvada (5):
>   platform/x86/intel/tpmi: Don't create devices for disabled features
>   platform/x86/intel/tpmi: Modify external interface to get
> read/write
>     state
>   platform/x86/intel/tpmi: Move TPMI ID definition
>   platform/x86: ISST: Process read/write blocked feature status
>   platform/x86/intel-uncore-freq: Process read/write blocked feature
>     status
> 
>  .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++
>  drivers/platform/x86/intel/tpmi.c             | 35 +++++++++--------
> --
>  .../uncore-frequency/uncore-frequency-tpmi.c  | 15 ++++++++
>  include/linux/intel_tpmi.h                    | 18 ++++++++--
>  4 files changed, 72 insertions(+), 21 deletions(-)
> 

Re: [PATCH v2 1/5] platform/x86/intel/tpmi: Don't create devices for disabled features

[Hans de Goede]

Hi,

On 11/30/23 22:47, Srinivas Pandruvada wrote:
> If some TPMI features are disabled, don't create auxiliary devices. In
> this way feature drivers will not load.
> 
> While creating auxiliary devices, call tpmi_read_feature_status() to
> check feature state and return if the feature is disabled without
> creating a device.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks, patch looks good to me:

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

Regards,

Hans


> ---
> v2
> - Add comment for returning -EOPNOTSUPP
> 
>  drivers/platform/x86/intel/tpmi.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index c89aa4d14bea..868067ff966e 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -604,9 +604,21 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  	struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
>  	char feature_id_name[TPMI_FEATURE_NAME_LEN];
>  	struct intel_vsec_device *feature_vsec_dev;
> +	struct tpmi_feature_state feature_state;
>  	struct resource *res, *tmp;
>  	const char *name;
> -	int i;
> +	int i, ret;
> +
> +	ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If not enabled, continue to look at other features in the PFS, so return -EOPNOTSUPP.
> +	 * This will not cause failure of loading of this driver.
> +	 */
> +	if (!feature_state.enabled)
> +		return -EOPNOTSUPP;
>  
>  	name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
>  	if (!name)

Re: [PATCH v2 4/5] platform/x86: ISST: Process read/write blocked feature status

[Hans de Goede]

Hi,

On 11/30/23 22:47, Srinivas Pandruvada wrote:
> When a feature is read blocked, don't continue to read SST information
> and register with SST core.
> 
> When the feature is write blocked, continue to offer read interface for
> SST parameters, but don't allow any operation to change state. A state
> change results from SST level change, feature change or class of service
> change.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2
> - Change read_blocked, write_blocked to bool
> - Move the check for power_domain_info->write_blocked for SST-CP
> to only write operations

Thanks, patch looks good to me:

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

Did you drop Ilpo's Reviewed-by from v1 on purpose
because of the changes ? Or did you forget to add it ?

Regards,

Hans


> 
>  .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> 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
> index 0b6d2c864437..2662fbbddf0c 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> @@ -234,6 +234,7 @@ struct perf_level {
>   * @saved_clos_configs:	Save SST-CP CLOS configuration to store restore for suspend/resume
>   * @saved_clos_assocs:	Save SST-CP CLOS association to store restore for suspend/resume
>   * @saved_pp_control:	Save SST-PP control information to store restore for suspend/resume
> + * @write_blocked:	Write operation is blocked, so can't change SST state
>   *
>   * 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
> @@ -259,6 +260,7 @@ struct tpmi_per_power_domain_info {
>  	u64 saved_clos_configs[4];
>  	u64 saved_clos_assocs[4];
>  	u64 saved_pp_control;
> +	bool write_blocked;
>  };
>  
>  /**
> @@ -515,6 +517,9 @@ static long isst_if_clos_param(void __user *argp)
>  		return -EINVAL;
>  
>  	if (clos_param.get_set) {
> +		if (power_domain_info->write_blocked)
> +			return -EPERM;
> +
>  		_write_cp_info("clos.min_freq", clos_param.min_freq_mhz,
>  			       (SST_CLOS_CONFIG_0_OFFSET + clos_param.clos * SST_REG_SIZE),
>  			       SST_CLOS_CONFIG_MIN_START, SST_CLOS_CONFIG_MIN_WIDTH,
> @@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user *argp)
>  
>  		power_domain_info = &sst_inst->power_domain_info[punit_id];
>  
> +		if (assoc_cmds.get_set && power_domain_info->write_blocked)
> +			return -EPERM;
> +
>  		offset = SST_CLOS_ASSOC_0_OFFSET +
>  				(punit_cpu_no / SST_CLOS_ASSOC_CPUS_PER_REG) * SST_REG_SIZE;
>  		shift = punit_cpu_no % SST_CLOS_ASSOC_CPUS_PER_REG;
> @@ -752,6 +760,9 @@ static int isst_if_set_perf_level(void __user *argp)
>  	if (!power_domain_info)
>  		return -EINVAL;
>  
> +	if (power_domain_info->write_blocked)
> +		return -EPERM;
> +
>  	if (!(power_domain_info->pp_header.allowed_level_mask & BIT(perf_level.level)))
>  		return -EINVAL;
>  
> @@ -809,6 +820,9 @@ static int isst_if_set_perf_feature(void __user *argp)
>  	if (!power_domain_info)
>  		return -EINVAL;
>  
> +	if (power_domain_info->write_blocked)
> +		return -EPERM;
> +
>  	_write_pp_info("perf_feature", perf_feature.feature, SST_PP_CONTROL_OFFSET,
>  		       SST_PP_FEATURE_STATE_START, SST_PP_FEATURE_STATE_WIDTH,
>  		       SST_MUL_FACTOR_NONE)
> @@ -1257,11 +1271,21 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
>  
>  int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
>  {
> +	bool read_blocked = 0, write_blocked = 0;
>  	struct intel_tpmi_plat_info *plat_info;
>  	struct tpmi_sst_struct *tpmi_sst;
>  	int i, ret, pkg = 0, inst = 0;
>  	int num_resources;
>  
> +	ret = tpmi_get_feature_status(auxdev, TPMI_ID_SST, &read_blocked, &write_blocked);
> +	if (ret)
> +		dev_info(&auxdev->dev, "Can't read feature status: ignoring read/write blocked status\n");
> +
> +	if (read_blocked) {
> +		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
> +		return -ENODEV;
> +	}
> +
>  	plat_info = tpmi_get_platform_data(auxdev);
>  	if (!plat_info) {
>  		dev_err(&auxdev->dev, "No platform info\n");
> @@ -1306,6 +1330,7 @@ int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
>  		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].write_blocked = write_blocked;
>  		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);

Re: [PATCH v2 0/5] TPMI update for permissions

[Hans de Goede]

Hi Srinivas,

On 12/1/23 00:24, srinivas pandruvada wrote:
> On Thu, 2023-11-30 at 13:47 -0800, Srinivas Pandruvada wrote:
>> Process read/write and enabled state for feature drivers. When a
>> feature
>> is disabled, don't create a device to load a feature driver. When a
>> read
>> is blocked then don't load feature drivers. When write is blocked
>> continue
>> to function in read only mode.
>>
>> v2:
>>         Dropped patch platform/x86/intel/tpmi: Add additional TPMI
>> header fields
>>         Addressed other review comments, details are in each patch
>>
> lkp@intel.com is complaining that these patches can't be applied. I
> will fix any issue after review and post v3 if required.

Thanks.

I guess this may conflict with or depend on:

"[PATCH V6 00/20] intel_pmc: Add telemetry API to read counters"

which I have just merged into pdx86/review-hans, please make
sure your next version on top of pdx86/review-hans.

Regards,

Hans


> 
> Thanks,
> Srinivas
> 
> 
>> Srinivas Pandruvada (5):
>>   platform/x86/intel/tpmi: Don't create devices for disabled features
>>   platform/x86/intel/tpmi: Modify external interface to get
>> read/write
>>     state
>>   platform/x86/intel/tpmi: Move TPMI ID definition
>>   platform/x86: ISST: Process read/write blocked feature status
>>   platform/x86/intel-uncore-freq: Process read/write blocked feature
>>     status
>>
>>  .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++
>>  drivers/platform/x86/intel/tpmi.c             | 35 +++++++++--------
>> --
>>  .../uncore-frequency/uncore-frequency-tpmi.c  | 15 ++++++++
>>  include/linux/intel_tpmi.h                    | 18 ++++++++--
>>  4 files changed, 72 insertions(+), 21 deletions(-)
>>
> 

Re: [PATCH v2 4/5] platform/x86: ISST: Process read/write blocked feature status

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On Mon, 4 Dec 2023, Hans de Goede wrote:
> On 11/30/23 22:47, Srinivas Pandruvada wrote:
> > When a feature is read blocked, don't continue to read SST information
> > and register with SST core.
> > 
> > When the feature is write blocked, continue to offer read interface for
> > SST parameters, but don't allow any operation to change state. A state
> > change results from SST level change, feature change or class of service
> > change.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> > v2
> > - Change read_blocked, write_blocked to bool
> > - Move the check for power_domain_info->write_blocked for SST-CP
> > to only write operations
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Did you drop Ilpo's Reviewed-by from v1 on purpose
> because of the changes ? Or did you forget to add it ?

No he didn't because this patch had an issue in v1 so I never gave my 
rev-by for this patch. ;-)

But here it's now for v2:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v2 1/5] platform/x86/intel/tpmi: Don't create devices for disabled features

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On Thu, 30 Nov 2023, Srinivas Pandruvada wrote:

> If some TPMI features are disabled, don't create auxiliary devices. In
> this way feature drivers will not load.
> 
> While creating auxiliary devices, call tpmi_read_feature_status() to
> check feature state and return if the feature is disabled without
> creating a device.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2
> - Add comment for returning -EOPNOTSUPP
> 
>  drivers/platform/x86/intel/tpmi.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index c89aa4d14bea..868067ff966e 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -604,9 +604,21 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  	struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
>  	char feature_id_name[TPMI_FEATURE_NAME_LEN];
>  	struct intel_vsec_device *feature_vsec_dev;
> +	struct tpmi_feature_state feature_state;
>  	struct resource *res, *tmp;
>  	const char *name;
> -	int i;
> +	int i, ret;
> +
> +	ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If not enabled, continue to look at other features in the PFS, so return -EOPNOTSUPP.
> +	 * This will not cause failure of loading of this driver.
> +	 */
> +	if (!feature_state.enabled)
> +		return -EOPNOTSUPP;
>  
>  	name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
>  	if (!name)

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v2 4/5] platform/x86: ISST: Process read/write blocked feature status

[Hans de Goede]

Hi,

On 12/4/23 15:11, Ilpo Järvinen wrote:
> On Mon, 4 Dec 2023, Hans de Goede wrote:
>> On 11/30/23 22:47, Srinivas Pandruvada wrote:
>>> When a feature is read blocked, don't continue to read SST information
>>> and register with SST core.
>>>
>>> When the feature is write blocked, continue to offer read interface for
>>> SST parameters, but don't allow any operation to change state. A state
>>> change results from SST level change, feature change or class of service
>>> change.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>> v2
>>> - Change read_blocked, write_blocked to bool
>>> - Move the check for power_domain_info->write_blocked for SST-CP
>>> to only write operations
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Did you drop Ilpo's Reviewed-by from v1 on purpose
>> because of the changes ? Or did you forget to add it ?
> 
> No he didn't because this patch had an issue in v1 so I never gave my 
> rev-by for this patch. ;-)

Ah, there are 2 patches with "Process read/write blocked feature status"
as part of the subject, that confused me :)

> But here it's now for v2:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks & Regards,

Hans