I'd also like to see some comments about code sharing between this and
the main driver. For instance, this *could* try to share 99% of the
->fault function. Why doesn't it? I'm sure there's a good reason.
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> new file mode 100644
> index 000000000000..e1ad7856d878
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2016-20 Intel Corporation. */
> +
> +#define pr_fmt(fmt) "SGX virtual EPC: " fmt
Does this actually get used anywhere? Also, isn't this a bit long? Maybe:
#define pr_fmt(fmt) "sgx/virt: " fmt
Also, a one-line summary about what's in here would be nice next to the
copyright (which needs to be updated).
/*
* Device driver to expose SGX enclave memory to KVM guests.
*
* Copyright(c) 2016-20 Intel Corporation.
*/
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/sched/mm.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
> +#include <linux/xarray.h>
> +#include <asm/sgx.h>
> +#include <uapi/asm/sgx.h>
> +
> +#include "encls.h"
> +#include "sgx.h"
> +#include "virt.h"
> +
> +struct sgx_vepc {
> + struct xarray page_array;
> + struct mutex lock;
> +};
> +
> +static struct mutex zombie_secs_pages_lock;
> +static struct list_head zombie_secs_pages;
Comments would be nice for this random lock and list.
The main core functions (fault, etc...) are looking OK to me.
...
> +int __init sgx_vepc_init(void)
> +{
> + /* SGX virtualization requires KVM to work */
> + if (!boot_cpu_has(X86_FEATURE_VMX) || !IS_ENABLED(CONFIG_KVM_INTEL))
> + return -ENODEV;
Can this even be built without IS_ENABLED(CONFIG_KVM_INTEL)?
> + INIT_LIST_HEAD(&zombie_secs_pages);
> + mutex_init(&zombie_secs_pages_lock);
> +
> + return misc_register(&sgx_vepc_dev);
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/virt.h b/arch/x86/kernel/cpu/sgx/virt.h
> new file mode 100644
> index 000000000000..44d872380ca1
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/virt.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +#ifndef _ASM_X86_SGX_VIRT_H
> +#define _ASM_X86_SGX_VIRT_H
> +
> +#ifdef CONFIG_X86_SGX_KVM
> +int __init sgx_vepc_init(void);
> +#else
> +static inline int __init sgx_vepc_init(void)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +#endif /* _ASM_X86_SGX_VIRT_H */
Is more going to go in this header? It's a little sparse as-is.
