Re: [PATCH v4 07/18] nitro_enclaves: Init misc device providing the ioctl interface

[Alexander Graf]

On 06.07.20 09:49, Paraschiv, Andra-Irina wrote:
> On 06/07/2020 10:13, Alexander Graf wrote:
> > On 22.06.20 22:03, Andra Paraschiv wrote:
> > > The Nitro Enclaves driver provides an ioctl interface to the user space
> > > for enclave lifetime management e.g. enclave creation / termination and
> > > setting enclave resources such as memory and CPU.
> > >
> > > This ioctl interface is mapped to a Nitro Enclaves misc device.
> > >
> > > Signed-off-by: Andra Paraschiv <andraprs@xxxxxxxxxx>
> > > ---
> > > Changelog
> > >
> > > v3 -> v4
> > >
> > > * Use dev_err instead of custom NE log pattern.
> > > * Remove the NE CPU pool init during kernel module loading, as the CPU
> > >    pool is now setup at runtime, via a sysfs file for the kernel
> > >    parameter.
> > > * Add minimum enclave memory size definition.
> > >
> > > v2 -> v3
> > >
> > > * Remove the GPL additional wording as SPDX-License-Identifier is
> > >    already in place.
> > > * Remove the WARN_ON calls.
> > > * Remove linux/bug and linux/kvm_host includes that are not needed.
> > > * Remove "ratelimited" from the logs that are not in the ioctl call
> > >    paths.
> > > * Remove file ops that do nothing for now - open and release.
> > >
> > > v1 -> v2
> > >
> > > * Add log pattern for NE.
> > > * Update goto labels to match their purpose.
> > > * Update ne_cpu_pool data structure to include the global mutex.
> > > * Update NE misc device mode to 0660.
> > > * Check if the CPU siblings are included in the NE CPU pool, as full CPU
> > >    cores are given for the enclave(s).
> > > ---
> > >   drivers/virt/nitro_enclaves/ne_misc_dev.c | 133 ++++++++++++++++++++++
> > >   drivers/virt/nitro_enclaves/ne_pci_dev.c  |  11 ++
> > >   2 files changed, 144 insertions(+)
> > >   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev.c
> > >
> > > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c 
> > > b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > > new file mode 100644
> > > index 000000000000..628fb10c2b36
> > > --- /dev/null
> > > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > > @@ -0,0 +1,133 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights 
> > > Reserved.
> > > + */
> > > +
> > > +/**
> > > + * Enclave lifetime management driver for Nitro Enclaves (NE).
> > > + * Nitro is a hypervisor that has been developed by Amazon.
> > > + */
> > > +
> > > +#include <linux/anon_inodes.h>
> > > +#include <linux/capability.h>
> > > +#include <linux/cpu.h>
> > > +#include <linux/device.h>
> > > +#include <linux/file.h>
> > > +#include <linux/hugetlb.h>
> > > +#include <linux/list.h>
> > > +#include <linux/miscdevice.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/mman.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/nitro_enclaves.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/poll.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include "ne_misc_dev.h"
> > > +#include "ne_pci_dev.h"
> > > +
> > > +#define NE_EIF_LOAD_OFFSET (8 * 1024UL * 1024UL)
> > > +
> > > +#define NE_MIN_ENCLAVE_MEM_SIZE (64 * 1024UL * 1024UL)
> > > +
> > > +#define NE_MIN_MEM_REGION_SIZE (2 * 1024UL * 1024UL)
> > > +
> > > +/*
> > > + * TODO: Update logic to create new sysfs entries instead of using
> > > + * a kernel parameter e.g. if multiple sysfs files needed.
> > > + */
> > > +static const struct kernel_param_ops ne_cpu_pool_ops = {
> >
> > Adding an empty ops struct looks very odd. If you fill it in a later 
> > patch, please indicate so in a comment here.
>
> True, I already updated this in v5, to have the .get function here and 
> the .set one in a later patch.
>
> > > +};
> > > +
> > > +static char ne_cpus[PAGE_SIZE];
> >
> > PAGE_SIZE is a bit excessive, no? Even if you list every single CPU of 
> > a 256 CPU system you are <1024.
>
> It is a bit too much, I was thinking of it while declaring this. I can 
> update to 1024 in v5.

The largest NUMA node CPU count I'm aware of today is 64. Since we limit 
the pool to a single node, we can't go beyond that. Let's be a bit 
future proof and double that number: 128. Then we get to 401 characters 
if you pass in every single CPU as comma separated. I would seriously 
hope most people would just pass ranges though.

So how about we make it 512 for now?


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879