Re: [PATCH] powerpc/pseries: add lparctl driver for platform-specific functions

[Nathan Lynch <nathanl@linux.ibm.com>]

Laurent Dufour <ldufour@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.