diff mbox series

[v3,07/25] x86/sgx: Initialize virtual EPC driver even when SGX driver is disabled

Message ID d35d17a02bbf8feef83a536cec8b43746d4ea557.1616136308.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM SGX virtualization support | expand

Commit Message

Huang, Kai March 19, 2021, 7:23 a.m. UTC 
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled.  The SGX 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.

Acked-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Borislav Petkov April 2, 2021, 9:48 a.m. UTC | #1 
On Fri, Mar 19, 2021 at 08:23:02PM +1300, Kai Huang wrote:
> Modify sgx_init() to always try to initialize the virtual EPC driver,
> even if the SGX driver is disabled.  The SGX 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.
> 
> Acked-by: Dave Hansen <dave.hansen@intel.com>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 6a734f484aa7..b73114150ff8 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -743,7 +743,15 @@ static int __init sgx_init(void)
>  		goto err_page_cache;
>  	}
>  
> -	ret = sgx_drv_init();
> +	/*
> +	 * Always try to initialize the native *and* KVM drivers.
> +	 * The KVM driver is less picky than the native one and
> +	 * can function if the native one is not supported on the
> +	 * current system or fails to initialize.
> +	 *
> +	 * Error out only if both fail to initialize.
> +	 */
> +	ret = !!sgx_drv_init() & !!sgx_vepc_init();

This is a silly way of writing:

        if (sgx_drv_init() && sgx_vepc_init())
                goto err_kthread;

methinks.
Huang, Kai April 2, 2021, 11:08 a.m. UTC | #2 
On Fri, 2 Apr 2021 11:48:16 +0200 Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 08:23:02PM +1300, Kai Huang wrote:
> > Modify sgx_init() to always try to initialize the virtual EPC driver,
> > even if the SGX driver is disabled.  The SGX 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.
> > 
> > Acked-by: Dave Hansen <dave.hansen@intel.com>
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 6a734f484aa7..b73114150ff8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -743,7 +743,15 @@ static int __init sgx_init(void)
> >  		goto err_page_cache;
> >  	}
> >  
> > -	ret = sgx_drv_init();
> > +	/*
> > +	 * Always try to initialize the native *and* KVM drivers.
> > +	 * The KVM driver is less picky than the native one and
> > +	 * can function if the native one is not supported on the
> > +	 * current system or fails to initialize.
> > +	 *
> > +	 * Error out only if both fail to initialize.
> > +	 */
> > +	ret = !!sgx_drv_init() & !!sgx_vepc_init();
> 
> This is a silly way of writing:
> 
>         if (sgx_drv_init() && sgx_vepc_init())
>                 goto err_kthread;
> 
> methinks.

Works for me. Thanks.

Do you want me to send updated patch?
Borislav Petkov April 2, 2021, 11:22 a.m. UTC | #3 
On Sat, Apr 03, 2021 at 12:08:10AM +1300, Kai Huang wrote:
> Do you want me to send updated patch?

No need. If I do, I'll ask kindly, otherwise you don't have to do
anything.

Thx.
Huang, Kai April 2, 2021, 11:38 a.m. UTC | #4 
On Fri, 2 Apr 2021 13:22:35 +0200 Borislav Petkov wrote:
> On Sat, Apr 03, 2021 at 12:08:10AM +1300, Kai Huang wrote:
> > Do you want me to send updated patch?
> 
> No need. If I do, I'll ask kindly, otherwise you don't have to do
> anything.
> 
I see. Thanks.
Sean Christopherson April 2, 2021, 3:42 p.m. UTC | #5 
On Fri, Apr 02, 2021, Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 08:23:02PM +1300, Kai Huang wrote:
> > Modify sgx_init() to always try to initialize the virtual EPC driver,
> > even if the SGX driver is disabled.  The SGX 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.
> > 
> > Acked-by: Dave Hansen <dave.hansen@intel.com>
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 6a734f484aa7..b73114150ff8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -743,7 +743,15 @@ static int __init sgx_init(void)
> >  		goto err_page_cache;
> >  	}
> >  
> > -	ret = sgx_drv_init();
> > +	/*
> > +	 * Always try to initialize the native *and* KVM drivers.
> > +	 * The KVM driver is less picky than the native one and
> > +	 * can function if the native one is not supported on the
> > +	 * current system or fails to initialize.
> > +	 *
> > +	 * Error out only if both fail to initialize.
> > +	 */
> > +	ret = !!sgx_drv_init() & !!sgx_vepc_init();
> 
> This is a silly way of writing:
> 
>         if (sgx_drv_init() && sgx_vepc_init())
>                 goto err_kthread;
> 
> methinks.

