Laurent Dufour <lduf...@linux.ibm.com> writes:
> Le 30/07/2022 à 02:04, Nathan Lynch a écrit :
>> +static long lparctl_get_sysparm(struct lparctl_get_system_parameter __user
>> *argp)
>> +{
>> + struct lparctl_get_system_parameter *gsp;
>> + long ret;
>> + int fwrc;
>> +
>> + /*
>> + * Special case to allow user space to probe the command.
>> + */
>> + if (argp == NULL)
>> + return 0;
>> +
>> + gsp = memdup_user(argp, sizeof(*gsp));
>> + if (IS_ERR(gsp)) {
>> + ret = PTR_ERR(gsp);
>> + goto err_return;
>> + }
>> +
>> + ret = -EINVAL;
>> + if (gsp->rtas_status != 0)
>> + goto err_free;
>> +
>> + do {
>> + static_assert(sizeof(gsp->data) <= sizeof(rtas_data_buf));
>> +
>> + spin_lock(&rtas_data_buf_lock);
>> + memset(rtas_data_buf, 0, sizeof(rtas_data_buf));
>> + memcpy(rtas_data_buf, gsp->data, sizeof(gsp->data));
>> + fwrc = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
>> + NULL, gsp->token, __pa(rtas_data_buf),
>> + sizeof(gsp->data));
>> + if (fwrc == 0)
>> + memcpy(gsp->data, rtas_data_buf, sizeof(gsp->data));
>
> May be the amount of data copied out to the user space could be
> gsp->length. This would prevent copying 4K bytes all the time.
>
> In a more general way, the size of the RTAS buffer is quite big, and I'm
> wondering if all the data need to be copied back and forth to the kernel.
>
> Unless there are a high frequency of calls this doesn't make sense, and
> keeping the code simple might be the best way. Otherwise limiting the bytes
> copied could help a bit.
This is not intended to be a high-bandwidth interface and I don't think
there's much of a performance concern here, so I'd rather just keep the
copy sizes involved constant.
>> +static long lparctl_set_sysparm(struct lparctl_set_system_parameter __user
>> *argp)
>> +{
>> + struct lparctl_set_system_parameter *ssp;
>> + long ret;
>> + int fwrc;
>> +
>> + /*
>> + * Special case to allow user space to probe the command.
>> + */
>> + if (argp == NULL)
>> + return 0;
>> +
>> + ssp = memdup_user(argp, sizeof(*ssp));
>
> As for the get case, would it be nice to limit the amount of bytes copied
> to the interesting "length" ?
No, the intent is to pass the buffer contents straight to RTAS without
any validation by the kernel, which would duplicate work already
performed by firmware.
