Messages in this thread | | | Subject | Re: [PATCH v2 07/18] nitro_enclaves: Init misc device providing the ioctl interface | From | "Paraschiv, Andra-Irina" <> | Date | Mon, 25 May 2020 23:49:50 +0300 |
| |
On 22/05/2020 10:07, Greg KH wrote: > On Fri, May 22, 2020 at 09:29:35AM +0300, Andra Paraschiv wrote: >> +static char *ne_cpus; >> +module_param(ne_cpus, charp, 0644); >> +MODULE_PARM_DESC(ne_cpus, "<cpu-list> - CPU pool used for Nitro Enclaves"); > This is not the 1990's, don't use module parameters if you can help it. > Why is this needed, and where is it documented?
This is a CPU pool that can be set by the root user and that includes CPUs set aside to be used for the enclave(s) setup; these CPUs are offlined. From this CPU pool, the kernel logic chooses the CPUs that are set for the created enclave(s).
The cpu-list format is matching the same that is documented here:
https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html
I've also thought of having a sysfs entry for the setup of this enclave CPU pool.
> >> +/* CPU pool used for Nitro Enclaves. */ >> +struct ne_cpu_pool { >> + /* Available CPUs in the pool. */ >> + cpumask_var_t avail; >> + struct mutex mutex; >> +}; >> + >> +static struct ne_cpu_pool ne_cpu_pool; >> + >> +static int ne_open(struct inode *node, struct file *file) >> +{ >> + return 0; >> +} > If open does nothing, just don't even provide it.
I removed this and other file ops occurrences that do nothing for now.
> >> + >> +static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >> +{ >> + switch (cmd) { >> + >> + default: >> + return -ENOTTY; >> + } >> + >> + return 0; >> +} > Same for ioctl.
This logic is completed in the next patch in the series.
> >> + >> +static int ne_release(struct inode *inode, struct file *file) >> +{ >> + return 0; >> +} > Same for release.
Done, I removed it for now.
> >> + >> +static const struct file_operations ne_fops = { >> + .owner = THIS_MODULE, >> + .llseek = noop_llseek, >> + .unlocked_ioctl = ne_ioctl, >> + .open = ne_open, >> + .release = ne_release, >> +}; >> + >> +struct miscdevice ne_miscdevice = { >> + .minor = MISC_DYNAMIC_MINOR, >> + .name = NE_DEV_NAME, >> + .fops = &ne_fops, >> + .mode = 0660, >> +}; >> + >> +static int __init ne_init(void) >> +{ >> + unsigned int cpu = 0; >> + unsigned int cpu_sibling = 0; >> + int rc = -EINVAL; >> + >> + memset(&ne_cpu_pool, 0, sizeof(ne_cpu_pool)); > Why did you just set a structure to 0 that was already initialized by > the system to 0? Are you sure about this?
True, this is not needed. Removed the memset() call.
> >> + >> + if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL)) >> + return -ENOMEM; >> + >> + mutex_init(&ne_cpu_pool.mutex); >> + >> + rc = cpulist_parse(ne_cpus, ne_cpu_pool.avail); >> + if (rc < 0) { >> + pr_err_ratelimited(NE "Error in cpulist parse [rc=%d]\n", rc); > Again, drop all ratelimited stuff please.
Updated to pr_err().
Thank you.
Andra
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
| |