Message-ID: <a27b4997-12a5-46e7-de81-41ec2b550fc2@amazon.de>
Date:   Mon, 10 Aug 2020 07:22:06 +0200
From:   Alexander Graf <graf@...zon.de>
To:     Andra Paraschiv <andraprs@...zon.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
CC:     Anthony Liguori <aliguori@...zon.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        "David Duncan" <davdunc@...zon.com>,
        Bjoern Doebel <doebel@...zon.de>,
        "David Woodhouse" <dwmw@...zon.co.uk>,
        Frank van der Linden <fllinden@...zon.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Karen Noel <knoel@...hat.com>,
        "Martin Pohlack" <mpohlack@...zon.de>,
        Matt Wilson <msw@...zon.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Balbir Singh <sblbir@...zon.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stewart Smith <trawets@...zon.com>,
        Uwe Dannowski <uwed@...zon.de>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        kvm <kvm@...r.kernel.org>,
        ne-devel-upstream <ne-devel-upstream@...zon.com>
Subject: Re: [PATCH v6 07/18] nitro_enclaves: Init misc device providing the
 ioctl interface



On 05.08.20 11:10, 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@...zon.com>
> ---
> Changelog
> 
> v5 -> v6
> 
> * Remove the ioctl to query API version.
> * Update documentation to kernel-doc format.
> 
> v4 -> v5
> 
> * Update the size of the NE CPU pool string from 4096 to 512 chars.
> 
> 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 | 121 ++++++++++++++++++++++
>   drivers/virt/nitro_enclaves/ne_pci_dev.c  |  11 ++
>   2 files changed, 132 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..472850250220
> --- /dev/null
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + */
> +
> +/**
> + * DOC: 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"
> +
> +/**
> + * NE_CPUS_SIZE - Size for max 128 CPUs, for now, in a cpu-list string, comma
> + *		  separated. The NE CPU pool includes CPUs from a single NUMA
> + *		  node.
> + */
> +#define NE_CPUS_SIZE		(512)
> +
> +/**
> + * NE_EIF_LOAD_OFFSET - The offset where to copy the Enclave Image Format (EIF)
> + *			image in enclave memory.
> + */
> +#define NE_EIF_LOAD_OFFSET	(8 * 1024UL * 1024UL)
> +
> +/**
> + * NE_MIN_ENCLAVE_MEM_SIZE - The minimum memory size an enclave can be launched
> + *			     with.
> + */
> +#define NE_MIN_ENCLAVE_MEM_SIZE	(64 * 1024UL * 1024UL)
> +
> +/**
> + * NE_MIN_MEM_REGION_SIZE - The minimum size of an enclave memory region.
> + */
> +#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 = {
> +	.get	= param_get_string,
> +};
> +
> +static char ne_cpus[NE_CPUS_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);
> +/* https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html#cpu-lists */
> +MODULE_PARM_DESC(ne_cpus, "<cpu-list> - CPU pool used for Nitro Enclaves");
> +
> +/**
> + * struct ne_cpu_pool - CPU pool used for Nitro Enclaves.
> + * @avail_cores:	Available CPU cores in the pool.
> + * @avail_cores_size:	The size of the available cores array.
> + * @mutex:		Mutex for the access to the NE CPU pool.
> + * @numa_node:		NUMA node of the CPUs in the pool.
> + */
> +struct ne_cpu_pool {
> +	cpumask_var_t	*avail_cores;
> +	unsigned int	avail_cores_size;
> +	struct mutex	mutex;
> +	int		numa_node;
> +};
> +
> +static struct ne_cpu_pool ne_cpu_pool;
> +
> +static const struct file_operations ne_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= noop_llseek,
> +};
> +
> +struct miscdevice ne_misc_dev = {
> +	.minor	= MISC_DYNAMIC_MINOR,
> +	.name	= "nitro_enclaves",
> +	.fops	= &ne_fops,
> +	.mode	= 0660,
> +};
> +
> +static int __init ne_init(void)
> +{
> +	mutex_init(&ne_cpu_pool.mutex);
> +
> +	return pci_register_driver(&ne_pci_driver);
> +}
> +
> +static void __exit ne_exit(void)
> +{
> +	pci_unregister_driver(&ne_pci_driver);
> +}
> +
> +/* 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 a898fae066d9..1e434bf44c9d 100644
> --- a/drivers/virt/nitro_enclaves/ne_pci_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c
> @@ -527,6 +527,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 you set ne_misc_dev.parent to &pdev->dev, you can establish a full 
device path connection between the device node and the underlying NE PCI 
device. That means that in the ioctl path, you can also just access the 
device rather than search for it.

Alex

> +	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);
> @@ -536,6 +543,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:
> @@ -561,6 +570,8 @@ static void ne_pci_remove(struct pci_dev *pdev)
>   {
>   	struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
>   
> +	misc_deregister(&ne_misc_dev);
> +
>   	ne_pci_dev_disable(pdev);
>   
>   	ne_teardown_msix(pdev);
> 



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

Powered by blists - more mailing lists