Re: [PATCH 07/11] platform/x86/amd/hsmp: Add per-platform bitmap to validate HSMP messages

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Wed, 21 Oct 2026, Ilpo Järvinen wrote:

> On Thu, 8 Oct 2026, Suma Hegde wrote:
> 
> > From: Suma Hegde <Suma.Hegde@amd.com>
> > 
> > The HSMP driver previously allowed sending any message defined in
> > amd_hsmp.h, including ones not supported on the running platform.
> > 
> > Add read and write bitmaps per socket to store which HSMP messages
> > are enabled on the platform. Add validation in validate_message()
> > to reject unsupported messages before they reach the SMU.
> > 
> > Key changes:
> > - Introduce enabled_read_msg_ids and enabled_write_msg_ids bitmaps in
> >   struct hsmp_socket.
> > - Add is_read_msg_enabled() and is_write_msg_enabled() helpers in
> >   hsmp.h.
> > - Use is_get_msg() to distinguish read and write HSMP messages.
> > - Mark HSMP_TEST, HSMP_GET_PROTO_VER, and HSMP_GET_SMU_VER as always
> >   enabled during bitmap initialization, as these cannot be masked by
> >   BIOS.
> > - Skip bitmap init, populate, and validation on client platforms; the
> >   client message set has no per-platform bitmaps yet.
> > 
> > Co-developed-by: Dhruv Gupta <dhruv.gupta@amd.com>
> > Signed-off-by: Dhruv Gupta <dhruv.gupta@amd.com>
> > Co-developed-by: Muthusamy Ramalingam <Muthusamy.Ramalingam@amd.com>
> > Signed-off-by: Muthusamy Ramalingam <Muthusamy.Ramalingam@amd.com>
> > Signed-off-by: Suma Hegde <Suma.Hegde@amd.com>
> > ---
> >  drivers/platform/x86/amd/hsmp/acpi.c | 13 ++++
> >  drivers/platform/x86/amd/hsmp/hsmp.c | 94 ++++++++++++++++++++++++++++
> >  drivers/platform/x86/amd/hsmp/hsmp.h | 27 ++++++++
> >  drivers/platform/x86/amd/hsmp/plat.c | 13 ++++
> >  4 files changed, 147 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> > index b6f54e8b91663..0d3b324fe2244 100644
> > --- a/drivers/platform/x86/amd/hsmp/acpi.c
> > +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> > @@ -566,6 +566,11 @@ static int init_acpi(struct device *dev)
> >  
> >  	sock = &hsmp_pdev->sock[sock_ind];
> >  
> > +	if (!is_client_platform()) {
> > +		init_read_enabled_msg_bitmap(sock);
> > +		init_write_enabled_msg_bitmap(sock);
> > +	}
> > +
> >  	/* Test the hsmp interface */
> >  	ret = hsmp_test(sock_ind, 0xDEADBEEF);
> >  	if (ret) {
> > @@ -581,6 +586,14 @@ static int init_acpi(struct device *dev)
> >  		return ret;
> >  	}
> >  
> > +	if (!is_client_platform()) {
> > +		ret = hsmp_populate_msg_bitmaps(sock_ind);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to get enabled HSMP messages on this platform\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	if ((is_client_platform() &&
> >  	     sock->proto_ver >= RYZEN_MASTER_PROTO_VER1) ||
> >  	    (!is_client_platform() &&
> > diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> > index 8dc1fc75bd9ba..89528632c9532 100644
> > --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> > +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include <linux/acpi.h>
> >  #include <linux/array_size.h>
> > +#include <linux/bitmap.h>
> >  #include <linux/build_bug.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/compiler.h>
> > @@ -285,6 +286,8 @@ static int validate_message(struct hsmp_message *msg)
> >  {
> >  	const struct hsmp_plat_desc *plat_desc = hsmp_desc();
> >  	const struct hsmp_msg_desc *desc;
> > +	struct hsmp_socket *sock;
> > +	unsigned int sock_ind;
> >  
> >  	if (!plat_desc->msg_desc) {
> >  		/*
> > @@ -294,6 +297,9 @@ static int validate_message(struct hsmp_message *msg)
> >  		 * supports in each direction and leave msg_id correctness,
> >  		 * such as an unsupported or out-of-range value, to the SMU
> >  		 * status code __hsmp_send_message() already translates.
> > +		 *
> > +		 * Client platforms also have no per-platform enabled-message
> > +		 * bitmaps yet, so skip that check here.
> 
> I'm unsure how to read this. This validation seems to be on driver side so 
> why cannot it be validated for client?
> 
> >  		 */
> >  		if (msg->num_args > HSMP_CLIENT_MAX_MSG_LEN ||
> >  		    msg->response_sz > HSMP_CLIENT_MAX_MSG_LEN)
> > @@ -307,6 +313,17 @@ static int validate_message(struct hsmp_message *msg)
> >  	if (!desc)
> >  		return -ENOMSG;
> >  
> > +	sock_ind = array_index_nospec(msg->sock_ind, hsmp_pdev.num_sockets);
> > +	sock = &hsmp_pdev.sock[sock_ind];
> > +
> > +	if (is_get_msg(desc, msg)) {
> > +		if (!is_read_msg_enabled(sock, msg->msg_id))
> > +			return -ENOMSG;
> > +	} else {
> > +		if (!is_write_msg_enabled(sock, msg->msg_id))
> > +			return -ENOMSG;
> > +	}
> > +
> >  	/*
> >  	 * num_args passed by user should match the num_args specified in
> >  	 * message description table.
> > @@ -822,6 +839,83 @@ int hsmp_cache_proto_ver(u16 sock_ind)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(hsmp_cache_proto_ver, "AMD_HSMP");
> >  
> > +/*
> > + * Predefine read and write enabled message bitmaps for use on platforms where
> > + * HSMP_GET_ENABLED_HSMP_CMDS (37h) is not available.
> > + */
> > +static const u32 read_bitmap_f19h_m00h_0fh[] = {0x001b8cde, 0x00000000, 0x00000000};
> 
> These are just a combination of BIT(msg_id), right? If yes, these should 
> be constructed from the named defines by OR'ing them together instead of 
> using those obscure literals.
> 
> > +static const u32 write_bitmap_f19h_m00h_0fh[] = {0x00047320, 0x00000000, 0x00000000};
> > +
> > +static const u32 read_bitmap_zen4_variants[] = {0x7fdb8cde, 0x00000000, 0x00000000};
> > +static const u32 write_bitmap_zen4_variants[] = {0x80047320, 0x00000007, 0x00000000};
> > +
> > +static const u32 read_bitmap_f19h_m80h_9fh[] = {0x7e0b8cde, 0x00000038, 0x00000000};
> > +static const u32 write_bitmap_f19h_m80h_9fh[] = {0x00040320, 0x00000000, 0x00000000};
> > +
> > +static const u32 read_bitmap_f1ah_m00h_1fh[] = {0x7fdb8cde, 0x00070182, 0x00000000};
> > +static const u32 write_bitmap_f1ah_m00h_1fh[] = {0x80047320, 0x000001c6, 0x00000000};
> 
> What's the general plan with these? Do we have to keep adding to them when 
> more platforms appear?

Nevermind the question, I see the answer in patch 9.

Could you please add a note about the future platforms having automatic 
way to acquire this into changelog of this patch as well.

--
 i.

> > +/*
> > + * Family 0x1A model 0x50-0x5f, family 0x1A model 0xC0-0xCF,
> > + * family 0x1A model 0x90-0x9B and family 0x1A model 0xA0-0xAB
> > + * belong to 6th Gen AMD EPYC Server family.
> > + */
> > +static const u32 read_bitmap_zen6_variants[] = {0x7fdbbcde, 0x05ff07fe, 0x00000000};
> > +static const u32 write_bitmap_zen6_variants[] = {0x00047320, 0x070007c6, 0x00000000};
> > +
> > +#define set_read_enabled_msg_bitmap(sock, src)	\
> > +	bitmap_from_arr32((sock)->enabled_read_msg_ids, (src), HSMP_MAX_MSGS)
> > +#define set_write_enabled_msg_bitmap(sock, src)	\
> > +	bitmap_from_arr32((sock)->enabled_write_msg_ids, (src), HSMP_MAX_MSGS)
> > +
> > +int hsmp_populate_msg_bitmaps(u16 sock_ind)
> > +{
> > +	struct hsmp_socket *sock = &hsmp_pdev.sock[sock_ind];
> > +
> > +	switch (boot_cpu_data.x86) {
> > +	case 0x19:
> > +		switch (boot_cpu_data.x86_model) {
> > +		case 0x00 ... 0x0F:
> > +			set_read_enabled_msg_bitmap(sock, read_bitmap_f19h_m00h_0fh);
> > +			set_write_enabled_msg_bitmap(sock, write_bitmap_f19h_m00h_0fh);
> > +			return 0;
> > +		case 0x10 ... 0x1F:
> > +		case 0xA0 ... 0xAF:
> > +			set_read_enabled_msg_bitmap(sock, read_bitmap_zen4_variants);
> > +			set_write_enabled_msg_bitmap(sock, write_bitmap_zen4_variants);
> > +			return 0;
> > +		case 0x80 ... 0x9F:
> > +			set_read_enabled_msg_bitmap(sock, read_bitmap_f19h_m80h_9fh);
> > +			set_write_enabled_msg_bitmap(sock, write_bitmap_f19h_m80h_9fh);
> > +			return 0;
> > +		}
> > +		break;
> > +	case 0x1A:
> > +		switch (boot_cpu_data.x86_model) {
> > +		case 0x00 ... 0x1F:
> > +			set_read_enabled_msg_bitmap(sock, read_bitmap_f1ah_m00h_1fh);
> > +			set_write_enabled_msg_bitmap(sock, write_bitmap_f1ah_m00h_1fh);
> > +			return 0;
> > +		case 0x50 ... 0x5F:
> > +		case 0x90 ... 0x9B:
> > +		case 0xA0 ... 0xAB:
> > +		case 0xC0 ... 0xCF:
> > +			set_read_enabled_msg_bitmap(sock, read_bitmap_zen6_variants);
> > +			set_write_enabled_msg_bitmap(sock, write_bitmap_zen6_variants);
> > +			/*
> > +			 * PC6 control (msg 0x29) is not supported from
> > +			 * protocol 0x70002 onwards on these models.
> > +			 */
> > +			if (sock->proto_ver >= 0x70002) {
> > +				clear_bit(HSMP_PC6_ENABLE, sock->enabled_read_msg_ids);
> > +				clear_bit(HSMP_PC6_ENABLE, sock->enabled_write_msg_ids);
> > +			}
> > +			return 0;
> > +		}
> > +	}
> > +	return -ENODEV;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(hsmp_populate_msg_bitmaps, "AMD_HSMP");
> > +
> >  static const struct file_operations hsmp_fops = {
> >  	.owner		= THIS_MODULE,
> >  	.unlocked_ioctl	= hsmp_ioctl,
> > diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> > index 6c850c91a51b1..e67bbdd2b9bee 100644
> > --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> > +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> > @@ -13,6 +13,8 @@
> >  #include <asm/amd/hsmp.h>
> >  
> >  #include <linux/acpi.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/bitops.h>
> >  #include <linux/compiler_types.h>
> >  #include <linux/device.h>
> >  #include <linux/hwmon.h>
> > @@ -26,6 +28,8 @@
> >  #include <linux/sysfs.h>
> >  #include <linux/types.h>
> >  
> > +#define HSMP_MAX_MSGS			96
> > +
> >  #define HSMP_METRICS_TABLE_NAME	"metrics_bin"
> >  
> >  #define HSMP_ATTR_GRP_NAME_SIZE	10
> > @@ -77,6 +81,8 @@ struct hsmp_socket {
> >  	struct device *dev;
> >  	u16 sock_ind;
> >  	u32 proto_ver;
> > +	DECLARE_BITMAP(enabled_read_msg_ids, HSMP_MAX_MSGS);
> > +	DECLARE_BITMAP(enabled_write_msg_ids, HSMP_MAX_MSGS);
> >  	int (*amd_hsmp_rdwr)(struct hsmp_socket *sock, u32 off, u32 *val, bool rw);
> >  };
> >  
> > @@ -86,7 +92,28 @@ struct hsmp_plat_device {
> >  	u16 num_sockets;
> >  };
> >  
> > +/* HSMP_TEST, HSMP_GET_PROTO_VER, HSMP_GET_SMU_VER are always enabled in
> > + * all the platforms and can't be masked by BIOS. Set those bits in the
> > + * bitmap during bitmap initialization.
> > + */
> > +#define init_read_enabled_msg_bitmap(sock)					\
> > +	do {									\
> > +		bitmap_zero((sock)->enabled_read_msg_ids, HSMP_MAX_MSGS);	\
> > +		bitmap_set((sock)->enabled_read_msg_ids, HSMP_TEST, 1);		\
> > +		bitmap_set((sock)->enabled_read_msg_ids, HSMP_GET_PROTO_VER, 1);\
> > +		bitmap_set((sock)->enabled_read_msg_ids, HSMP_GET_SMU_VER, 1);	\
> > +	} while (0)
> > +
> > +#define init_write_enabled_msg_bitmap(sock)	\
> > +	bitmap_zero((sock)->enabled_write_msg_ids, HSMP_MAX_MSGS)
> > +
> > +#define is_read_msg_enabled(sock, msg_id)	\
> > +	test_bit((msg_id), (sock)->enabled_read_msg_ids)
> > +#define is_write_msg_enabled(sock, msg_id)	\
> > +	test_bit((msg_id), (sock)->enabled_write_msg_ids)
> > +
> >  int hsmp_cache_proto_ver(u16 sock_ind);
> > +int hsmp_populate_msg_bitmaps(u16 sock_ind);
> >  int hsmp_test(u16 sock_ind, u32 value);
> >  long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg);
> >  void hsmp_misc_deregister(void);
> > diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> > index 40c98d9c66d4f..310a776125961 100644
> > --- a/drivers/platform/x86/amd/hsmp/plat.c
> > +++ b/drivers/platform/x86/amd/hsmp/plat.c
> > @@ -178,6 +178,11 @@ static int init_platform_device(struct device *dev)
> >  		sock->mbinfo.msg_arg_off	= SMN_HSMP_MSG_DATA;
> >  		sema_init(&sock->hsmp_sem, 1);
> >  
> > +		if (!is_client_platform()) {
> > +			init_read_enabled_msg_bitmap(sock);
> > +			init_write_enabled_msg_bitmap(sock);
> > +		}
> > +
> >  		/* Test the hsmp interface on each socket */
> >  		ret = hsmp_test(i, 0xDEADBEEF);
> >  		if (ret) {
> > @@ -193,6 +198,14 @@ static int init_platform_device(struct device *dev)
> >  			return ret;
> >  		}
> >  
> > +		if (!is_client_platform()) {
> > +			ret = hsmp_populate_msg_bitmaps(i);
> > +			if (ret) {
> > +				dev_err(dev, "Failed to get enabled HSMP messages on this platform\n");
> > +				return ret;
> > +			}
> > +		}
> > +
> >  		if (sock->proto_ver == HSMP_PROTO_VER6) {
> >  			ret = hsmp_get_tbl_dram_base(i);
> >  			if (ret)
> > 
> 
>