On Tue, 26 Jan 2021 08:19:25 -0800 Dave Hansen wrote:

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.

quoted

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

It is not used. My bad. I'll remove it.

And yes "sgx/virt: " is better. 

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.
 */

Will do. However the year should not be 2016-20, but should be 2021, right?

I think it has been ignored since the day Sean wrote the file.

quoted

+#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.

Thanks. How about below comment?

/*
 * List to temporarily hold SECS pages that cannot be EREMOVE'd due to
 * having child in other virtual EPC instances, and the lock to protect it.
 */

...

quoted

+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)?

I think no. Thanks. I'll remove IS_ENABLED(CONFIG_KVM_INTEL).

quoted

+	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.

No there's no more. The sgx_vepc_init() function declaration needs to be here
since sgx/main.c needs to use it.

May I know your suggestion?