On Tue, 26 Jan 2021 16:27:25 -0800 Dave Hansen wrote:
> On 1/26/21 4:16 PM, Kai Huang wrote:
> > On Tue, 26 Jan 2021 08:19:25 -0800 Dave Hansen wrote:
> >> 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.
>
> Yes, should be 2021. Also, there shouldn't be *ANY* parts of these
> files which you, the submitter and newly-minted effective maintainer,
> have ignored.
Yes agreed.
>
> It sounds like you owe us some homework to give every line of these a
> once-over.
I'll also check other files. Thanks.
>
> ...
> >>> +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.
> > */
>
> Fine. It's just a bit silly to say that it's a list. It's also not so
> temporary. Pages can live on here forever.
I'll remove the 'List':
/* SECS pages that cannot be EREMOVE'd due to... */
The list should be empty after VM's all virtual EPC instances have been
released. If one page lives in list forever, the WARN_ONCE() in
sgx_vepc_free_page() will catch it, and there's bug here.
>
> >>> + 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?
>
> I'd toss it in some other existing header that has more meat in it. I'm
> lazy.
>
I can put it into arch/x86/kernel/cpu/sgx/sgx.h, if it is good to you.
