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

Dave Jiang
2024-07-22T22:52:47Z

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