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 <[email protected]>
---
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 = {
+};
+
+static char ne_cpus[PAGE_SIZE];
+static struct kparam_string ne_cpus_arg = {
+ .maxlen = sizeof(ne_cpus),
+ .string = ne_cpus,
+};
+
+module_param_cb(ne_cpus, &ne_cpu_pool_ops, &ne_cpus_arg, 0644);
+MODULE_PARM_DESC(ne_cpus, "<cpu-list> - CPU pool used for Nitro Enclaves");
+
+/* 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 long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case NE_GET_API_VERSION:
+ return NE_API_VERSION;
+
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
+static const struct file_operations ne_fops = {
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = ne_ioctl,
+};
+
+struct miscdevice ne_misc_dev = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "nitro_enclaves",
+ .fops = &ne_fops,
+ .mode = 0660,
+};
+
+static int __init ne_init(void)
+{
+ struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
+ PCI_DEVICE_ID_NE, NULL);
+ int rc = -EINVAL;
+
+ if (!pdev)
+ return -ENODEV;
+
+ if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
+ return -ENOMEM;
+
+ mutex_init(&ne_cpu_pool.mutex);
+
+ rc = pci_register_driver(&ne_pci_driver);
+ if (rc < 0) {
+ dev_err(&pdev->dev,
+ "Error in pci register driver [rc=%d]\n", rc);
+
+ goto free_cpumask;
+ }
+
+ return 0;
+
+free_cpumask:
+ free_cpumask_var(ne_cpu_pool.avail);
+
+ return rc;
+}
+
+static void __exit ne_exit(void)
+{
+ pci_unregister_driver(&ne_pci_driver);
+
+ free_cpumask_var(ne_cpu_pool.avail);
+}
+
+/* TODO: Handle actions such as reboot, kexec. */
+
+module_init(ne_init);
+module_exit(ne_exit);
+
+MODULE_AUTHOR("Amazon.com, Inc. or its affiliates");
+MODULE_DESCRIPTION("Nitro Enclaves Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c b/drivers/virt/nitro_enclaves/ne_pci_dev.c
index 9a137862cade..c781cd0a50bf 100644
--- a/drivers/virt/nitro_enclaves/ne_pci_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c
@@ -557,6 +557,13 @@ static int ne_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
goto teardown_msix;
}
+ rc = misc_register(&ne_misc_dev);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Error in misc dev register [rc=%d]\n", rc);
+
+ goto disable_ne_pci_dev;
+ }
+
atomic_set(&ne_pci_dev->cmd_reply_avail, 0);
init_waitqueue_head(&ne_pci_dev->cmd_reply_wait_q);
INIT_LIST_HEAD(&ne_pci_dev->enclaves_list);
@@ -566,6 +573,8 @@ static int ne_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return 0;
+disable_ne_pci_dev:
+ ne_pci_dev_disable(pdev);
teardown_msix:
ne_teardown_msix(pdev);
iounmap_pci_bar:
@@ -588,6 +597,8 @@ static void ne_pci_remove(struct pci_dev *pdev)
if (!ne_pci_dev || !ne_pci_dev->iomem_base)
return;
+ misc_deregister(&ne_misc_dev);
+
ne_pci_dev_disable(pdev);
ne_teardown_msix(pdev);
--
2.20.1 (Apple Git-117)
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.
On 29/06/2020 19:20, Greg KH wrote:
>
> On Mon, Jun 22, 2020 at 11:03:18PM +0300, Andra Paraschiv wrote:
>> +static int __init ne_init(void)
>> +{
>> + struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
>> + PCI_DEVICE_ID_NE, NULL);
>> + int rc = -EINVAL;
>> +
>> + if (!pdev)
>> + return -ENODEV;
> Ick, that's a _very_ old-school way of binding to a pci device. Please
> just be a "real" pci driver and your probe function will be called if
> your hardware is present (or when it shows up.) To do it this way
> prevents your driver from being auto-loaded for when your hardware is
> seen in the system, as well as lots of other things.
This check is mainly here in the case any codebase is added before the
pci driver register call below.
And if we log any error with dev_err() instead of pr_err() before the
driver register.
That check was only for logging purposes, if done with dev_err(). I
removed the check in v5.
>> +
>> + if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
>> + return -ENOMEM;
>> +
>> + mutex_init(&ne_cpu_pool.mutex);
>> +
>> + rc = pci_register_driver(&ne_pci_driver);
> Nice, you did it right here, but why the above crazy test?
>
>> + if (rc < 0) {
>> + dev_err(&pdev->dev,
>> + "Error in pci register driver [rc=%d]\n", rc);
>> +
>> + goto free_cpumask;
>> + }
>> +
>> + return 0;
> You leaked a reference on that pci device, didn't you? Not good :(
Yes, the pci get device call needs its pair - pci_dev_put(). I added it
here and for the other occurrences where it was missing.
Thanks for review.
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.
On Mon, Jun 22, 2020 at 11:03:18PM +0300, Andra Paraschiv wrote:
> +static int __init ne_init(void)
> +{
> + struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
> + PCI_DEVICE_ID_NE, NULL);
> + int rc = -EINVAL;
> +
> + if (!pdev)
> + return -ENODEV;
Ick, that's a _very_ old-school way of binding to a pci device. Please
just be a "real" pci driver and your probe function will be called if
your hardware is present (or when it shows up.) To do it this way
prevents your driver from being auto-loaded for when your hardware is
seen in the system, as well as lots of other things.
> +
> + if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
> + return -ENOMEM;
> +
> + mutex_init(&ne_cpu_pool.mutex);
> +
> + rc = pci_register_driver(&ne_pci_driver);
Nice, you did it right here, but why the above crazy test?
> + if (rc < 0) {
> + dev_err(&pdev->dev,
> + "Error in pci register driver [rc=%d]\n", rc);
> +
> + goto free_cpumask;
> + }
> +
> + return 0;
You leaked a reference on that pci device, didn't you? Not good :(
thanks,
greg k-h
On Mon, Jun 29, 2020 at 08:45:25PM +0300, Paraschiv, Andra-Irina wrote:
>
>
> On 29/06/2020 19:20, Greg KH wrote:
> >
> > On Mon, Jun 22, 2020 at 11:03:18PM +0300, Andra Paraschiv wrote:
> > > +static int __init ne_init(void)
> > > +{
> > > + struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
> > > + PCI_DEVICE_ID_NE, NULL);
> > > + int rc = -EINVAL;
> > > +
> > > + if (!pdev)
> > > + return -ENODEV;
> > Ick, that's a _very_ old-school way of binding to a pci device. Please
> > just be a "real" pci driver and your probe function will be called if
> > your hardware is present (or when it shows up.) To do it this way
> > prevents your driver from being auto-loaded for when your hardware is
> > seen in the system, as well as lots of other things.
>
> This check is mainly here in the case any codebase is added before the pci
> driver register call below.
What do you mean by "codebase"? You control this driver, just do all of
the logic in the probe() function, no need to do this in the module init
call.
> And if we log any error with dev_err() instead of pr_err() before the driver
> register.
Don't do that.
> That check was only for logging purposes, if done with dev_err(). I removed
> the check in v5.
Again, don't do it :)
>
> > > +
> > > + if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
> > > + return -ENOMEM;
> > > +
> > > + mutex_init(&ne_cpu_pool.mutex);
> > > +
> > > + rc = pci_register_driver(&ne_pci_driver);
> > Nice, you did it right here, but why the above crazy test?
> >
> > > + if (rc < 0) {
> > > + dev_err(&pdev->dev,
> > > + "Error in pci register driver [rc=%d]\n", rc);
> > > +
> > > + goto free_cpumask;
> > > + }
> > > +
> > > + return 0;
> > You leaked a reference on that pci device, didn't you? Not good :(
>
> Yes, the pci get device call needs its pair - pci_dev_put(). I added it here
> and for the other occurrences where it was missing.
Again, just don't do it and then you don't have to worry about any of
this.
thanks,
greg k-h
On 30/06/2020 11:05, Greg KH wrote:
>
> On Mon, Jun 29, 2020 at 08:45:25PM +0300, Paraschiv, Andra-Irina wrote:
>>
>> On 29/06/2020 19:20, Greg KH wrote:
>>> On Mon, Jun 22, 2020 at 11:03:18PM +0300, Andra Paraschiv wrote:
>>>> +static int __init ne_init(void)
>>>> +{
>>>> + struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
>>>> + PCI_DEVICE_ID_NE, NULL);
>>>> + int rc = -EINVAL;
>>>> +
>>>> + if (!pdev)
>>>> + return -ENODEV;
>>> Ick, that's a _very_ old-school way of binding to a pci device. Please
>>> just be a "real" pci driver and your probe function will be called if
>>> your hardware is present (or when it shows up.) To do it this way
>>> prevents your driver from being auto-loaded for when your hardware is
>>> seen in the system, as well as lots of other things.
>> This check is mainly here in the case any codebase is added before the pci
>> driver register call below.
> What do you mean by "codebase"? You control this driver, just do all of
> the logic in the probe() function, no need to do this in the module init
> call.
>
>> And if we log any error with dev_err() instead of pr_err() before the driver
>> register.
> Don't do that.
>
>> That check was only for logging purposes, if done with dev_err(). I removed
>> the check in v5.
> Again, don't do it :)
>
>>>> +
>>>> + if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
>>>> + return -ENOMEM;
>>>> +
>>>> + mutex_init(&ne_cpu_pool.mutex);
>>>> +
>>>> + rc = pci_register_driver(&ne_pci_driver);
>>> Nice, you did it right here, but why the above crazy test?
>>>
>>>> + if (rc < 0) {
>>>> + dev_err(&pdev->dev,
>>>> + "Error in pci register driver [rc=%d]\n", rc);
>>>> +
>>>> + goto free_cpumask;
>>>> + }
>>>> +
>>>> + return 0;
>>> You leaked a reference on that pci device, didn't you? Not good :(
>> Yes, the pci get device call needs its pair - pci_dev_put(). I added it here
>> and for the other occurrences where it was missing.
> Again, just don't do it and then you don't have to worry about any of
> this.
Yup, already started this morning to check & update where we can go
without this call to get a PCI device reference, as a follow-up to what
we discussed yesterday.
Thanks,
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.
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 <[email protected]>
> ---
> 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.
> +};
> +
> +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.
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
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 <[email protected]>
>> ---
>> 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.
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.
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 <[email protected]>
>>> ---
>>> 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
On 06/07/2020 11:01, Alexander Graf wrote:
>
>
> 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 <[email protected]>
>>>> ---
>>>> 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?
We can set it like this, I changed to 512 and updated the comment as well.
Thanks,
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.