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?
