On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote:

quoted

+static int plpks_get_variable(const char *key, uint64_t key_len,
+                             u8 *data, uint64_t *data_size)
+{
+       struct plpks_var var = {0};
+       u16 ucs2_namelen;
+       u8 *ucs2_name;
+       int rc = 0;
+
+       ucs2_namelen = get_ucs2name(key, &ucs2_name);
+       if (!ucs2_namelen)
+               return -ENOMEM;
+
+       var.name = ucs2_name;
+       var.namelen = ucs2_namelen;
+       var.os = PLPKS_VAR_LINUX;
+       rc = plpks_read_os_var(&var);
+
+       if (rc)
+               goto err;
+
+       *data_size = var.datalen + sizeof(var.policy);
+
+       // We can be called with data = NULL to just get the object
size.
+       if (data) {
+               memcpy(data, &var.policy, sizeof(var.policy));
+               memcpy(data + sizeof(var.policy), var.data,
var.datalen);
+       }

There's a lot of allocation and copying going on. The secvar-sysfs.c
data_read() has kzalloc'ed data, but only after already doing the
hcall
to get the size. Then plpks_read_os_var() does an allocation for the
hcall and then another allocation of the exact data size. Then
data_read()
does another copy into the sysfs supplied buffer.

So to read a single variable we do the hcall twice, and allocate/copy
the content of the variable 4 times?

 - Hypervisor into "output" in plpks_read_var().
 - "output" into "var->data" in plpks_read_var().
 - "var.data" into "data" in plpks_get_variable().
 - "data" into "buf" in data_read().

As long as maxobjsize is < PAGE_SIZE I think we could do the hcall
directly into "buf". Maybe we want to avoid writing into "buf"
directly
in case the hcall fails or something, but the other 3 copies seem
unnecessary.

In the general case, I don't like passing buffer pointers straight from
parameters into hcalls, since the address has to be in the linear map,
and that's a detail I'd rather hide from callers. But otherwise, yes I
think we can probably shift to having the caller allocate the buffers.

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited