* [PATCH v5 1/8] platform/x86/amd/hsmp: Add HSMP messages for Family 1Ah, Model 50h-5Fh
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 2/8] platform/x86/amd/hsmp: Add UAPI structures for Family 1Ah Model 50h-5Fh metrics table Muralidhara M K
` (6 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Suma Hegde,
Muthusamy Ramalingam
Family 1Ah Model 50h-5Fh firmware exposes new HSMP messages (0x29-0x2A,
0x33-0x3A) for PC6/CC6 control, CCD power/thermal monitoring, DIMM
sideband access, floor- and SDPS-limit control, and command-enable
discovery. The same firmware extends three existing SET-only messages
(HSMP_SET_XGMI_LINK_WIDTH 0x0C, HSMP_SET_DF_PSTATE 0x0D,
HSMP_SET_PSTATE_MAX_MIN 0x22) with a read-back path selected by bit[31]
of args[0] (0 = set, 1 = get). Add the new IDs, convert the three
messages to HSMP_SET_GET, and bump the driver version to 2.6.
Also add PQoS-related HSMP messages HSMP_PQOS_TRAFFIC_PRIORITY (0x3B)
and HSMP_PQOS_FLOATING_BW (0x3C) with matching hsmp_msg_desc_table[]
descriptors so userspace can reach the new functionality.
Backward compatibility is preserved on prior platforms: new IDs
previously occupied HSMP_RSVD slots, and existing userspace that leaves
bit[31] = 0 continues to take a pure SET path.
Reviewed-by: Suma Hegde <suma.hegde@amd.com>
Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
arch/x86/include/uapi/asm/amd_hsmp.h | 169 ++++++++++++++++++++++++---
drivers/platform/x86/amd/hsmp/hsmp.h | 2 +-
2 files changed, 156 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index 92d8f256d096..ff8b6defec1d 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -53,9 +53,21 @@ enum hsmp_message_ids {
HSMP_SET_XGMI_PSTATE_RANGE, /* 26h Set xGMI P-state range */
HSMP_CPU_RAIL_ISO_FREQ_POLICY, /* 27h Get/Set Cpu Iso frequency policy */
HSMP_DFC_ENABLE_CTRL, /* 28h Enable/Disable DF C-state */
+ HSMP_PC6_ENABLE, /* 29h Get/Set PC6 enable/disable status */
+ HSMP_CC6_ENABLE, /* 2Ah Get/Set CC6 enable/disable status */
HSMP_GET_RAPL_UNITS = 0x30, /* 30h Get scaling factor for energy */
HSMP_GET_RAPL_CORE_COUNTER, /* 31h Get core energy counter value */
HSMP_GET_RAPL_PACKAGE_COUNTER, /* 32h Get package energy counter value */
+ HSMP_DIMM_SB_RD, /* 33h Get DIMM sideband data */
+ HSMP_READ_CCD_POWER, /* 34h Get average CCD power */
+ HSMP_READ_TDELTA, /* 35h Get thermal behaviour */
+ HSMP_GET_SVI3_VR_CTRL_TEMP, /* 36h Get SVI3 VR controller rail temp */
+ HSMP_GET_ENABLED_HSMP_CMDS, /* 37h Get supported HSMP commands */
+ HSMP_SET_GET_FLOOR_LIMIT, /* 38h Get/Set core floor frequency limit */
+ HSMP_DIMM_SB_WR, /* 39h Set DIMM sideband data */
+ HSMP_SDPS_LIMIT, /* 3Ah Get/Set SDPS limit */
+ HSMP_PQOS_TRAFFIC_PRIORITY, /* 3Bh Get/Set traffic priority */
+ HSMP_PQOS_FLOATING_BW, /* 3Ch Get/Set max floating bandwidth */
HSMP_MSG_ID_MAX,
};
@@ -170,16 +182,27 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[]
{0, 1, HSMP_GET},
/*
- * HSMP_SET_XGMI_LINK_WIDTH, num_args = 1, response_sz = 0
- * input: args[0] = min link width[15:8] + max link width[7:0]
+ * HSMP_SET_XGMI_LINK_WIDTH, num_args = 1, response_sz = 0/1
+ * input: args[0] = set/get XGMI Link width[31] (0 = set, 1 = get) +
+ * min link width[15:8] + max link width[7:0]
+ * Link width encoding: 0 = x4, 1 = x8, 2 = x16.
+ * On SET, max must be >= min. On GET, [15:0] are reserved.
+ * output: args[0] = reserved[31:16] + min link width[15:8] +
+ * max link width[7:0]
*/
- {1, 0, HSMP_SET},
+ {1, 1, HSMP_SET_GET},
/*
- * HSMP_SET_DF_PSTATE, num_args = 1, response_sz = 0
- * input: args[0] = df pstate[7:0]
+ * HSMP_SET_DF_PSTATE (APBDisable), num_args = 1, response_sz = 0/1
+ * input: args[0] = set APB_DISABLE / get APB state[31]
+ * (0 = set & lock DF P-state, 1 = get) +
+ * reserved[30:8] +
+ * DF P-state[7:0] (0..2; reserved on GET)
+ * output: args[0] = reserved[31:9] +
+ * APB state[8] (1 = disabled, 0 = enabled) +
+ * locked DF P-state[7:0] if [8] = 1, else reserved
*/
- {1, 0, HSMP_SET},
+ {1, 1, HSMP_SET_GET},
/* HSMP_SET_AUTO_DF_PSTATE, num_args = 0, response_sz = 0 */
{0, 0, HSMP_SET},
@@ -305,16 +328,32 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[]
{1, 1, HSMP_SET},
/*
- * HSMP_SET_POWER_MODE, num_args = 1, response_sz = 0
- * input: args[0] = power efficiency mode[2:0]
+ * HSMP_SET_POWER_MODE (PwrEfficiencyModeSelection),
+ * num_args = 1, response_sz = 1
+ * input: args[0] = set/get policy[31] (0 = set, 1 = get) +
+ * high util point[30:24] +
+ * low util point[23:17] +
+ * PPT limit[16:5] +
+ * reserved[4:3] + mode selection[2:0]
+ * [30:5] are valid only when [2:0] is a balanced core mode
+ * (4 or 5). [2:0] is reserved when getting (bit[31] = 1).
+ * output: args[0] same layout, [31] reserved, [2:0] = arbitrated
+ * current efficiency mode.
*/
{1, 1, HSMP_SET_GET},
/*
- * HSMP_SET_PSTATE_MAX_MIN, num_args = 1, response_sz = 0
- * input: args[0] = min df pstate[15:8] + max df pstate[7:0]
+ * HSMP_SET_PSTATE_MAX_MIN (DfPstateRange), num_args = 1, response_sz = 0/1
+ * input: args[0] = set/get DF P-state range[31] (0 = set, 1 = get) +
+ * reserved[30:16] +
+ * min DF P-state[15:8] + max DF P-state[7:0]
+ * DF P-state encoding: 0 = DFP0 (high performance),
+ * 1 = DFP1, 2 = DFP2 (low performance).
+ * [15:0] are reserved when getting (args[0] bit[31] = 1).
+ * output: args[0] = reserved[31:16] + min DF P-state[15:8] +
+ * max DF P-state[7:0]
*/
- {1, 0, HSMP_SET},
+ {1, 1, HSMP_SET_GET},
/*
* HSMP_GET_METRIC_TABLE_VER, num_args = 0, response_sz = 1
@@ -355,9 +394,31 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[]
*/
{1, 1, HSMP_SET_GET},
- /* RESERVED(0x29-0x2f) */
- {0, 0, HSMP_RSVD},
- {0, 0, HSMP_RSVD},
+ /*
+ * HSMP_PC6_ENABLE (Pc6Enable), num_args = 1, response_sz = 0/1
+ * input: args[0] = set/get PC6 control[31] (0 = set, 1 = get) +
+ * reserved[30:1] +
+ * enable PC6[0] (0 = disable, 1 = enable;
+ * reserved on GET)
+ * output: args[0] = reserved[31:1] + current PC6 control[0]
+ * (last value configured via HSMP or APML)
+ */
+ {1, 1, HSMP_SET_GET},
+
+ /*
+ * HSMP_CC6_ENABLE (CC6Enable), num_args = 1, response_sz = 0/1
+ * Configures CC6 enable for all cores; changing the setting does
+ * not by itself transition cores in or out of CC6.
+ * input: args[0] = set/get CC6 control[31] (0 = set, 1 = get) +
+ * reserved[30:1] +
+ * enable CC6[0] (0 = disable, 1 = enable;
+ * reserved on GET)
+ * output: args[0] = reserved[31:1] + current CC6 control[0]
+ * (last value configured via HSMP or APML)
+ */
+ {1, 1, HSMP_SET_GET},
+
+ /* RESERVED(0x2B-0x2F) */
{0, 0, HSMP_RSVD},
{0, 0, HSMP_RSVD},
{0, 0, HSMP_RSVD},
@@ -385,6 +446,86 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[]
*/
{0, 2, HSMP_GET},
+ /*
+ * HSMP_DIMM_SB_RD, num_args = 1, response_sz = 1
+ * input: args[0] = reg space[23] + reg offset[22:12] +
+ * device LID[11:8] + DIMM address[7:0]
+ * output: args[0] = read data byte[3:0]
+ */
+ {1, 1, HSMP_GET},
+
+ /*
+ * HSMP_READ_CCD_POWER, num_args = 1, response_sz = 1
+ * input: args[0] = apic id of core[15:0]
+ * output: args[0] = CCD power(mWatts)[31:0]
+ */
+ {1, 1, HSMP_GET},
+
+ /*
+ * HSMP_READ_TDELTA, num_args = 0, response_sz = 1
+ * output: args[0] = thermal behaviour[31:0]
+ */
+ {0, 1, HSMP_GET},
+
+ /*
+ * HSMP_GET_SVI3_VR_CTRL_TEMP, num_args = 1, response_sz = 1
+ * input: args[0] = SVI3 rail index[3:1] + read temperature[0]
+ * output: args[0] = SVI3 rail index[30:28] +
+ * rail temperature in degree C[27:0]
+ */
+ {1, 1, HSMP_GET},
+
+ /*
+ * HSMP_GET_ENABLED_HSMP_CMDS, num_args = 1, response_sz = 3
+ * input: args[0] = HSMP command mask[0]
+ * output: status of HSMP command = args[0], args[1], args[2]
+ */
+ {1, 3, HSMP_GET},
+
+ /*
+ * HSMP_SET_GET_FLOOR_LIMIT, num_args = 1, response_sz = 1
+ * input: args[0] = op[31:30] + reserved[29:28] +
+ * apic id[27:16] + floor frequency MHz[15:0]
+ * op encoding: 00 = set per-core floor,
+ * 01 = set all-cores floor (apic id reserved),
+ * 10 = get per-core floor,
+ * 11 = get per-core effective floor.
+ * Floor frequency field is reserved on GET (bit[31] = 1).
+ * output: args[0] = floor frequency MHz[15:0]
+ * (effective for op 11, configured for op 10;
+ * reserved on SET)
+ */
+ {1, 1, HSMP_SET_GET},
+
+ /*
+ * HSMP_DIMM_SB_WR, num_args = 1, response_sz = 0
+ * input: args[0] = write data[31:24] + reg space[23] +
+ * reg offset[22:12] + device LID[11:8] +
+ * DIMM address[7:0]
+ */
+ {1, 0, HSMP_SET},
+
+ /*
+ * HSMP_SDPS_LIMIT, num_args = 1, response_sz = 1
+ * input: args[0] = Set/Get[31] + SDPS Limit[30:0]
+ * output: args[0] = SDPS Limit[30:0]
+ */
+ {1, 1, HSMP_SET_GET},
+
+ /*
+ * HSMP_PQOS_TRAFFIC_PRIORITY, num_args = 1, response_sz = 1
+ * input: args[0] = Op[31:30] + Priority sel[27:26] + Priority val[21:20] + Input[19:0]
+ * output: args[0] = Supported priorities or Priority val[1:0]
+ */
+ {1, 1, HSMP_SET_GET},
+
+ /*
+ * HSMP_PQOS_FLOATING_BW, num_args = 1, response_sz = 2
+ * input: args[0] = Op[31] + Sub-op[30:29] + params[28:0]
+ * output: args[0] = Discovery bits or Floating/Global memory BW (Gbps)
+ * output: args[1] = Reserved or config (drop adj, sampling delay, hysteresis)
+ */
+ {1, 2, HSMP_SET_GET},
};
/* Metrics table (supported only with proto version 6) */
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index 0509a442eaae..b153527e0a0d 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -27,7 +27,7 @@
#define HSMP_DEVNODE_NAME "hsmp"
#define ACPI_HSMP_DEVICE_HID "AMDI0097"
-#define DRIVER_VERSION "2.5"
+#define DRIVER_VERSION "2.6"
struct hsmp_mbaddr_info {
u32 base_addr;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 2/8] platform/x86/amd/hsmp: Add UAPI structures for Family 1Ah Model 50h-5Fh metrics table
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 1/8] platform/x86/amd/hsmp: Add HSMP messages for Family 1Ah, Model 50h-5Fh Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 3/8] platform/x86/amd/hsmp: Unify response_sz validation to an upper-bound check Muralidhara M K
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Suma Hegde,
Muthusamy Ramalingam
HSMP protocol version 7 on Family 1Ah Model 50h-5Fh delivers a new
per-IOD/per-CCD metric-table layout that is not compatible with the
existing hsmp_metric_table. Userspace tooling needs a stable UAPI
description of this layout to interpret the firmware blob carried by
the HSMP character device.
Add hsmp_metric_table_zen6_iod, hsmp_metric_table_zen6_ccd and the
top-level hsmp_metric_table_zen6 that mirror the firmware-defined
binary layout 1:1. The ccd[] array is sized to the hardware maximum
(HSMP_F1A_M50_M5F_MAX_CCDS = 8) to keep that mirror exact; userspace
iterates ccd[0 .. iod.num_active_ccds - 1] and ignores the rest.
num_active_ccds lives in the IOD block because the IOD die is the
aggregator that reports the CCD population, and keeping it there
preserves the 1:1 mapping with the firmware layout.
Driver enablement for protocol version 7 is added in a follow-up
patch.
Reviewed-by: Suma Hegde <suma.hegde@amd.com>
Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
arch/x86/include/uapi/asm/amd_hsmp.h | 93 ++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index ff8b6defec1d..34e25d9048d5 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -608,6 +608,99 @@ struct hsmp_metric_table {
__u32 gfxclk_frequency[8];
};
+#define HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD 32
+#define HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE 4
+#define HSMP_F1A_M50_M5F_MAX_XGMI_LINKS 8
+#define HSMP_F1A_M50_M5F_MAX_PCIE_LINKS 8
+#define HSMP_F1A_M50_M5F_MAX_CCDS 8
+
+/* Metrics table (supported only with proto version 7) */
+struct hsmp_metric_table_zen6_iod {
+ __u32 num_active_ccds; /* Number of valid entries in the sibling ccd[] array */
+ __u32 accumulation_counter;
+
+ /* TEMPERATURE */
+ __u64 max_socket_temperature_acc;
+
+ /* POWER */
+ __u32 socket_power_limit;
+ __u32 max_socket_power_limit;
+ __u64 socket_power_acc;
+ __u64 core_power_acc;
+ __u64 uncore_power_acc;
+
+ /* ENERGY */
+ __u64 timestamp;
+ __u64 socket_energy_acc;
+ __u64 core_energy_acc;
+ __u64 uncore_energy_acc;
+
+ /* FREQUENCY */
+ __u64 fclk_frequency_acc;
+ __u64 uclk_frequency_acc;
+ __u64 ddr_rate_acc;
+ __u64 lclk_frequency_acc[HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE];
+
+ /* FREQUENCY RANGE */
+ __u32 fclk_frequency_table[HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE];
+ __u32 uclk_frequency_table[HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE];
+ __u32 ddr_rate_table[HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE];
+ __u32 max_df_pstate_range;
+ __u32 min_df_pstate_range;
+ __u32 lclk_frequency_table[HSMP_F1A_M50_M5F_MAX_FREQ_TABLE_SIZE];
+ __u32 max_lclk_dpm_range;
+ __u32 min_lclk_dpm_range;
+
+ /* XGMI */
+ __u64 xgmi_bit_rate[HSMP_F1A_M50_M5F_MAX_XGMI_LINKS];
+ __u64 xgmi_read_bandwidth[HSMP_F1A_M50_M5F_MAX_XGMI_LINKS];
+ __u64 xgmi_write_bandwidth[HSMP_F1A_M50_M5F_MAX_XGMI_LINKS];
+
+ /* ACTIVITY */
+ __u64 socket_c0_residency_acc;
+ __u64 socket_df_cstate_residency_acc;
+ __u64 dram_read_bandwidth_acc;
+ __u64 dram_write_bandwidth_acc;
+ __u32 max_dram_bandwidth;
+ __u64 pcie_bandwidth_acc[HSMP_F1A_M50_M5F_MAX_PCIE_LINKS];
+
+ /* THROTTLERS */
+ __u32 prochot_residency_acc;
+ __u32 ppt_residency_acc;
+ __u32 thm_residency_acc;
+ __u32 vrhot_residency_acc;
+ __u32 cpu_tdc_residency_acc;
+ __u32 soc_tdc_residency_acc;
+ __u32 io_mem_tdc_residency_acc;
+ __u32 fit_residency_acc;
+};
+
+struct hsmp_metric_table_zen6_ccd {
+ __u32 core_apicid_of_thread0[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_c0[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_cc1[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_cc6[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_frequency[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_frequency_effective[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+ __u64 core_power[HSMP_F1A_M50_M5F_MAX_CORES_PER_CCD];
+};
+
+/*
+ * Metrics table for Family 0x1A, Models 0x50 to 0x5F, table version 0x00700000.
+ *
+ * The layout mirrors the SMU's binary metric table verbatim: one IOD block
+ * followed by HSMP_F1A_M50_M5F_MAX_CCDS fixed-size CCD blocks. Only the first
+ * iod.num_active_ccds entries in ccd[] carry meaningful data; the remaining
+ * entries are firmware padding and must be ignored by userspace.
+ *
+ * Future processors within the same family and model may support a
+ * variable number of CCDs and cores.
+ */
+struct hsmp_metric_table_zen6 {
+ struct hsmp_metric_table_zen6_iod iod;
+ struct hsmp_metric_table_zen6_ccd ccd[HSMP_F1A_M50_M5F_MAX_CCDS];
+};
+
/* Reset to default packing */
#pragma pack()
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 3/8] platform/x86/amd/hsmp: Unify response_sz validation to an upper-bound check
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 1/8] platform/x86/amd/hsmp: Add HSMP messages for Family 1Ah, Model 50h-5Fh Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 2/8] platform/x86/amd/hsmp: Add UAPI structures for Family 1Ah Model 50h-5Fh metrics table Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 4/8] platform/x86/amd/hsmp: Source metric-table size from firmware Muralidhara M K
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Suma Hegde,
Muthusamy Ramalingam
As HSMP protocol versions evolve, existing message IDs sometimes
gain additional response words on newer firmware. validate_message()
currently enforces a strict equality (response_sz == table value)
for HSMP_SET and HSMP_GET, so userspace compiled against an earlier
descriptor table is rejected with -EINVAL when it asks for fewer
response words than the in-kernel table now declares - even though
that caller has no interest in the additional words. Only
HSMP_SET_GET already used a relaxed upper-bound check.
Replace the per-type branching with a single upper-bound check for
all message types. Userspace can now request fewer response words
than hardware provides, while requests that exceed the descriptor
table (and therefore the hardware capability) are still rejected.
Reviewed-by: Suma Hegde <suma.hegde@amd.com>
Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
drivers/platform/x86/amd/hsmp/hsmp.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 631ffc0978d1..9bad58fef304 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -182,20 +182,15 @@ static int validate_message(struct hsmp_message *msg)
return -EINVAL;
/*
- * Some older HSMP SET messages are updated to add GET in the same message.
- * In these messages, GET returns the current value and SET also returns
- * the successfully set value. To support this GET and SET in same message
- * while maintaining backward compatibility for the HSMP users,
- * hsmp_msg_desc_table[] indicates only maximum allowed response_sz.
+ * As the HSMP protocol evolves, newer platforms may define more
+ * response arguments for existing messages. Use an upper-bound
+ * check so that older userspace callers requesting fewer response
+ * words than what the current hsmp_msg_desc_table[] defines are
+ * still accepted, while rejecting requests that exceed the
+ * hardware capability.
*/
- if (hsmp_msg_desc_table[msg->msg_id].type == HSMP_SET_GET) {
- if (msg->response_sz > hsmp_msg_desc_table[msg->msg_id].response_sz)
- return -EINVAL;
- } else {
- /* only HSMP_SET or HSMP_GET messages go through this strict check */
- if (msg->response_sz != hsmp_msg_desc_table[msg->msg_id].response_sz)
- return -EINVAL;
- }
+ if (msg->response_sz > hsmp_msg_desc_table[msg->msg_id].response_sz)
+ return -EINVAL;
return 0;
}
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 4/8] platform/x86/amd/hsmp: Source metric-table size from firmware
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
` (2 preceding siblings ...)
2026-06-11 5:29 ` [PATCH v5 3/8] platform/x86/amd/hsmp: Unify response_sz validation to an upper-bound check Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 5/8] platform/x86/amd/hsmp: Add IOCTL_GET_TELEMETRY_DATA for metric table reads Muralidhara M K
` (3 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Muthusamy Ramalingam
The driver hard-codes the metric-table region size to
sizeof(struct hsmp_metric_table). That is correct for HSMP protocol
version 6 but mis-sizes the ioremap of the SMU DRAM region on newer
platforms: Family 1Ah Model 50h-5Fh exposes a ~13 KB
hsmp_metric_table_zen6, and the table is expected to keep growing
on future firmware. The same hard-coded value also forces
hsmp_metric_tbl_read() to reject any read that follows the actual
firmware layout.
Pick up the table size from firmware instead. SMU on Family 1Ah
Model 50h and later populates HSMP_GET_METRIC_TABLE_DRAM_ADDR's
args[2] with the DRAM region size in bytes; older firmware leaves
it 0. Bump the descriptor's response_sz to 3 so the field is read,
store the value in hsmp_pdev.hsmp_table_size at probe time, and
fall back to sizeof(struct hsmp_metric_table) when firmware reports
0. Use hsmp_table_size both for the devm_ioremap() of the region
and as the upper bound for hsmp_metric_tbl_read().
Behaviour on existing protocol-version-6 hardware is unchanged:
firmware returns 0, the fallback yields the same value as the
previous hard-coded one, and both the ioremap and the size check
produce the same result as before.
The ioctl interface added later in this series uses the same
hsmp_table_size to validate the userspace request and to copy out
the full firmware-reported region.
Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
arch/x86/include/uapi/asm/amd_hsmp.h | 5 +++--
drivers/platform/x86/amd/hsmp/hsmp.c | 11 ++++++++---
drivers/platform/x86/amd/hsmp/hsmp.h | 1 +
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index 34e25d9048d5..77bea5ab7ade 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -367,11 +367,12 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[]
{0, 0, HSMP_GET},
/*
- * HSMP_GET_METRIC_TABLE_DRAM_ADDR, num_args = 0, response_sz = 2
+ * HSMP_GET_METRIC_TABLE_DRAM_ADDR, num_args = 0, response_sz = 3
* output: args[0] = lower 32 bits of the address
* output: args[1] = upper 32 bits of the address
+ * output: args[2] = DRAM region size in bytes
*/
- {0, 2, HSMP_GET},
+ {0, 3, HSMP_GET},
/*
* HSMP_SET_XGMI_PSTATE_RANGE, num_args = 1, response_sz = 0
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 9bad58fef304..cf9392f99298 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -356,8 +356,7 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
return -ENOMEM;
}
- /* Do not support lseek(), also don't allow more than the size of metric table */
- if (size != sizeof(struct hsmp_metric_table)) {
+ if (size != hsmp_pdev.hsmp_table_size) {
dev_err(sock->dev, "Wrong buffer size\n");
return -EINVAL;
}
@@ -398,8 +397,14 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
dev_err(sock->dev, "Invalid DRAM address for metric table\n");
return -ENOMEM;
}
+ /* SMU returns table size from Family 1Ah Model 50h and forward */
+ if (msg.args[2])
+ hsmp_pdev.hsmp_table_size = msg.args[2];
+ else
+ hsmp_pdev.hsmp_table_size = sizeof(struct hsmp_metric_table);
+
sock->metric_tbl_addr = devm_ioremap(sock->dev, dram_addr,
- sizeof(struct hsmp_metric_table));
+ hsmp_pdev.hsmp_table_size);
if (!sock->metric_tbl_addr) {
dev_err(sock->dev, "Failed to ioremap metric table addr\n");
return -ENOMEM;
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index b153527e0a0d..e7f051475728 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -55,6 +55,7 @@ struct hsmp_plat_device {
u32 proto_ver;
u16 num_sockets;
bool is_probed;
+ size_t hsmp_table_size;
};
int hsmp_cache_proto_ver(u16 sock_ind);
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 5/8] platform/x86/amd/hsmp: Add IOCTL_GET_TELEMETRY_DATA for metric table reads
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
` (3 preceding siblings ...)
2026-06-11 5:29 ` [PATCH v5 4/8] platform/x86/amd/hsmp: Source metric-table size from firmware Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1 Muralidhara M K
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Muthusamy Ramalingam
The metric table needs to be delivered to userspace as a single
atomic snapshot, but the current sysfs metrics_bin path is a file
read: userspace can read it in chunks and observe a torn snapshot
if an SMU refresh happens between read() calls. The same path is
also bounded by PAGE_SIZE, so the ~13 KB hsmp_metric_table_zen6
used on Family 1Ah Model 50h-5Fh cannot be returned at all,
regardless of how userspace reads it. Rather than extend sysfs to
lift both restrictions, expose the metric table through the
existing HSMP character device using a new ioctl that always copies
the full table in one shot.
Add struct hsmp_telemetry_data and HSMP_IOCTL_GET_TELEMETRY_DATA
to the UAPI header. Under the surrounding #pragma pack(4), placing
the __u64 user pointer first gives a tight 16-byte layout that is
identical for 32- and 64-bit callers, and the trailing __u16
reserved field is rejected with -EINVAL if non-zero so future
kernels can repurpose it without breaking already-deployed
userspace. Userspace sizes its buffer using the matching UAPI
metric-table struct for the running platform; sizes that disagree
with the firmware-reported table size are rejected so a short copy
can never silently truncate the snapshot.
Dispatch hsmp_ioctl() on the ioctl command: the existing message
handler is factored out as hsmp_ioctl_msg() for HSMP_IOCTL_CMD, and
HSMP_IOCTL_GET_TELEMETRY_DATA goes to a new
hsmp_ioctl_get_telemetry() helper.
The user-controlled socket index in HSMP_IOCTL_GET_TELEMETRY_DATA is
clamped with array_index_nospec() before indexing hsmp_pdev.sock[],
mitigating Spectre v1 (CVE-2017-5753).
Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
Documentation/arch/x86/amd_hsmp.rst | 30 +++++++-
arch/x86/include/uapi/asm/amd_hsmp.h | 43 +++++++++++
drivers/platform/x86/amd/hsmp/hsmp.c | 104 ++++++++++++++++++++++++++-
3 files changed, 175 insertions(+), 2 deletions(-)
diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
index 8bb411f0d70d..301c0091f5bf 100644
--- a/Documentation/arch/x86/amd_hsmp.rst
+++ b/Documentation/arch/x86/amd_hsmp.rst
@@ -68,6 +68,14 @@ under per socket sysfs directory created at
Note: lseek() is not supported as entire metrics table is read.
+The sysfs metrics_bin path supports only HSMP protocol version 6 and,
+because it is a file read, can return a torn snapshot if userspace
+reads in pieces. Protocol version 7 metric tables
+(``struct hsmp_metric_table_zen6``, ~13 KB) also exceed PAGE_SIZE, so
+a read returns ``-EOPNOTSUPP``. For atomic reads on any protocol
+version, use the ``HSMP_IOCTL_GET_TELEMETRY_DATA`` ioctl on /dev/hsmp
+(see below).
+
Metrics table definitions will be documented as part of Public PPR.
The same is defined in the amd_hsmp.h header.
@@ -167,7 +175,7 @@ Next thing, open the device file, as follows::
exit(1);
}
-The following IOCTL is defined:
+The following IOCTLs are defined:
``ioctl(file, HSMP_IOCTL_CMD, struct hsmp_message *msg)``
The argument is a pointer to a::
@@ -180,6 +188,26 @@ The following IOCTL is defined:
__u16 sock_ind; /* socket number */
};
+``ioctl(file, HSMP_IOCTL_GET_TELEMETRY_DATA, struct hsmp_telemetry_data *req)``
+ Atomically fetch the firmware metric (telemetry) table for a socket.
+ The ioctl copies the table in one shot, so unlike the metrics_bin
+ sysfs path it cannot return a torn snapshot and is not bounded by
+ PAGE_SIZE. Required for HSMP protocol version 7+ (e.g. Family 1Ah
+ Model 50h-5Fh, ~13 KB ``struct hsmp_metric_table_zen6``). Argument::
+
+ struct hsmp_telemetry_data {
+ __u64 buf; /* User pointer to destination buffer */
+ __u32 size; /* Size of @buf, must equal firmware-reported table size */
+ __u16 sock_ind; /* Socket index */
+ __u16 reserved; /* Reserved, must be zero */
+ };
+
+ Userspace picks the layout matching the running protocol version
+ (``struct hsmp_metric_table`` for v6, ``struct hsmp_metric_table_zen6``
+ for v7, queried via the ``protocol_version`` sysfs attribute) and
+ uses ``sizeof()`` of that struct for ``size``. The kernel rejects
+ a mismatched ``size`` or non-zero ``reserved`` with ``-EINVAL``.
+
The ioctl would return a non-zero on failure; you can read errno to see
what happened. The transaction returns 0 on success.
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index 77bea5ab7ade..9b246fd724ef 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -702,6 +702,40 @@ struct hsmp_metric_table_zen6 {
struct hsmp_metric_table_zen6_ccd ccd[HSMP_F1A_M50_M5F_MAX_CCDS];
};
+/**
+ * struct hsmp_telemetry_data - Request descriptor for HSMP telemetry IOCTL
+ * @buf: Input. Userspace pointer (encoded as __u64 to keep the layout
+ * stable between 32-bit and 64-bit callers) to the destination
+ * buffer that receives the metric table.
+ * @size: Input. Size in bytes of the buffer pointed to by @buf. Must
+ * match the firmware-reported metric table size for the running
+ * HSMP protocol version (see below); any other value results in
+ * -EINVAL. The kernel does not write this field back.
+ * @sock_ind: Input. Socket index from which the metric table is read.
+ * @reserved: Reserved for future use. Callers should set this to zero;
+ * future kernels may begin interpreting the field, so passing
+ * a non-zero value today is not forwards compatible.
+ *
+ * Placing @buf first lets all fields fall on their natural alignment under
+ * the surrounding #pragma pack(4), so the struct is a tight 16 bytes with
+ * the same wire layout on 32-bit and 64-bit userspace.
+ *
+ * The exact metric table layout depends on the HSMP protocol version reported
+ * by the firmware:
+ * - Protocol version 6 -> struct hsmp_metric_table
+ * - Protocol version 7 -> struct hsmp_metric_table_zen6
+ *
+ * Userspace queries the protocol version (e.g. via the protocol_version sysfs
+ * attribute) and uses sizeof() on the matching UAPI structure for both @size
+ * and the allocation backing @buf.
+ */
+struct hsmp_telemetry_data {
+ __u64 buf;
+ __u32 size;
+ __u16 sock_ind;
+ __u16 reserved;
+};
+
/* Reset to default packing */
#pragma pack()
@@ -709,4 +743,13 @@ struct hsmp_metric_table_zen6 {
#define HSMP_BASE_IOCTL_NR 0xF8
#define HSMP_IOCTL_CMD _IOWR(HSMP_BASE_IOCTL_NR, 0, struct hsmp_message)
+/*
+ * Fetch the firmware metric (telemetry) table for a given socket via the
+ * HSMP character device. This avoids the PAGE_SIZE limitation of the
+ * sysfs binary attribute path for tables larger than one page (such as the
+ * ~13 KB hsmp_metric_table_zen6 used on Family 1Ah Model 50h-5Fh).
+ */
+#define HSMP_IOCTL_GET_TELEMETRY_DATA \
+ _IOWR(HSMP_BASE_IOCTL_NR, 1, struct hsmp_telemetry_data)
+
#endif /*_ASM_X86_AMD_HSMP_H_*/
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index cf9392f99298..36ff83744684 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -12,8 +12,11 @@
#include <linux/acpi.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/nospec.h>
#include <linux/semaphore.h>
+#include <linux/slab.h>
#include <linux/sysfs.h>
+#include <linux/uaccess.h>
#include "hsmp.h"
@@ -287,7 +290,7 @@ static bool is_get_msg(struct hsmp_message *msg)
return false;
}
-long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+static long hsmp_ioctl_msg(struct file *fp, unsigned long arg)
{
int __user *arguser = (int __user *)arg;
struct hsmp_message msg = { 0 };
@@ -343,6 +346,105 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
return 0;
}
+/*
+ * Fetch the firmware metric (telemetry) table for the requested socket and
+ * copy it to the userspace buffer described by the request.
+ *
+ * The metric table size is variable across HSMP protocol versions and on
+ * Family 1Ah Model 50h-5Fh exceeds PAGE_SIZE. Userspace must therefore
+ * supply a buffer equal to the firmware-reported size in bytes.
+ */
+static long hsmp_ioctl_get_telemetry(struct file *fp, unsigned long arg)
+{
+ void __user *arguser = (void __user *)arg;
+ struct hsmp_telemetry_data req;
+ struct hsmp_socket *sock;
+ void __user *user_buf;
+ size_t tbl_size;
+ unsigned int sock_ind;
+ void *kbuf;
+ int ret;
+
+ /* Telemetry data is read-only; require read access on the fd. */
+ if (!(fp->f_mode & FMODE_READ))
+ return -EPERM;
+
+ if (copy_from_user(&req, arguser, sizeof(req)))
+ return -EFAULT;
+
+ /*
+ * Reserved fields must be zero so future kernels can safely
+ * repurpose them without breaking already-deployed userspace.
+ */
+ if (req.reserved)
+ return -EINVAL;
+
+ if (!hsmp_pdev.sock || req.sock_ind >= hsmp_pdev.num_sockets)
+ return -ENODEV;
+
+ tbl_size = hsmp_pdev.hsmp_table_size;
+ if (!tbl_size)
+ return -ENODEV;
+
+ /*
+ * Userspace must size its buffer using the appropriate UAPI metric
+ * table struct for the running protocol version. Reject mismatched
+ * sizes so we never silently truncate or short-write.
+ */
+ if (req.size != tbl_size)
+ return -EINVAL;
+
+ /*
+ * Sanitize the user-controlled socket index against speculative
+ * execution. The bounds check above retires the out-of-range case
+ * with -ENODEV, but a mispredicted branch can still let the CPU
+ * speculatively use sock_ind as an index into hsmp_pdev.sock[] and
+ * pull arbitrary kernel memory into the cache (Spectre v1, CVE-2017-
+ * 5753). array_index_nospec() turns the bounds check into a
+ * data-flow clamp so the speculative load is in-range too.
+ */
+ sock_ind = array_index_nospec(req.sock_ind, hsmp_pdev.num_sockets);
+ sock = &hsmp_pdev.sock[sock_ind];
+ if (!sock->metric_tbl_addr)
+ return -ENODEV;
+
+ user_buf = u64_to_user_ptr(req.buf);
+
+ /*
+ * The bounce buffer is overwritten in full by memcpy_fromio() inside
+ * hsmp_metric_tbl_read(); use kvmalloc() to avoid the zeroing cost of
+ * kvzalloc() on the ~13 KB allocation done on every ioctl call.
+ */
+ kbuf = kvmalloc(tbl_size, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ ret = hsmp_metric_tbl_read(sock, kbuf, tbl_size);
+ if (ret < 0)
+ goto out;
+
+ if (copy_to_user(user_buf, kbuf, tbl_size))
+ ret = -EFAULT;
+ else
+ ret = 0;
+
+out:
+ kvfree(kbuf);
+ return ret;
+}
+
+long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case HSMP_IOCTL_CMD:
+ return hsmp_ioctl_msg(fp, arg);
+ case HSMP_IOCTL_GET_TELEMETRY_DATA:
+ return hsmp_ioctl_get_telemetry(fp, arg);
+ default:
+ return -ENOTTY;
+ }
+}
+
ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
{
struct hsmp_message msg = { 0 };
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
` (4 preceding siblings ...)
2026-06-11 5:29 ` [PATCH v5 5/8] platform/x86/amd/hsmp: Add IOCTL_GET_TELEMETRY_DATA for metric table reads Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 12:09 ` Ilpo Järvinen
2026-06-11 5:29 ` [PATCH v5 7/8] platform/x86/amd/hsmp: Enable HSMP_PROTO_VER7 metric tables on the ACPI driver via the IOCTL Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex) Muralidhara M K
7 siblings, 1 reply; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Muthusamy Ramalingam
Although validate_message() checks msg_id, a mispredicted branch can
still allow speculative indexing into hsmp_msg_desc_table[]. Clamp
msg.msg_id with array_index_nospec() at entry to hsmp_ioctl_msg() so
downstream dereferences (including via is_get_msg() and
hsmp_send_message()) see a bounded index.
Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
drivers/platform/x86/amd/hsmp/hsmp.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 36ff83744684..a9dca97568b8 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -306,6 +306,19 @@ static long hsmp_ioctl_msg(struct file *fp, unsigned long arg)
if (msg.msg_id < HSMP_TEST || msg.msg_id >= HSMP_MSG_ID_MAX)
return -ENOMSG;
+ /*
+ * Sanitize the user-controlled msg_id against speculative
+ * execution. The bounds check above retires the out-of-range
+ * case with -ENOMSG, but a mispredicted branch can still let the
+ * CPU speculatively use msg_id as an index into
+ * hsmp_msg_desc_table[] (here and in validate_message() /
+ * is_get_msg() called downstream via hsmp_send_message()), and
+ * pull arbitrary kernel memory into the cache (Spectre v1,
+ * CVE-2017-5753). Clamp once into msg.msg_id so every downstream
+ * dereference sees the sanitized value.
+ */
+ msg.msg_id = array_index_nospec(msg.msg_id, HSMP_MSG_ID_MAX);
+
switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
case FMODE_WRITE:
/*
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1
2026-06-11 5:29 ` [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1 Muralidhara M K
@ 2026-06-11 12:09 ` Ilpo Järvinen
2026-06-11 16:46 ` M K, Muralidhara
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2026-06-11 12:09 UTC (permalink / raw)
To: Muralidhara M K; +Cc: platform-driver-x86, LKML, Muthusamy Ramalingam
On Thu, 11 Jun 2026, Muralidhara M K wrote:
> Although validate_message() checks msg_id, a mispredicted branch can
> still allow speculative indexing into hsmp_msg_desc_table[]. Clamp
> msg.msg_id with array_index_nospec() at entry to hsmp_ioctl_msg() so
> downstream dereferences (including via is_get_msg() and
> hsmp_send_message()) see a bounded index.
>
> Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
> drivers/platform/x86/amd/hsmp/hsmp.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 36ff83744684..a9dca97568b8 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -306,6 +306,19 @@ static long hsmp_ioctl_msg(struct file *fp, unsigned long arg)
> if (msg.msg_id < HSMP_TEST || msg.msg_id >= HSMP_MSG_ID_MAX)
> return -ENOMSG;
>
> + /*
> + * Sanitize the user-controlled msg_id against speculative
> + * execution. The bounds check above retires the out-of-range
> + * case with -ENOMSG, but a mispredicted branch can still let the
> + * CPU speculatively use msg_id as an index into
> + * hsmp_msg_desc_table[] (here and in validate_message() /
> + * is_get_msg() called downstream via hsmp_send_message()), and
> + * pull arbitrary kernel memory into the cache (Spectre v1,
> + * CVE-2017-5753). Clamp once into msg.msg_id so every downstream
> + * dereference sees the sanitized value.
> + */
> + msg.msg_id = array_index_nospec(msg.msg_id, HSMP_MSG_ID_MAX);
> +
> switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
> case FMODE_WRITE:
> /*
Sashiko mentions there's similar gadget for sock_ind in
hsmp_send_message().
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1
2026-06-11 12:09 ` Ilpo Järvinen
@ 2026-06-11 16:46 ` M K, Muralidhara
0 siblings, 0 replies; 13+ messages in thread
From: M K, Muralidhara @ 2026-06-11 16:46 UTC (permalink / raw)
To: Ilpo Järvinen, Muralidhara M K
Cc: platform-driver-x86, LKML, Muthusamy Ramalingam
On 6/11/2026 5:39 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 11 Jun 2026, Muralidhara M K wrote:
>
>> Although validate_message() checks msg_id, a mispredicted branch can
>> still allow speculative indexing into hsmp_msg_desc_table[]. Clamp
>> msg.msg_id with array_index_nospec() at entry to hsmp_ioctl_msg() so
>> downstream dereferences (including via is_get_msg() and
>> hsmp_send_message()) see a bounded index.
>>
>> Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
>> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
>> ---
>> drivers/platform/x86/amd/hsmp/hsmp.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
>> index 36ff83744684..a9dca97568b8 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -306,6 +306,19 @@ static long hsmp_ioctl_msg(struct file *fp, unsigned long arg)
>> if (msg.msg_id < HSMP_TEST || msg.msg_id >= HSMP_MSG_ID_MAX)
>> return -ENOMSG;
>>
>> + /*
>> + * Sanitize the user-controlled msg_id against speculative
>> + * execution. The bounds check above retires the out-of-range
>> + * case with -ENOMSG, but a mispredicted branch can still let the
>> + * CPU speculatively use msg_id as an index into
>> + * hsmp_msg_desc_table[] (here and in validate_message() /
>> + * is_get_msg() called downstream via hsmp_send_message()), and
>> + * pull arbitrary kernel memory into the cache (Spectre v1,
>> + * CVE-2017-5753). Clamp once into msg.msg_id so every downstream
>> + * dereference sees the sanitized value.
>> + */
>> + msg.msg_id = array_index_nospec(msg.msg_id, HSMP_MSG_ID_MAX);
>> +
>> switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
>> case FMODE_WRITE:
>> /*
>
> Sashiko mentions there's similar gadget for sock_ind in
> hsmp_send_message().
>
Thanks for pointing. I will update in next version.
> --
> i.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 7/8] platform/x86/amd/hsmp: Enable HSMP_PROTO_VER7 metric tables on the ACPI driver via the IOCTL
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
` (5 preceding siblings ...)
2026-06-11 5:29 ` [PATCH v5 6/8] platform/x86/amd/hsmp: Sanitize hsmp_ioctl_msg() msg_id for Spectre v1 Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 5:29 ` [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex) Muralidhara M K
7 siblings, 0 replies; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Muthusamy Ramalingam
The ACPI driver currently prepares the per-socket metric table only
on HSMP_PROTO_VER6. With protocol version 7 in use on Family 1Ah
Model 50h-5Fh, userspace cannot reach the new ~13 KB
hsmp_metric_table_zen6: hsmp_get_tbl_dram_base() is skipped,
sock->metric_tbl_addr stays NULL, and the ioctl added earlier in
this series has nothing to read.
Widen the proto_ver gate in init_acpi() from '== HSMP_PROTO_VER6'
to '>= HSMP_PROTO_VER6' so the DRAM region is mapped and
hsmp_pdev.hsmp_table_size is populated on protocol version 7 (and
any future compatible version), making the ioctl path functional.
hsmp_metric_tbl_acpi_read() now returns -EOPNOTSUPP whenever the
running protocol version is not VER6. v7 userspace gets a clear,
actionable error and a documented pointer to
HSMP_IOCTL_GET_TELEMETRY_DATA; v6 userspace sees no change.
The non-ACPI plat.c path is intentionally left untouched: it covers
Family 1Ah Model 0h-Fh hardware fixed at protocol version 6, where
the existing metrics_bin remains the supported interface.
Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
drivers/platform/x86/amd/hsmp/acpi.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index 97ed71593bdf..8dc6b4a8bd27 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -238,13 +238,32 @@ static ssize_t hsmp_metric_tbl_acpi_read(struct file *filp, struct kobject *kobj
struct device *dev = container_of(kobj, struct device, kobj);
struct hsmp_socket *sock = dev_get_drvdata(dev);
+ /*
+ * metrics_bin is a sysfs binary attribute and is capped at PAGE_SIZE.
+ * It can therefore only carry the protocol version 6 metric table
+ * (struct hsmp_metric_table). Larger tables (HSMP_PROTO_VER7+, e.g.
+ * struct hsmp_metric_table_zen6 at ~13 KB used on Family 1Ah Model
+ * 50h-5Fh) do not fit; userspace on those systems must read the
+ * snapshot through HSMP_IOCTL_GET_TELEMETRY_DATA on /dev/hsmp.
+ * Surface the unsupported case here as -EOPNOTSUPP rather than
+ * silently truncating the snapshot.
+ */
+ if (hsmp_pdev->proto_ver != HSMP_PROTO_VER6)
+ return -EOPNOTSUPP;
+
return hsmp_metric_tbl_read(sock, buf, count);
}
static umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
const struct bin_attribute *battr, int id)
{
- if (hsmp_pdev->proto_ver == HSMP_PROTO_VER6)
+ /*
+ * Keep metrics_bin visible for HSMP_PROTO_VER7+ as well, so that
+ * userspace which expects the file to exist gets a clear
+ * -EOPNOTSUPP from the read handler instead of -ENOENT, and is
+ * pointed at HSMP_IOCTL_GET_TELEMETRY_DATA as the supported path.
+ */
+ if (hsmp_pdev->proto_ver >= HSMP_PROTO_VER6)
return battr->attr.mode;
return 0;
@@ -491,7 +510,7 @@ static int init_acpi(struct device *dev)
return ret;
}
- if (hsmp_pdev->proto_ver == HSMP_PROTO_VER6) {
+ if (hsmp_pdev->proto_ver >= HSMP_PROTO_VER6) {
ret = hsmp_get_tbl_dram_base(sock_ind);
if (ret)
dev_info(dev, "Failed to init metric table\n");
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex)
2026-06-11 5:29 [PATCH v5 0/8] platform/x86/amd/hsmp: Family 1Ah Model 50h-5Fh HSMP and metrics Muralidhara M K
` (6 preceding siblings ...)
2026-06-11 5:29 ` [PATCH v5 7/8] platform/x86/amd/hsmp: Enable HSMP_PROTO_VER7 metric tables on the ACPI driver via the IOCTL Muralidhara M K
@ 2026-06-11 5:29 ` Muralidhara M K
2026-06-11 13:18 ` Ilpo Järvinen
7 siblings, 1 reply; 13+ messages in thread
From: Muralidhara M K @ 2026-06-11 5:29 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: platform-driver-x86, linux-kernel, Muralidhara M K, Muthusamy Ramalingam
hsmp_metric_tbl_read() refreshes the SMU-side metric table and then
memcpy_fromio()'s the result. Without serialization, two parallel
readers can interleave the refresh and the copy and the caller
observes a torn (mixed old/new) snapshot. Add a per-socket
metric_tbl_lock so the refresh-and-copy sequence is atomic from
userspace's point of view.
Use scoped guard(mutex) so the lock is released on every return
path without hand-written goto chains, and initialize the mutex
with devm_mutex_init() so no explicit mutex_destroy() cleanup is
required.
Initialize the mutex before devm_ioremap() so the invariant
"sock->metric_tbl_addr != NULL implies metric_tbl_lock is usable"
holds on every error exit. Both callers of hsmp_get_tbl_dram_base()
(init_acpi() and init_platform_device()) intentionally only log a
failure and continue probing, so initializing the mutex after a
successful ioremap would leave sock->metric_tbl_addr populated with
an uninitialized lock, and the next hsmp_metric_tbl_read() would
take guard(mutex)() on garbage memory. With the order swapped, a
devm_mutex_init() failure returns early before metric_tbl_addr is
ever set, and the existing NULL check in hsmp_metric_tbl_read()
keeps rejecting the read with -ENOMEM as before.
Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
drivers/platform/x86/amd/hsmp/hsmp.c | 19 +++++++++++++++++++
drivers/platform/x86/amd/hsmp/hsmp.h | 3 +++
2 files changed, 22 insertions(+)
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index a9dca97568b8..46e8dc7cfb60 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -479,6 +479,7 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
msg.msg_id = HSMP_GET_METRIC_TABLE;
msg.sock_ind = sock->sock_ind;
+ guard(mutex)(&sock->metric_tbl_lock);
ret = hsmp_send_message(&msg);
if (ret)
return ret;
@@ -495,6 +496,24 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
phys_addr_t dram_addr;
int ret;
+ /*
+ * Initialize the per-socket lock before anything that can set
+ * sock->metric_tbl_addr to a non-NULL value. hsmp_metric_tbl_read()
+ * gates on sock->metric_tbl_addr being non-NULL and then takes
+ * metric_tbl_lock unconditionally; both callers of this function
+ * (init_acpi() and init_platform_device()) intentionally only log
+ * a failure here and continue probing, so an init order that left
+ * metric_tbl_addr populated while devm_mutex_init() failed would
+ * leave the read path locking an uninitialized mutex. Doing the
+ * mutex init first preserves the invariant "metric_tbl_addr !=
+ * NULL implies the lock is usable" on every error exit.
+ */
+ ret = devm_mutex_init(sock->dev, &sock->metric_tbl_lock);
+ if (ret) {
+ dev_err(sock->dev, "Failed to initialize metric table lock\n");
+ return ret;
+ }
+
msg.sock_ind = sock_ind;
msg.response_sz = hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
msg.msg_id = HSMP_GET_METRIC_TABLE_DRAM_ADDR;
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index e7f051475728..f7b1cbf19932 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -15,6 +15,7 @@
#include <linux/hwmon.h>
#include <linux/kconfig.h>
#include <linux/miscdevice.h>
+#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/semaphore.h>
#include <linux/sysfs.h>
@@ -41,6 +42,8 @@ struct hsmp_socket {
struct bin_attribute hsmp_attr;
struct hsmp_mbaddr_info mbinfo;
void __iomem *metric_tbl_addr;
+ /* Serializes concurrent metric table refreshes from the sysfs path */
+ struct mutex metric_tbl_lock;
void __iomem *virt_base_addr;
struct semaphore hsmp_sem;
char name[HSMP_ATTR_GRP_NAME_SIZE];
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex)
2026-06-11 5:29 ` [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex) Muralidhara M K
@ 2026-06-11 13:18 ` Ilpo Järvinen
2026-06-11 16:51 ` M K, Muralidhara
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2026-06-11 13:18 UTC (permalink / raw)
To: Muralidhara M K; +Cc: platform-driver-x86, LKML, Muthusamy Ramalingam
On Thu, 11 Jun 2026, Muralidhara M K wrote:
> hsmp_metric_tbl_read() refreshes the SMU-side metric table and then
> memcpy_fromio()'s the result. Without serialization, two parallel
> readers can interleave the refresh and the copy and the caller
> observes a torn (mixed old/new) snapshot. Add a per-socket
> metric_tbl_lock so the refresh-and-copy sequence is atomic from
> userspace's point of view.
>
> Use scoped guard(mutex) so the lock is released on every return
> path without hand-written goto chains, and initialize the mutex
> with devm_mutex_init() so no explicit mutex_destroy() cleanup is
> required.
>
> Initialize the mutex before devm_ioremap() so the invariant
> "sock->metric_tbl_addr != NULL implies metric_tbl_lock is usable"
> holds on every error exit. Both callers of hsmp_get_tbl_dram_base()
> (init_acpi() and init_platform_device()) intentionally only log a
> failure and continue probing, so initializing the mutex after a
> successful ioremap would leave sock->metric_tbl_addr populated with
> an uninitialized lock, and the next hsmp_metric_tbl_read() would
> take guard(mutex)() on garbage memory. With the order swapped, a
> devm_mutex_init() failure returns early before metric_tbl_addr is
> ever set, and the existing NULL check in hsmp_metric_tbl_read()
> keeps rejecting the read with -ENOMEM as before.
>
> Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
> drivers/platform/x86/amd/hsmp/hsmp.c | 19 +++++++++++++++++++
> drivers/platform/x86/amd/hsmp/hsmp.h | 3 +++
> 2 files changed, 22 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index a9dca97568b8..46e8dc7cfb60 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -479,6 +479,7 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
> msg.msg_id = HSMP_GET_METRIC_TABLE;
> msg.sock_ind = sock->sock_ind;
>
> + guard(mutex)(&sock->metric_tbl_lock);
> ret = hsmp_send_message(&msg);
> if (ret)
> return ret;
> @@ -495,6 +496,24 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
> phys_addr_t dram_addr;
> int ret;
>
> + /*
> + * Initialize the per-socket lock before anything that can set
> + * sock->metric_tbl_addr to a non-NULL value. hsmp_metric_tbl_read()
> + * gates on sock->metric_tbl_addr being non-NULL and then takes
> + * metric_tbl_lock unconditionally; both callers of this function
> + * (init_acpi() and init_platform_device()) intentionally only log
> + * a failure here and continue probing, so an init order that left
> + * metric_tbl_addr populated while devm_mutex_init() failed would
> + * leave the read path locking an uninitialized mutex. Doing the
> + * mutex init first preserves the invariant "metric_tbl_addr !=
> + * NULL implies the lock is usable" on every error exit.
> + */
> + ret = devm_mutex_init(sock->dev, &sock->metric_tbl_lock);
> + if (ret) {
> + dev_err(sock->dev, "Failed to initialize metric table lock\n");
> + return ret;
> + }
Sashiko flags a concurrency problem here.
This fundamentally stems from earlier design decisions:
1) hsmp_acpi_probe() is not really doing any concurrency control for
.is_probed access. I somehow seem to recall I did brought this up earlier
with somebody else working with this driver earlier but apparently there
still are not locks or other concurrency control in the probe.
I don't remember anymore what happened with it back then.
(The problem #1 is not exactly mentioned by sashiko but it's there,
AFAICT, nothing guarantees only one probe sees !hsmp_pdev->is_probed and
assigns to ->sock.)
2) ->sock teardown being bound to which ever socket allocated ->sock.
Leading to use-after-free in devm_ teardown for any remove that runs after
it.
I think the early teardown of the misc device was the only thing that
initially prevented use-after-frees. As it kind of worked, I never voiced
my concerns about how fragile the teardown was. Looking through the
history now, it seems things got broken after adding hwmon code which does
use devm and calls hsmp_send_message(). As a result, removing this driver
is currently broken. This patch adds to the problem.
I don't think is_probed is good solution here but the release of ->sock
should be properly reference counted and that might be reusable for the
alloc side.
> msg.sock_ind = sock_ind;
> msg.response_sz = hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
> msg.msg_id = HSMP_GET_METRIC_TABLE_DRAM_ADDR;
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index e7f051475728..f7b1cbf19932 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -15,6 +15,7 @@
> #include <linux/hwmon.h>
> #include <linux/kconfig.h>
> #include <linux/miscdevice.h>
> +#include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/semaphore.h>
> #include <linux/sysfs.h>
> @@ -41,6 +42,8 @@ struct hsmp_socket {
> struct bin_attribute hsmp_attr;
> struct hsmp_mbaddr_info mbinfo;
> void __iomem *metric_tbl_addr;
> + /* Serializes concurrent metric table refreshes from the sysfs path */
> + struct mutex metric_tbl_lock;
> void __iomem *virt_base_addr;
> struct semaphore hsmp_sem;
> char name[HSMP_ATTR_GRP_NAME_SIZE];
>
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v5 8/8] platform/x86/amd/hsmp: Make metric table read locking use guard(mutex)
2026-06-11 13:18 ` Ilpo Järvinen
@ 2026-06-11 16:51 ` M K, Muralidhara
0 siblings, 0 replies; 13+ messages in thread
From: M K, Muralidhara @ 2026-06-11 16:51 UTC (permalink / raw)
To: Ilpo Järvinen, Muralidhara M K
Cc: platform-driver-x86, LKML, Muthusamy Ramalingam
On 6/11/2026 6:48 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 11 Jun 2026, Muralidhara M K wrote:
>
>> hsmp_metric_tbl_read() refreshes the SMU-side metric table and then
>> memcpy_fromio()'s the result. Without serialization, two parallel
>> readers can interleave the refresh and the copy and the caller
>> observes a torn (mixed old/new) snapshot. Add a per-socket
>> metric_tbl_lock so the refresh-and-copy sequence is atomic from
>> userspace's point of view.
>>
>> Use scoped guard(mutex) so the lock is released on every return
>> path without hand-written goto chains, and initialize the mutex
>> with devm_mutex_init() so no explicit mutex_destroy() cleanup is
>> required.
>>
>> Initialize the mutex before devm_ioremap() so the invariant
>> "sock->metric_tbl_addr != NULL implies metric_tbl_lock is usable"
>> holds on every error exit. Both callers of hsmp_get_tbl_dram_base()
>> (init_acpi() and init_platform_device()) intentionally only log a
>> failure and continue probing, so initializing the mutex after a
>> successful ioremap would leave sock->metric_tbl_addr populated with
>> an uninitialized lock, and the next hsmp_metric_tbl_read() would
>> take guard(mutex)() on garbage memory. With the order swapped, a
>> devm_mutex_init() failure returns early before metric_tbl_addr is
>> ever set, and the existing NULL check in hsmp_metric_tbl_read()
>> keeps rejecting the read with -ENOMEM as before.
>>
>> Reviewed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
>> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
>> ---
>> drivers/platform/x86/amd/hsmp/hsmp.c | 19 +++++++++++++++++++
>> drivers/platform/x86/amd/hsmp/hsmp.h | 3 +++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
>> index a9dca97568b8..46e8dc7cfb60 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -479,6 +479,7 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
>> msg.msg_id = HSMP_GET_METRIC_TABLE;
>> msg.sock_ind = sock->sock_ind;
>>
>> + guard(mutex)(&sock->metric_tbl_lock);
>> ret = hsmp_send_message(&msg);
>> if (ret)
>> return ret;
>> @@ -495,6 +496,24 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
>> phys_addr_t dram_addr;
>> int ret;
>>
>> + /*
>> + * Initialize the per-socket lock before anything that can set
>> + * sock->metric_tbl_addr to a non-NULL value. hsmp_metric_tbl_read()
>> + * gates on sock->metric_tbl_addr being non-NULL and then takes
>> + * metric_tbl_lock unconditionally; both callers of this function
>> + * (init_acpi() and init_platform_device()) intentionally only log
>> + * a failure here and continue probing, so an init order that left
>> + * metric_tbl_addr populated while devm_mutex_init() failed would
>> + * leave the read path locking an uninitialized mutex. Doing the
>> + * mutex init first preserves the invariant "metric_tbl_addr !=
>> + * NULL implies the lock is usable" on every error exit.
>> + */
>> + ret = devm_mutex_init(sock->dev, &sock->metric_tbl_lock);
>> + if (ret) {
>> + dev_err(sock->dev, "Failed to initialize metric table lock\n");
>> + return ret;
>> + }
>
> Sashiko flags a concurrency problem here.
>
> This fundamentally stems from earlier design decisions:
>
> 1) hsmp_acpi_probe() is not really doing any concurrency control for
> .is_probed access. I somehow seem to recall I did brought this up earlier
> with somebody else working with this driver earlier but apparently there
> still are not locks or other concurrency control in the probe.
> I don't remember anymore what happened with it back then.
>
> (The problem #1 is not exactly mentioned by sashiko but it's there,
> AFAICT, nothing guarantees only one probe sees !hsmp_pdev->is_probed and
> assigns to ->sock.)
>
> 2) ->sock teardown being bound to which ever socket allocated ->sock.
> Leading to use-after-free in devm_ teardown for any remove that runs after
> it.
>
Understood. you are pointing out Unsynchronized is_probed / sock setup.
In hsmp_acpi_probe(), if (!hsmp_pdev->is_probed) guarded num_sockets,
devm_kcalloc(..., sock), and later hsmp_misc_register() with no lock.
Several AMDI0097 devices can probe in parallel, so two paths can both
see !is_probed, or interleave alloc vs misc_register() in a racy way.
> I think the early teardown of the misc device was the only thing that
> initially prevented use-after-frees. As it kind of worked, I never voiced
> my concerns about how fragile the teardown was. Looking through the
> history now, it seems things got broken after adding hwmon code which does
> use devm and calls hsmp_send_message(). As a result, removing this driver
> is currently broken. This patch adds to the problem.
>
>
> I don't think is_probed is good solution here but the release of ->sock
> should be properly reference counted and that might be reusable for the
> alloc side.
>
Understood. is_probed is a weak substitute for “who owns sock and when
may it go away. will explore and try to add reference counted way.
>> msg.sock_ind = sock_ind;
>> msg.response_sz = hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
>> msg.msg_id = HSMP_GET_METRIC_TABLE_DRAM_ADDR;
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
>> index e7f051475728..f7b1cbf19932 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
>> @@ -15,6 +15,7 @@
>> #include <linux/hwmon.h>
>> #include <linux/kconfig.h>
>> #include <linux/miscdevice.h>
>> +#include <linux/mutex.h>
>> #include <linux/pci.h>
>> #include <linux/semaphore.h>
>> #include <linux/sysfs.h>
>> @@ -41,6 +42,8 @@ struct hsmp_socket {
>> struct bin_attribute hsmp_attr;
>> struct hsmp_mbaddr_info mbinfo;
>> void __iomem *metric_tbl_addr;
>> + /* Serializes concurrent metric table refreshes from the sysfs path */
>> + struct mutex metric_tbl_lock;
>> void __iomem *virt_base_addr;
>> struct semaphore hsmp_sem;
>> char name[HSMP_ATTR_GRP_NAME_SIZE];
>>
>
> --
> i.
>
^ permalink raw reply [flat|nested] 13+ messages in thread