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

[Kai Huang <kai.huang@intel.com>]

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

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

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

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