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

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 3/4/2025 11:52 AM, Jason Gunthorpe wrote:
> 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?

Hmm... I think so.  I'll poke at that.

> 
>> +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

Sure.

Thanks,
sln



> 
> Jason