Nope!  That's wrong, as sgx_epc_init() will not be called if sgx_drv_init()
succeeds.  And writing it as "if (sgx_drv_init() || sgx_vepc_init())" is also
wrong since that would kill SGX when one of the drivers is alive and well.
Huang, Kai April 2, 2021, 7:08 p.m. UTC | #6 
On Fri, 2 Apr 2021 15:42:51 +0000 Sean Christopherson wrote:
> On Fri, Apr 02, 2021, Borislav Petkov wrote:
> > On Fri, Mar 19, 2021 at 08:23:02PM +1300, Kai Huang wrote:
> > > Modify sgx_init() to always try to initialize the virtual EPC driver,
> > > even if the SGX driver is disabled.  The SGX 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.
> > > 
> > > Acked-by: Dave Hansen <dave.hansen@intel.com>
> > > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > index 6a734f484aa7..b73114150ff8 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -743,7 +743,15 @@ static int __init sgx_init(void)
> > >  		goto err_page_cache;
> > >  	}
> > >  
> > > -	ret = sgx_drv_init();
> > > +	/*
> > > +	 * Always try to initialize the native *and* KVM drivers.
> > > +	 * The KVM driver is less picky than the native one and
> > > +	 * can function if the native one is not supported on the
> > > +	 * current system or fails to initialize.
> > > +	 *
> > > +	 * Error out only if both fail to initialize.
> > > +	 */
> > > +	ret = !!sgx_drv_init() & !!sgx_vepc_init();
> > 
> > This is a silly way of writing:
> > 
> >         if (sgx_drv_init() && sgx_vepc_init())
> >                 goto err_kthread;
> > 
> > methinks.
> 
> Nope!  That's wrong, as sgx_epc_init() will not be called if sgx_drv_init()
> succeeds.  And writing it as "if (sgx_drv_init() || sgx_vepc_init())" is also
> wrong since that would kill SGX when one of the drivers is alive and well.

Right. Thanks for pointing out.
Borislav Petkov April 2, 2021, 7:19 p.m. UTC | #7 
On Fri, Apr 02, 2021 at 03:42:51PM +0000, Sean Christopherson wrote:
> Nope!  That's wrong, as sgx_epc_init() will not be called if sgx_drv_init()
> succeeds.  And writing it as "if (sgx_drv_init() || sgx_vepc_init())" is also
> wrong since that would kill SGX when one of the drivers is alive and well.

Bah, right you are.

How about:

	/* Error out only if both fail to initialize. */
        ret = sgx_drv_init();

        if (sgx_vepc_init() && ret)
                goto err_kthread;

And yah, this looks strange so it needs the comment to explain what's
going on here.

Thx.
Sean Christopherson April 2, 2021, 7:30 p.m. UTC | #8 
On Fri, Apr 02, 2021, Borislav Petkov wrote:
> On Fri, Apr 02, 2021 at 03:42:51PM +0000, Sean Christopherson wrote:
> > Nope!  That's wrong, as sgx_epc_init() will not be called if sgx_drv_init()
> > succeeds.  And writing it as "if (sgx_drv_init() || sgx_vepc_init())" is also
> > wrong since that would kill SGX when one of the drivers is alive and well.
> 
> Bah, right you are.
> 
> How about:
> 
> 	/* Error out only if both fail to initialize. */
>         ret = sgx_drv_init();
> 
>         if (sgx_vepc_init() && ret)
>                 goto err_kthread;

Heh, that's what I had originally and used for literally years.  IIRC, I
suggested the "!! & !!" abomination after internal review complained about the
oddness of the above.

FWIW, I think the above is far less likely to be misread, even though I love the
cleverness of the bitwise AND.

> 
> And yah, this looks strange so it needs the comment to explain what's
> going on here.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov April 2, 2021, 7:46 p.m. UTC | #9 
On Fri, Apr 02, 2021 at 07:30:23PM +0000, Sean Christopherson wrote:
> Heh, that's what I had originally and used for literally years.  IIRC, I
> suggested the "!! & !!" abomination after internal review complained about the
> oddness of the above.

Whut?

> FWIW, I think the above is far less likely to be misread, even though I love the
> cleverness of the bitwise AND.

The problem with using bitwise operations here is that they don't belong
in a logical expression of this sort - you do those when you actually
work with bitmasks etc and not when you wanna check whether the functions
returned success or not.

Yeah, yeah, the bitwise thing gets you what you want and yadda yadda but
readability is important. That thing that keeps this code maintainable
years from now...

Also, your original suggestion is literally translating the comment in
code, while

	!!sgx_drv_init() & !!sgx_vepc_init()

especially with that bitwise-& in there, makes me go "say what now?!"

So yeah, you were right the first time.

:-)
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 6a734f484aa7..b73114150ff8 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -743,7 +743,15 @@  static int __init sgx_init(void)
 		goto err_page_cache;
 	}
 
-	ret = sgx_drv_init();
+	/*
+	 * Always try to initialize the native *and* KVM drivers.
+	 * The KVM driver is less picky than the native one and
+	 * can function if the native one is not supported on the
+	 * current system or fails to initialize.
+	 *
+	 * Error out only if both fail to initialize.
+	 */
+	ret = !!sgx_drv_init() & !!sgx_vepc_init();
 	if (ret)
 		goto err_kthread;