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

From: Kai Huang <kai.huang@intel.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: <linux-sgx@vger.kernel.org>, <kvm@vger.kernel.org>,
	<x86@kernel.org>, <seanjc@google.com>, <jarkko@kernel.org>,
	<luto@kernel.org>, <haitao.huang@intel.com>,
	<pbonzini@redhat.com>, <bp@alien8.de>, <tglx@linutronix.de>,
	<mingo@redhat.com>, <hpa@zytor.com>
Subject: Re: [RFC PATCH v3 06/27] x86/sgx: Introduce virtual EPC for use by KVM guests
Date: Wed, 27 Jan 2021 13:48:51 +1300	[thread overview]
Message-ID: <20210127134851.b8d7830989f080c475f1447c@intel.com> (raw)
In-Reply-To: <7fbdb7f9-c7b0-d4f1-6e36-d99c6a116b82@intel.com>

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.