On 22.06.20 22:03, Andra Paraschiv wrote:
An enclave, before being started, has its resources set. One of its
resources is CPU.
The NE CPU pool is set for choosing CPUs for enclaves from it. Offline
the CPUs from the NE CPU pool during the pool setup and online them back
during the NE CPU pool teardown.
The enclave CPUs need to be full cores and from the same NUMA node. CPU
0 and its siblings have to remain available to the primary / parent VM.
Add ioctl command logic for enclave vCPU creation. Return as result a
file descriptor that is associated with the enclave vCPU.
Signed-off-by: Alexandru Vasile <lexnv@xxxxxxxxxx>
Signed-off-by: Andra Paraschiv <andraprs@xxxxxxxxxx>
---
Changelog
v3 -> v4
* Setup the NE CPU pool at runtime via a sysfs file for the kernel
parameter.
* Check enclave CPUs to be from the same NUMA node.
* Use dev_err instead of custom NE log pattern.
* Update the NE ioctl call to match the decoupling from the KVM API.
v2 -> v3
* Remove the WARN_ON calls.
* Update static calls sanity checks.
* Update kzfree() calls to kfree().
* Remove file ops that do nothing for now - open, ioctl and release.
v1 -> v2
* Add log pattern for NE.
* Update goto labels to match their purpose.
* Remove the BUG_ON calls.
* Check if enclave state is init when setting enclave vcpu.
---
drivers/virt/nitro_enclaves/ne_misc_dev.c | 491 ++++++++++++++++++++++
1 file changed, 491 insertions(+)
diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index f70496813033..d6777008f685 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -39,7 +39,11 @@
* TODO: Update logic to create new sysfs entries instead of using
* a kernel parameter e.g. if multiple sysfs files needed.
*/
+static int ne_set_kernel_param(const char *val, const struct kernel_param *kp);
+
static const struct kernel_param_ops ne_cpu_pool_ops = {
+ .get = param_get_string,
+ .set = ne_set_kernel_param,
};
static char ne_cpus[PAGE_SIZE];
@@ -60,6 +64,485 @@ struct ne_cpu_pool {
static struct ne_cpu_pool ne_cpu_pool;
+static const struct file_operations ne_enclave_vcpu_fops = {
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+};
Do we really need an fd for an object without operations? I think the
general flow to add CPUs from the pool to the VM is very sensible. But I
don't think we really need an fd as return value from that operation.
+
+/**
+ * ne_check_enclaves_created - Verify if at least one enclave has been created.
+ *
+ * @pdev: PCI device used for enclave lifetime management.
+ *
+ * @returns: true if at least one enclave is created, false otherwise.
+ */
+static bool ne_check_enclaves_created(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+
+ if (!ne_pci_dev)
+ return false;
Please pass in the ne_pci_dev into this function directly.
+
+ mutex_lock(&ne_pci_dev->enclaves_list_mutex);
+
+ if (list_empty(&ne_pci_dev->enclaves_list)) {
+ mutex_unlock(&ne_pci_dev->enclaves_list_mutex);
+
+ return false;
If you make this a return variable, you save on the unlock duplication.
+ }
+
+ mutex_unlock(&ne_pci_dev->enclaves_list_mutex);
+
+ return true;
+}
+
+/**
+ * ne_setup_cpu_pool - Set the NE CPU pool after handling sanity checks such as
+ * not sharing CPU cores with the primary / parent VM or not using CPU 0, which
+ * should remain available for the primary / parent VM. Offline the CPUs from
+ * the pool after the checks passed.
+ *
+ * @pdev: PCI device used for enclave lifetime management.
+ * @ne_cpu_list: the CPU list used for setting NE CPU pool.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_setup_cpu_pool(struct pci_dev *pdev, const char *ne_cpu_list)
+{
+ unsigned int cpu = 0;
+ unsigned int cpu_sibling = 0;
+ int numa_node = -1;
+ int rc = -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN)) {
+ dev_err(&pdev->dev, "No admin capability for CPU pool setup\n");
No need to print anything here. It only gives non-admin users a chance
to spill the kernel log. If non-admin users can write at all? Can they?
Also, isn't this at the wrong abstraction level? I would expect such a
check to happen on the file write function, not here.
+
+ return -EPERM;
+ }
+
+ if (!ne_cpu_list)
+ return 0;
+
+ if (ne_check_enclaves_created(pdev)) {
+ dev_err(&pdev->dev, "The CPU pool is used, enclaves created\n");
+
+ return -EINVAL;
+ }
+
+ mutex_lock(&ne_cpu_pool.mutex);
+
+ rc = cpulist_parse(ne_cpu_list, ne_cpu_pool.avail);
+ if (rc < 0) {
+ dev_err(&pdev->dev,
+ "Error in cpulist parse [rc=%d]\n", rc);
+
+ goto unlock_mutex;
+ }
+
+ /*
+ * Check if CPU 0 and its siblings are included in the provided CPU pool
+ * They should remain available for the primary / parent VM.
+ */
+ if (cpumask_test_cpu(0, ne_cpu_pool.avail)) {
+
+ dev_err(&pdev->dev,
+ "CPU 0 has to remain available for the primary VM\n");
Shouldn't this also change the read value of the sysfs file?
+
+ rc = -EINVAL;
+
+ goto unlock_mutex;
+ }
+
+ for_each_cpu(cpu_sibling, topology_sibling_cpumask(0)) {
+ if (cpumask_test_cpu(cpu_sibling, ne_cpu_pool.avail)) {
+ dev_err(&pdev->dev,
+ "CPU sibling %d of CPU 0 is in the CPU pool\n",
+ cpu_sibling);
Same here. I would expect the sysfs file to reflect either the previous
state or <empty> because failures mean no CPUs are donated anymore.
Can we somehow implement the get function of the param as something that
gets generated dynamically?
+
+ rc = -EINVAL;
+
+ goto unlock_mutex;
+ }
+ }
+
+ /*
+ * Check if CPU siblings are included in the provided CPU pool. The
+ * expectation is that CPU cores are made available in the CPU pool for
+ * enclaves.
+ */
+ for_each_cpu(cpu, ne_cpu_pool.avail) {
+ for_each_cpu(cpu_sibling, topology_sibling_cpumask(cpu)) {
+ if (!cpumask_test_cpu(cpu_sibling, ne_cpu_pool.avail)) {
+ dev_err(&pdev->dev,
+ "CPU %d is not in the CPU pool\n",
+ cpu_sibling);
+
+ rc = -EINVAL;
+
+ goto unlock_mutex;
+ }
+ }
+ }
+
+ /*
+ * Check if the CPUs from the NE CPU pool are from the same NUMA node.
+ */
+ for_each_cpu(cpu, ne_cpu_pool.avail) {
+ if (numa_node < 0) {
+ numa_node = cpu_to_node(cpu);
+
+ if (numa_node < 0) {
+ dev_err(&pdev->dev,
+ "Invalid NUMA node %d\n", numa_node);
+
+ rc = -EINVAL;
+
+ goto unlock_mutex;
+ }
+ } else {
+ if (numa_node != cpu_to_node(cpu)) {
+ dev_err(&pdev->dev,
+ "CPUs are from different NUMA nodes\n");
+
+ rc = -EINVAL;
+
+ goto unlock_mutex;
+ }
+ }
+ }
+
There should be a comment here that describes the why:
/*
* CPUs that are donated to enclaves should not be considered online
* by Linux anymore, as the hypervisor will degrade them to floating.
*
* We offline them here, to not degrade performance and expose correct
* topology to Linux and user space.
*/
+ for_each_cpu(cpu, ne_cpu_pool.avail) {
+ rc = remove_cpu(cpu);
+ if (rc != 0) {
+ dev_err(&pdev->dev,
+ "CPU %d is not offlined [rc=%d]\n", cpu, rc);
+
+ goto online_cpus;
+ }
+ }
+
+ mutex_unlock(&ne_cpu_pool.mutex);
+
+ return 0;
+
+online_cpus:
+ for_each_cpu(cpu, ne_cpu_pool.avail)
+ add_cpu(cpu);
+unlock_mutex:
+ mutex_unlock(&ne_cpu_pool.mutex);
+
+ return rc;
+}
+
+/**
+ * ne_teardown_cpu_pool - Online the CPUs from the NE CPU pool and cleanup the
+ * CPU pool.
+ *
+ * @pdev: PCI device used for enclave lifetime management.
+ */
+static void ne_teardown_cpu_pool(struct pci_dev *pdev)
+{
+ unsigned int cpu = 0;
+ int rc = -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN)) {
+ dev_err(&pdev->dev, "No admin capability for CPU pool setup\n");
+
+ return;
+ }
+
+ if (!ne_cpu_pool.avail)
+ return;
+
+ if (ne_check_enclaves_created(pdev)) {
+ dev_err(&pdev->dev, "The CPU pool is used, enclaves created\n");
+
+ return;
+ }
+
+ mutex_lock(&ne_cpu_pool.mutex);
+
+ for_each_cpu(cpu, ne_cpu_pool.avail) {
+ rc = add_cpu(cpu);
+ if (rc != 0)
+ dev_err(&pdev->dev,
+ "CPU %d is not onlined [rc=%d]\n", cpu, rc);
+ }
+
+ cpumask_clear(ne_cpu_pool.avail);
+
+ mutex_unlock(&ne_cpu_pool.mutex);
+}
+
+static int ne_set_kernel_param(const char *val, const struct kernel_param *kp)
+{
+ const char *ne_cpu_list = val;
+ struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
+ PCI_DEVICE_ID_NE, NULL);
Isn't there a better way?
+ int rc = -EINVAL;
+
+ if (!pdev)
+ return -ENODEV;
+
+ ne_teardown_cpu_pool(pdev);
+
+ rc = ne_setup_cpu_pool(pdev, ne_cpu_list);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Error in setup CPU pool [rc=%d]\n", rc);
+
+ return rc;
+ }
+
+ return param_set_copystring(val, kp);
+}
+
+/**
+ * ne_get_cpu_from_cpu_pool - Get a CPU from the CPU pool. If the vCPU id is 0,
+ * the CPU is autogenerated and chosen from the NE CPU pool.
+ *
+ * This function gets called with the ne_enclave mutex held.
+ *
+ * @ne_enclave: private data associated with the current enclave.
+ * @vcpu_id: id of the CPU to be associated with the given slot, apic id on x86.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_get_cpu_from_cpu_pool(struct ne_enclave *ne_enclave, u32 *vcpu_id)
That's a very awkward API. Can you instead just pass by-value and return
the resulting CPU ID?
+{
+ unsigned int cpu = 0;
+ unsigned int cpu_sibling = 0;
+
+ if (*vcpu_id != 0) {
+ if (cpumask_test_cpu(*vcpu_id, ne_enclave->cpu_siblings)) {
+ cpumask_clear_cpu(*vcpu_id, ne_enclave->cpu_siblings);
+
+ return 0;
+ }
+
+ mutex_lock(&ne_cpu_pool.mutex);
+
+ if (!cpumask_test_cpu(*vcpu_id, ne_cpu_pool.avail)) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "CPU %d is not in NE CPU pool\n",
+ *vcpu_id);
+
+ mutex_unlock(&ne_cpu_pool.mutex);
+
+ return -EINVAL;
I think you're better off making the return value explicit for the
error, so that user space can print the error message rather than us.
+ }
+
+ cpumask_clear_cpu(*vcpu_id, ne_cpu_pool.avail);
+
+ /*
+ * Make sure the CPU siblings are not marked as available
+ * anymore.
+ */
+ for_each_cpu(cpu_sibling, topology_sibling_cpumask(*vcpu_id)) {
+ if (cpu_sibling != *vcpu_id) {
+ cpumask_clear_cpu(cpu_sibling,
+ ne_cpu_pool.avail);
+
+ cpumask_set_cpu(cpu_sibling,
+ ne_enclave->cpu_siblings);
+ }
+ }
+
+ mutex_unlock(&ne_cpu_pool.mutex);
+
+ return 0;
+ }
+
+ /* There are CPU siblings available to choose from. */
+ cpu = cpumask_any(ne_enclave->cpu_siblings);
+ if (cpu < nr_cpu_ids) {
+ cpumask_clear_cpu(cpu, ne_enclave->cpu_siblings);
+
+ *vcpu_id = cpu;
+
+ return 0;
+ }
+
+ mutex_lock(&ne_cpu_pool.mutex);
+
+ /* Choose any CPU from the available CPU pool. */
+ cpu = cpumask_any(ne_cpu_pool.avail);
+ if (cpu >= nr_cpu_ids) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "No CPUs available in CPU pool\n");
+
+ mutex_unlock(&ne_cpu_pool.mutex);
+
+ return -EINVAL;
I think you're better off making the return value explicit for the
error, so that user space can print the error message rather than us.
+ }
+
+ cpumask_clear_cpu(cpu, ne_cpu_pool.avail);
+
+ /* Make sure the CPU siblings are not marked as available anymore. */
+ for_each_cpu(cpu_sibling, topology_sibling_cpumask(cpu)) {
+ if (cpu_sibling != cpu) {
+ cpumask_clear_cpu(cpu_sibling, ne_cpu_pool.avail);
+
+ cpumask_set_cpu(cpu_sibling, ne_enclave->cpu_siblings);
+ }
+ }
+
+ mutex_unlock(&ne_cpu_pool.mutex);
I find the function slightly confusingly structured. Why can't we do
something like
if (!vcpu_id) {
vcpu_id = find_next_free_vcpu_id();
if (vcpu_id < 0)
return -ENOSPC;
}
[logic to handle an explicit vcpu id]
I think that would be much more readable.
+
+ *vcpu_id = cpu;
+
+ return 0;
+}
+
+/**
+ * ne_create_vcpu_ioctl - Add vCPU to the slot associated with the current
+ * enclave. Create vCPU file descriptor to be further used for CPU handling.
+ *
+ * This function gets called with the ne_enclave mutex held.
+ *
+ * @ne_enclave: private data associated with the current enclave.
+ * @vcpu_id: id of the CPU to be associated with the given slot, apic id on x86.
+ *
+ * @returns: vCPU fd on success, negative return value on failure.
+ */
+static int ne_create_vcpu_ioctl(struct ne_enclave *ne_enclave, u32 vcpu_id)
+{
+ struct ne_pci_dev_cmd_reply cmd_reply = {};
+ int fd = 0;
+ struct file *file = NULL;
+ struct ne_vcpu_id *ne_vcpu_id = NULL;
+ int rc = -EINVAL;
+ struct slot_add_vcpu_req slot_add_vcpu_req = {};
+
+ if (ne_enclave->mm != current->mm)
+ return -EIO;
+
+ ne_vcpu_id = kzalloc(sizeof(*ne_vcpu_id), GFP_KERNEL);
+ if (!ne_vcpu_id)
+ return -ENOMEM;
+
+ fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fd < 0) {
+ rc = fd;
+
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in getting unused fd [rc=%d]\n", rc);
+
+ goto free_ne_vcpu_id;
+ }
+
+ /* TODO: Include (vcpu) id in the ne-vm-vcpu naming. */
+ file = anon_inode_getfile("ne-vm-vcpu", &ne_enclave_vcpu_fops,
+ ne_enclave, O_RDWR);
+ if (IS_ERR(file)) {
+ rc = PTR_ERR(file);
+
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in anon inode get file [rc=%d]\n",
+ rc);
+
+ goto put_fd;
+ }
+
+ slot_add_vcpu_req.slot_uid = ne_enclave->slot_uid;
+ slot_add_vcpu_req.vcpu_id = vcpu_id;
+
+ rc = ne_do_request(ne_enclave->pdev, SLOT_ADD_VCPU, &slot_add_vcpu_req,
+ sizeof(slot_add_vcpu_req), &cmd_reply,
+ sizeof(cmd_reply));
+ if (rc < 0) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in slot add vcpu [rc=%d]\n", rc);
+
+ goto put_file;
+ }
+
+ ne_vcpu_id->vcpu_id = vcpu_id;
+
+ list_add(&ne_vcpu_id->vcpu_id_list_entry, &ne_enclave->vcpu_ids_list);
+
+ ne_enclave->nr_vcpus++;
+
+ fd_install(fd, file);
+
+ return fd;
+
+put_file:
+ fput(file);
+put_fd:
+ put_unused_fd(fd);
+free_ne_vcpu_id:
+ kfree(ne_vcpu_id);
+
+ return rc;
+}
+
+static long ne_enclave_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct ne_enclave *ne_enclave = file->private_data;
+
+ if (!ne_enclave || !ne_enclave->pdev)
+ return -EINVAL;
+
+ switch (cmd) {
+ case NE_CREATE_VCPU: {
Can this be an ADD_VCPU rather than CREATE? We don't really need a vcpu
fd after all ...
Alex
+ int rc = -EINVAL;
+ u32 vcpu_id = 0;
+
+ if (copy_from_user(&vcpu_id, (void *)arg, sizeof(vcpu_id))) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in copy from user\n");
+
+ return -EFAULT;
+ }
+
+ mutex_lock(&ne_enclave->enclave_info_mutex);
+
+ if (ne_enclave->state != NE_STATE_INIT) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Enclave isn't in init state\n");
+
+ mutex_unlock(&ne_enclave->enclave_info_mutex);
+
+ return -EINVAL;
+ }
+
+ /* Use the CPU pool for choosing a CPU for the enclave. */
+ rc = ne_get_cpu_from_cpu_pool(ne_enclave, &vcpu_id);
+ if (rc < 0) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in get CPU from pool\n");
+
+ mutex_unlock(&ne_enclave->enclave_info_mutex);
+
+ return -EINVAL;
+ }
+
+ rc = ne_create_vcpu_ioctl(ne_enclave, vcpu_id);
+
+ /* Put back the CPU in enclave cpu pool, if add vcpu error. */
+ if (rc < 0)
+ cpumask_set_cpu(vcpu_id, ne_enclave->cpu_siblings);
+
+ mutex_unlock(&ne_enclave->enclave_info_mutex);
+
+ if (copy_to_user((void *)arg, &vcpu_id, sizeof(vcpu_id))) {
+ dev_err_ratelimited(ne_misc_dev.this_device,
+ "Error in copy to user\n");
+
+ return -EFAULT;
+ }
+
+ return rc;
+ }
+
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
static __poll_t ne_enclave_poll(struct file *file, poll_table *wait)
{
__poll_t mask = 0;
@@ -79,6 +562,7 @@ static const struct file_operations ne_enclave_fops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
.poll = ne_enclave_poll,
+ .unlocked_ioctl = ne_enclave_ioctl,
};
/**
@@ -286,8 +770,15 @@ static int __init ne_init(void)
static void __exit ne_exit(void)
{
+ struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
+ PCI_DEVICE_ID_NE, NULL);
+ if (!pdev)
+ return;
+
pci_unregister_driver(&ne_pci_driver);
+ ne_teardown_cpu_pool(pdev);
+
free_cpumask_var(ne_cpu_pool.avail);
}
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
