Laurent Dufour [off-list ref] writes:

Le 30/07/2022 à 02:04, Nathan Lynch a écrit :

quoted

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

quoted

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