Re: [PATCH] platform/x86/amd/hsmp: Add support for HSMP protocol version 7 messages

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

On Wed, 13 Nov 2024, Suma Hegde wrote:

> Following new HSMP messages are available on family 0x1A, model 0x00-0x1F
> platforms with protocol version 7. Add support for them in the driver.
> - SetXgmiPstateRange(26h)
> - CpuRailIsoFreqPolicy(27h)
> - DfcEnable(28h)
> - GetRaplUnit(30h)
> - GetRaplCoreCounter(31h)
> - GetRaplPackageCounter(32h)
> 
> Also update HSMP message PwrEfficiencyModeSelection-21h. This message is
> updated to include GET option in recent firmware.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> This patch is rebased on 
> https://lore.kernel.org/platform-driver-x86/20241112120450.2407125-1-suma.hegde@amd.com/T/#t
> 
>  arch/x86/include/uapi/asm/amd_hsmp.h | 64 +++++++++++++++++++++++++++-
>  drivers/platform/x86/amd/hsmp/hsmp.c | 46 ++++++++++++++++----
>  2 files changed, 99 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
> index e5d182c7373c..c83a5a7103b5 100644
> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
> @@ -50,6 +50,12 @@ enum hsmp_message_ids {
>  	HSMP_GET_METRIC_TABLE_VER,	/* 23h Get metrics table version */
>  	HSMP_GET_METRIC_TABLE,		/* 24h Get metrics table */
>  	HSMP_GET_METRIC_TABLE_DRAM_ADDR,/* 25h Get metrics table dram address */
> +	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_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_MSG_ID_MAX,
>  };
>  
> @@ -65,6 +71,7 @@ enum hsmp_msg_type {
>  	HSMP_RSVD = -1,
>  	HSMP_SET  = 0,
>  	HSMP_GET  = 1,
> +	HSMP_SET_GET	= 2,
>  };
>  
>  enum hsmp_proto_versions {
> @@ -72,7 +79,8 @@ enum hsmp_proto_versions {
>  	HSMP_PROTO_VER3,
>  	HSMP_PROTO_VER4,
>  	HSMP_PROTO_VER5,
> -	HSMP_PROTO_VER6
> +	HSMP_PROTO_VER6,
> +	HSMP_PROTO_VER7
>  };
>  
>  struct hsmp_msg_desc {
> @@ -299,7 +307,7 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>  	 * HSMP_SET_POWER_MODE, num_args = 1, response_sz = 0
>  	 * input: args[0] = power efficiency mode[2:0]
>  	 */
> -	{1, 0, HSMP_SET},
> +	{1, 1, HSMP_SET_GET},
>  
>  	/*
>  	 * HSMP_SET_PSTATE_MAX_MIN, num_args = 1, response_sz = 0
> @@ -324,6 +332,58 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>  	 * output: args[1] = upper 32 bits of the address
>  	 */
>  	{0, 2, HSMP_GET},
> +
> +	/*
> +	 * HSMP_SET_XGMI_PSTATE_RANGE, num_args = 1, response_sz = 0
> +	 * input: args[0] = min xGMI p-state[15:8] + max xGMI p-state[7:0]
> +	 */
> +	{1, 0, HSMP_SET},
> +
> +	/*
> +	 * HSMP_CPU_RAIL_ISO_FREQ_POLICY, num_args = 1, response_sz = 1
> +	 * input: args[0] = set/get policy[31] +
> +	 * disable/enable independent control[0]
> +	 * output: args[0] = current policy[0]
> +	 */
> +	{1, 1, HSMP_SET_GET},
> +
> +	/*
> +	 * HSMP_DFC_ENABLE_CTRL, num_args = 1, response_sz = 1
> +	 * input: args[0] = set/get policy[31] + enable/disable DFC[0]
> +	 * output: args[0] = current policy[0]
> +	 */
> +	{1, 1, HSMP_SET_GET},
> +
> +	/* RESERVED(0x29-0x2f) */
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +	{0, 0, HSMP_RSVD},
> +
> +	/*
> +	 * HSMP_GET_RAPL_UNITS, response_sz = 1
> +	 * output: args[0] = tu value[19:16] + esu value[12:8]
> +	 */
> +	{0, 1, HSMP_GET},
> +
> +	/*
> +	 * HSMP_GET_RAPL_CORE_COUNTER, num_args = 1, response_sz = 1
> +	 * input: args[0] = apic id[15:0]
> +	 * output: args[0] = lower 32 bits of energy
> +	 * output: args[1] = upper 32 bits of energy
> +	 */
> +	{1, 2, HSMP_GET},
> +
> +	/*
> +	 * HSMP_GET_RAPL_PACKAGE_COUNTER, num_args = 0, response_sz = 1
> +	 * output: args[0] = lower 32 bits of energy
> +	 * output: args[1] = upper 32 bits of energy
> +	 */
> +	{0, 2, HSMP_GET},
> +
>  };
>  
>  /* Metrics table (supported only with proto version 6) */
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index f29dd93fbf0b..3108eaec61ec 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -33,7 +33,7 @@
>  #define HSMP_WR			true
>  #define HSMP_RD			false
>  
> -#define DRIVER_VERSION		"2.3"
> +#define DRIVER_VERSION		"2.4"
>  
>  static struct hsmp_plat_device hsmp_pdev;
>  
> @@ -166,12 +166,23 @@ static int validate_message(struct hsmp_message *msg)
>  	/* msg_id is a reserved message ID */
>  	if (hsmp_msg_desc_table[msg->msg_id].type == HSMP_RSVD)
>  		return -ENOMSG;
> -
> -	/* num_args and response_sz against the HSMP spec */
> -	if (msg->num_args != hsmp_msg_desc_table[msg->msg_id].num_args ||
> -	    msg->response_sz != hsmp_msg_desc_table[msg->msg_id].response_sz)
> -		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.
> +	 */
> +	if (hsmp_msg_desc_table[msg->msg_id].type == HSMP_SET_GET) {
> +		if (msg->num_args != hsmp_msg_desc_table[msg->msg_id].num_args ||
> +		    msg->response_sz > hsmp_msg_desc_table[msg->msg_id].response_sz)
> +			return -EINVAL;
> +	} else {
> +		/* only SET or GET messages go through this strict check */
> +		if (msg->num_args != hsmp_msg_desc_table[msg->msg_id].num_args ||
> +		    msg->response_sz != hsmp_msg_desc_table[msg->msg_id].response_sz)
> +			return -EINVAL;

Please do the common check outside the if.

-- 
 i.

> +	}
>  	return 0;
>  }
>  
> @@ -239,6 +250,23 @@ int hsmp_test(u16 sock_ind, u32 value)
>  }
>  EXPORT_SYMBOL_NS_GPL(hsmp_test, AMD_HSMP);
>  
> +static bool is_get_msg(struct hsmp_message *msg)
> +{
> +	if (hsmp_msg_desc_table[msg->msg_id].type == HSMP_GET)
> +		return true;
> +
> +	if (hsmp_msg_desc_table[msg->msg_id].type == HSMP_SET_GET) {
> +		/*
> +		 * When same message numbers are used for both GET and SET operation,
> +		 * bit:31 indicates whether its SET or GET operation.
> +		 */
> +		if (msg->args[0] & BIT(31))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  {
>  	int __user *arguser = (int  __user *)arg;
> @@ -261,7 +289,7 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		 * Device is opened in O_WRONLY mode
>  		 * Execute only set/configure commands
>  		 */
> -		if (hsmp_msg_desc_table[msg.msg_id].type != HSMP_SET)
> +		if (is_get_msg(&msg))
>  			return -EPERM;
>  		break;
>  	case FMODE_READ:
> @@ -269,7 +297,7 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		 * Device is opened in O_RDONLY mode
>  		 * Execute only get/monitor commands
>  		 */
> -		if (hsmp_msg_desc_table[msg.msg_id].type != HSMP_GET)
> +		if (!is_get_msg(&msg))
>  			return -EPERM;
>  		break;
>  	case FMODE_READ | FMODE_WRITE:
>