Messages in this thread | | | Subject | Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic secure boot | From | Andrew Donnellan <> | Date | Tue, 10 Jan 2023 14:59:26 +1100 |
| |
On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote: > > > +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
| |