Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Feb 28, 2025 at 05:35:53PM -0800, Shannon Nelson wrote:

> +	/* reject unsupported and/or out of scope commands */
> +	op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
> +	for (i = 0; i < ep_info->operations->num_entries; i++) {
> +		if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
> +			if (scope < op_entry[i].scope)
> +				return -EPERM;
> +			return 0;

Oh I see, you are doing the scope like CXL with a "command intent's
report". That is a good idea.

> +	if (rpc->in.len > 0) {
> +		in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
> +		if (!in_payload) {
> +			dev_err(dev, "Failed to allocate in_payload\n");
> +			out = ERR_PTR(-ENOMEM);
> +			goto done;
> +		}
> +
> +		if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
> +				   rpc->in.len)) {
> +			dev_err(dev, "Failed to copy in_payload from user\n");
> +			out = ERR_PTR(-EFAULT);
> +			goto done;
> +		}

This wasn't really the intention to have a payload pointer inside the
existing payload pointer, is it the same issue as CXL where you wanted
a header?

I see your FW API also can't take a scatterlist, so it makes sense it
needs to be linearized like this.

I don't think it makes a difference to have the indirection or not, it
would be a bunch of stuff to keep the header seperate from the payload
and then still also end up with memcpy in the driver, so leave it.

> +#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT	0
> +#define PDS_FWCTL_RPC_OPCODE_CMD_MASK	GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
> +#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT	16
> +#define PDS_FWCTL_RPC_OPCODE_VER_MASK	GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
> +
> +#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op)	\
> +	(((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)

Isn't that FIELD_GET?

> +struct fwctl_rpc_pds {
> +	struct {
> +		__u32 op;
> +		__u32 ep;
> +		__u32 rsvd;
> +		__u32 len;
> +		__u64 payload;
> +	} in;
> +	struct {
> +		__u32 retval;
> +		__u32 rsvd[2];
> +		__u32 len;
> +		__u64 payload;
> +	} out;
> +};

Use __aligned_u64 in the uapi headers

Jason