Re: [RFC PATCH v3 08/27] x86/sgx: Initialize virtual EPC driver even when SGX driver is disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 3 Feb 2021 01:16:20 +0200 Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 10:20:47AM -0800, Sean Christopherson wrote:
> > On Tue, Feb 02, 2021, Jarkko Sakkinen wrote:
> > > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote:
> > > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote:
> > > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote:
> > > > > > Modify sgx_init() to always try to initialize the virtual EPC driver,
> > > > > > even if the bare-metal SGX driver is disabled.  The bare-metal driver
> > > > > > might be disabled if SGX Launch Control is in locked mode, or not
> > > > > > supported in the hardware at all.  This allows (non-Linux) guests that
> > > > > > support non-LC configurations to use SGX.
> > > > > > 
> > > > > > Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> > > > > > ---
> > > > > > v2->v3:
> > > > > > 
> > > > > >  - Changed from sgx_virt_epc_init() to sgx_vepc_init().
> > > > > > 
> > > > > > ---
> > > > > >  arch/x86/kernel/cpu/sgx/main.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > index 21c2ffa13870..93d249f7bff3 100644
> > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > > > @@ -12,6 +12,7 @@
> > > > > >  #include "driver.h"
> > > > > >  #include "encl.h"
> > > > > >  #include "encls.h"
> > > > > > +#include "virt.h"
> > > > > >  
> > > > > >  struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> > > > > >  static int sgx_nr_epc_sections;
> > > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void)
> > > > > >  		goto err_page_cache;
> > > > > >  	}
> > > > > >  
> > > > > > -	ret = sgx_drv_init();
> > > > > > +	/* Success if the native *or* virtual EPC driver initialized cleanly. */
> > > > > > +	ret = !!sgx_drv_init() & !!sgx_vepc_init();
> > > > > 
> > > > > If would create more dumb code and just add
> > > > > 
> > > > > ret = sgx_vepc_init()
> > > > > if (ret)
> > > > >         goto err_kthread;
> > > > 
> > > > Do you mean you want below?
> > > > 
> > > > 	ret = sgx_drv_init();
> > > > 	ret = sgx_vepc_init();
> > > > 	if (ret)
> > > > 		goto err_kthread;
> > > > 
> > > > This was Sean's original code, but Dave didn't like it.
> > > 
> > > I think it should be like:
> > > 
> > > ret = sgx_drv_init();
> > > if (ret)
> > >         pr_warn("Driver initialization failed with %d\n", ret);
> > > 
> > > ret = sgx_vepc_init();
> > > if (ret)
> > > 	goto err_kthread;
> > 
> > And that's wrong, it doesn't correctly handle the case where sgx_drv_init()
> > succeeds but sgx_vepc_init() fails.
> 
> After reading all of this, I think that the only acceptable way to
> to manage this is to
> 
> ret = sgx_drv_init();
> if (ret && ret != -ENODEV)
>         goto err_kthread;

Why? From SGX virtualization's perspective, it doesn't care what error code
caused driver not being initialized properly. Actually it even doesn't care
about whether driver initialization is successful or not.

> 
> ret = sgx_vepc_init();
> if (ret)
> 	goto err_kthread;
> 
> Anything else would be a bad idea.
> 
> We do support allowing KVM when the driver does not *support* SGX,
> not when something is working incorrectly. 

What working *incorrectly* thing is related to SGX virtualization? The things
SGX virtualization requires (basically just raw EPC allocation) are all in
sgx/main.c. 

In that case it is a bad
> idea to allow any SGX related initialization to continue.
> 
> Agreed that my earlier example is incorrect but so is the condition
> in the original patch.
> 
> /Jarkko 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux