Re: [RFC PATCH v3 06/27] x86/sgx: Introduce virtual EPC for use by KVM guests

[Dave Hansen <dave.hansen@intel.com>]

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.