Re: [RFC PATCH 10/13] fwctl/cxl: Add support for fwctl RPC command to enable CXL feature commands

[Dave Jiang <dave.jiang@intel.com>]

On 7/22/24 8:29 AM, Jason Gunthorpe wrote:
> On Thu, Jul 18, 2024 at 02:32:28PM -0700, Dave Jiang wrote:
>> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
>> index d3a735f1fe4e..de8949a28473 100644
>> --- a/include/uapi/fwctl/cxl.h
>> +++ b/include/uapi/fwctl/cxl.h
>> @@ -22,4 +22,68 @@ struct fwctl_info_cxl {
>>  	__u32 uctx_caps;
>>  };
>>  
>> +/*
>> + * CXL spec r3.1 Table 8-101 Set Feature Input Payload
>> + */
>> +struct set_feature_input {
>> +	uuid_t uuid;
>> +	__u32 flags;
>> +	__u16 offset;
>> +	__u8 version;
>> +	__u8 reserved[9];
>> +	__u8 data[];
>> +} __packed;
> 
> This struct probably needs to be prefixed with fwcl_, but should it be
> here I wonder or should the CXL defined structs be someplace else?
>

It's the "set feature" mailbox input for user to fill out. So probably should be in uapi. 

 
>> +/**
>> + * struct cxl_send_command - Send a command to a memory device.
>> + * @id: The command to send to the memory device. This must be one of the
>> + *	commands returned by the query command.
>> + * @flags: Flags for the command (input).
>> + * @raw: Special fields for raw commands
>> + * @raw.opcode: Opcode passed to hardware when using the RAW command.
>> + * @raw.rsvd: Must be zero.
>> + * @rsvd: Must be zero.
>> + * @retval: Return value from the memory device (output).
>> + * @in: Parameters associated with input payload.
>> + * @in.size: Size of the payload to provide to the device (input).
>> + * @in.rsvd: Must be zero.
>> + * @in.payload: Pointer to memory for payload input, payload is little endian.
>> + *
>> + * Output payload is defined with 'struct fwctl_rpc' and is the hardware output
>> + */
>> +struct fwctl_cxl_command {
>> +	__u32 id;
>> +	__u32 flags;
>> +	union {
>> +		struct {
>> +			__u16 opcode;
>> +			__u16 rsvd;
>> +		} raw;
>> +		__u32 rsvd;
>> +	};
>> +
>> +	struct {
>> +		__u32 size;
>> +		__u32 rsvd;
>> +		__u64 payload;
>> +	} in;
>> +};
> 
> Why all the structs and unions?

It was copied from 'struct cxl_send_command' for the current ioctl command. Will rework that.

> 
>> +/**
>> + * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input
>> + */
>> +struct fwctl_rpc_cxl {
>> +	__u32 rpc_cmd;
>> +	__u32 payload_size;
>> +	__u32 version;
>> +	__u32 rsvd;
>> +	struct fwctl_cxl_command send_cmd;
>> +};
>> +
>> +struct fwctl_rpc_cxl_out {
>> +	__u32 retval;
>> +	__u32 rsvd;
> 
>> +	__u8 payload[];
> 
> This might be better as 
> 
>   __aligned_u64 payload[]
> 
> Which will force a 8 byte alignment of the struct which will allow the
> payload to be casted to aligned u64 reliably.

ok will change.

> 
> How do you know the length of payload?

Yes will change the rsvd field to size.

> 
> Jason