Re: [PATCH v7 07/10] misc: amd-sbi: Add support for CPUID protocol

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Apr 2, 2025, at 07:58, Akshay Gupta wrote:
> - AMD provides custom protocol to read Processor feature
>   capabilities and configuration information through side band.
>   The information is accessed by providing CPUID Function,
>   extended function and thread ID to the protocol.
>   Undefined function returns 0.
>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
> ---
> Changes since v6:
> - Address Arnd comment
>   - Add padding to the uapi structure
> - Rebased patch, previously patch 8

This changes the UAPI again. since you change the common structure
layout.

> @@ -134,6 +279,9 @@ static long sbrmi_ioctl(struct file *fp, unsigned 
> int cmd, unsigned long arg)
>  		/* Mailbox protocol */
>  		ret = rmi_mailbox_xfer(data, &msg);
>  		break;
> +	case APML_CPUID:
> +		ret = rmi_cpuid_read(data, &msg);
> +		break;
>  	default:
>  		return -EINVAL;

As I previously commented, I would prefer to have a highl-level
interface per specific mailbox item you transfer, but I think that
is something we can debate, in particular if Greg or the x86
maintainers think it's ok, I'm not going to object.

However, having a combined ioctl command and data structure
for rmi_mailbox_xfer(), rmi_cpuid_read() and the commands
you add later seems to cause a lot of the extra complexity,
and I think this really has to be done differently, using
distinct ioctl command numbers for each of them, with an
appropriate structure to go along with it.

This does mean the existing userspace tool will be incompatible
with the upstream driver, but it can be easily updated to
support both kernel interfaces (trying the new one first,
and falling back to the old on after -ENOTTY).


>  struct apml_message {
>  	/*
>  	 * [0]...[3] mailbox 32bit input
> +	 *	     cpuid,
> +	 * [4][5] cpuid: thread
> +	 * [6] cpuid: ext function & read eax/ebx or ecx/edx
> +	 *	[7:0] -> bits [7:4] -> ext function &
> +	 *	bit [0] read eax/ebx or ecx/edx
>  	 * [7] read/write functionality
>  	 */
>  	union {
> +		__u64 cpu_msr_in;
>  		__u32 mb_in[2];
>  		__u8 reg_in[8];
>  	} data_in;
>  	/*
>  	 * 8 bit data for reg read,
>  	 * 32 bit data in case of mailbox,
> +	 * up to 64 bit in case of cpuid
>  	 */
>  	union {
> +		__u64 cpu_msr_out;
>  		__u32 mb_out[2];
>  		__u8 reg_out[8];
>  	} data_out;
>  	/* message ids:
>  	 * Mailbox Messages:	0x0 ... 0x999
> +	 * APML_CPUID:		0x1000
>  	 */
>  	__u32 cmd;
>  	/*
> +	 * Status code is returned in case of CPUID access
>  	 * Error code is returned in case of soft mailbox
>  	 */
>  	__u32 fw_ret_code;
> +	/* Add padding to align the structure */
> +	__u32 padding[2];
>  };

So this structure would become something like

struct apml_mailbox_message {
      __u32 cmd;
      __u32 mb_data[2];
      __u32 fw_ret_code;
};
#define SBRMI_IOCTL_MBOX_CMD	_IOWR(SB_BASE_IOCTL_NR, 0, struct apml_mbox_message)

struct apml_cpuid_message {
       __u64 cpu_msr;
       __u32 fw_ret_code;
       __u32 pad;
};
#define SBRMI_IOCTL_MBOX_CMD	_IOWR(SB_BASE_IOCTL_NR, 1, struct apml_cpuid_message)

       Arnd