Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
@ 2019-07-14 14:32 Jarkko Sakkinen
  2019-07-15  9:29 ` Jarkko Sakkinen
  2019-07-15 13:59 ` Sean Christopherson
  0 siblings, 2 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-07-14 14:32 UTC (permalink / raw)
  To: linux-sgx; +Cc: sean.j.christopherson, Jarkko Sakkinen

When the config option is not enabled the initialization is always
succesful.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
This was the reason why I got the -ENODEV error with my BuildRoot image.
The config option was not enabled but took some time realize as I was
kind of getting an error code from the driver initialization. Finally
when I used ftrace with 'sgx*' I knew what was going on.
 arch/x86/kernel/cpu/sgx/driver/driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/driver.h b/arch/x86/kernel/cpu/sgx/driver/driver.h
index da60839b133a..aafa64a4f481 100644
--- a/arch/x86/kernel/cpu/sgx/driver/driver.h
+++ b/arch/x86/kernel/cpu/sgx/driver/driver.h
@@ -37,7 +37,7 @@ int sgx_drv_init(void);
 #else
 static inline int sgx_drv_init(void)
 {
-	return -ENODEV;
+	return 0;
 }
 #endif
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-07-14 14:32 [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER Jarkko Sakkinen
@ 2019-07-15  9:29 ` Jarkko Sakkinen
  2019-07-15 13:59 ` Sean Christopherson
  1 sibling, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-07-15  9:29 UTC (permalink / raw)
  To: linux-sgx; +Cc: sean.j.christopherson

On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> When the config option is not enabled the initialization is always
> succesful.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

It would whole a lot simpler if the call was just flagged:

#ifdef CONFIG_INTEL_SGX_DRIVER
	ret = sgx_drv_init();
	if (ret)
		goto err_kthread;
#endif

/Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-07-14 14:32 [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER Jarkko Sakkinen
  2019-07-15  9:29 ` Jarkko Sakkinen
@ 2019-07-15 13:59 ` Sean Christopherson
  2019-08-01 16:22   ` Jarkko Sakkinen
  1 sibling, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2019-07-15 13:59 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> When the config option is not enabled the initialization is always
> succesful.

Why would the be initialization be considered successful?  It's dead code
and memory consumption if the driver can't load.  When KVM support gets
added, the initialization can be considered successful if the driver *or*
virtual EPC are enabled and load cleanly.

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> This was the reason why I got the -ENODEV error with my BuildRoot image.
> The config option was not enabled but took some time realize as I was
> kind of getting an error code from the driver initialization. Finally
> when I used ftrace with 'sgx*' I knew what was going on.
>  arch/x86/kernel/cpu/sgx/driver/driver.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/driver.h b/arch/x86/kernel/cpu/sgx/driver/driver.h
> index da60839b133a..aafa64a4f481 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/driver.h
> +++ b/arch/x86/kernel/cpu/sgx/driver/driver.h
> @@ -37,7 +37,7 @@ int sgx_drv_init(void);
>  #else
>  static inline int sgx_drv_init(void)
>  {
> -	return -ENODEV;
> +	return 0;
>  }
>  #endif
>  
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-07-15 13:59 ` Sean Christopherson
@ 2019-08-01 16:22   ` Jarkko Sakkinen
  2019-08-01 16:29     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-08-01 16:22 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

On Mon, Jul 15, 2019 at 06:59:03AM -0700, Sean Christopherson wrote:
> On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> > When the config option is not enabled the initialization is always
> > succesful.
> 
> Why would the be initialization be considered successful?  It's dead code
> and memory consumption if the driver can't load.  When KVM support gets
> added, the initialization can be considered successful if the driver *or*
> virtual EPC are enabled and load cleanly.

When a config option disabled means it that the functionality does not
exist at all, which means that there is nothing to fail. That is why it
would be actually better to flag the whole call than the way it is done
in this patch.

/Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-08-01 16:22   ` Jarkko Sakkinen
@ 2019-08-01 16:29     ` Sean Christopherson
  2019-08-02 19:33       ` Jarkko Sakkinen
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2019-08-01 16:29 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

On Thu, Aug 01, 2019 at 07:22:19PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 15, 2019 at 06:59:03AM -0700, Sean Christopherson wrote:
> > On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> > > When the config option is not enabled the initialization is always
> > > succesful.
> > 
> > Why would the be initialization be considered successful?  It's dead code
> > and memory consumption if the driver can't load.  When KVM support gets
> > added, the initialization can be considered successful if the driver *or*
> > virtual EPC are enabled and load cleanly.
> 
> When a config option disabled means it that the functionality does not
> exist at all, which means that there is nothing to fail. That is why it
> would be actually better to flag the whole call than the way it is done
> in this patch.

Regardless of how it's done, the core SGX management shouldn't consume
resources if it doesn't have downstream consumers.  Making INTEL_SGX
depend on INTEL_SGX_DRIVER is the obvious alternative.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-08-01 16:29     ` Sean Christopherson
@ 2019-08-02 19:33       ` Jarkko Sakkinen
  2019-08-13  1:22         ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-08-02 19:33 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

On Thu, Aug 01, 2019 at 09:29:31AM -0700, Sean Christopherson wrote:
> On Thu, Aug 01, 2019 at 07:22:19PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 15, 2019 at 06:59:03AM -0700, Sean Christopherson wrote:
> > > On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> > > > When the config option is not enabled the initialization is always
> > > > succesful.
> > > 
> > > Why would the be initialization be considered successful?  It's dead code
> > > and memory consumption if the driver can't load.  When KVM support gets
> > > added, the initialization can be considered successful if the driver *or*
> > > virtual EPC are enabled and load cleanly.
> > 
> > When a config option disabled means it that the functionality does not
> > exist at all, which means that there is nothing to fail. That is why it
> > would be actually better to flag the whole call than the way it is done
> > in this patch.
> 
> Regardless of how it's done, the core SGX management shouldn't consume
> resources if it doesn't have downstream consumers.  Making INTEL_SGX
> depend on INTEL_SGX_DRIVER is the obvious alternative.

Is there a specific blocker that prevents using SGX just with KVM when
the latter option is disabled?

/Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-08-02 19:33       ` Jarkko Sakkinen
@ 2019-08-13  1:22         ` Sean Christopherson
  2019-08-15 21:56           ` Jarkko Sakkinen
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2019-08-13  1:22 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

On Fri, Aug 02, 2019 at 10:33:38PM +0300, Jarkko Sakkinen wrote:
> On Thu, Aug 01, 2019 at 09:29:31AM -0700, Sean Christopherson wrote:
> > On Thu, Aug 01, 2019 at 07:22:19PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Jul 15, 2019 at 06:59:03AM -0700, Sean Christopherson wrote:
> > > > On Sun, Jul 14, 2019 at 05:32:12PM +0300, Jarkko Sakkinen wrote:
> > > > > When the config option is not enabled the initialization is always
> > > > > succesful.
> > > > 
> > > > Why would the be initialization be considered successful?  It's dead code
> > > > and memory consumption if the driver can't load.  When KVM support gets
> > > > added, the initialization can be considered successful if the driver *or*
> > > > virtual EPC are enabled and load cleanly.
> > > 
> > > When a config option disabled means it that the functionality does not
> > > exist at all, which means that there is nothing to fail. That is why it
> > > would be actually better to flag the whole call than the way it is done
> > > in this patch.
> > 
> > Regardless of how it's done, the core SGX management shouldn't consume
> > resources if it doesn't have downstream consumers.  Making INTEL_SGX
> > depend on INTEL_SGX_DRIVER is the obvious alternative.
> 
> Is there a specific blocker that prevents using SGX just with KVM when
> the latter option is disabled?

Nope, KVM does not have any dependencies on the native driver.  But if
sgx_drv_init() returns 0 when CONFIG_INTEL_SGX_DRIVER=n, then sgx_init()
won't handle KVM failure correctly since it will think the native driver
initialized cleanly.  E.g. with both KVM and driver in play, I was
thinking of something like this in sgx_init():

        /* Success if the native *or* virtual driver initialized cleanly. */
        ret = sgx_drv_init();
        ret = sgx_virt_epc_init() ? ret : 0;
        if (ret)
		goto err;

	return 0;

If sgx_drv_init() returns 0 when CONFIG_INTEL_SGX_DRIVER=n, then failure
in sgx_virt_epc_init() is ignored and we end up with the SGX subsystem
wasting resources again.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER
  2019-08-13  1:22         ` Sean Christopherson
@ 2019-08-15 21:56           ` Jarkko Sakkinen
  0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-08-15 21:56 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

On Mon, Aug 12, 2019 at 06:22:27PM -0700, Sean Christopherson wrote:
> Nope, KVM does not have any dependencies on the native driver.  But if
> sgx_drv_init() returns 0 when CONFIG_INTEL_SGX_DRIVER=n, then sgx_init()
> won't handle KVM failure correctly since it will think the native driver
> initialized cleanly.  E.g. with both KVM and driver in play, I was
> thinking of something like this in sgx_init():
> 
>         /* Success if the native *or* virtual driver initialized cleanly. */
>         ret = sgx_drv_init();
>         ret = sgx_virt_epc_init() ? ret : 0;
>         if (ret)
> 		goto err;
> 
> 	return 0;
> 
> If sgx_drv_init() returns 0 when CONFIG_INTEL_SGX_DRIVER=n, then failure
> in sgx_virt_epc_init() is ignored and we end up with the SGX subsystem
> wasting resources again.

I get what you are saying but what exist now does not align with this
and on the other hand nothing prevents the reconsider the flow once we
get to this point.

/Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-14 14:32 [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER Jarkko Sakkinen
2019-07-15  9:29 ` Jarkko Sakkinen
2019-07-15 13:59 ` Sean Christopherson
2019-08-01 16:22   ` Jarkko Sakkinen
2019-08-01 16:29     ` Sean Christopherson
2019-08-02 19:33       ` Jarkko Sakkinen
2019-08-13  1:22         ` Sean Christopherson
2019-08-15 21:56           ` Jarkko Sakkinen

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/ public-inbox