[PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[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

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

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

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Sean 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.

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
> 

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

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

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Sean Christopherson]

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.

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

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

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Sean Christopherson]

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.

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

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

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Sean Christopherson]

On Fri, Aug 16, 2019 at 12:56:00AM +0300, Jarkko Sakkinen wrote:
> 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.

How does the current code not align with this approach?  The core subsystem
should tear itself down if loading the driver fails, which includes failing
because it doesn't exist.

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

On Wed, Aug 21, 2019 at 10:24:00AM -0700, Sean Christopherson wrote:
> How does the current code not align with this approach?  The core subsystem
> should tear itself down if loading the driver fails, which includes failing
> because it doesn't exist.

I get now the real issue that you are trying to point out (did not
earlier). Still, I think your approach to fix it needs some
reconsideration.

Something that *does not exist* can never fail. That should be dead
obvious.

If the SGX driver does not exit and KVM does not have SGX support
compiled in, then the only logical conclusion that you can end up with
is that neither the SGX core should exist in vmlinux in the first place.

This all summarizes to that I have to remove the INTEL_SGX_DRIVER
kconfig flag. Its existence can only be considered when there >= 2
in-kernel users for SGX.

/Jarkko

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Sean Christopherson]

On Thu, Aug 22, 2019 at 03:29:40AM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 21, 2019 at 10:24:00AM -0700, Sean Christopherson wrote:
> > How does the current code not align with this approach?  The core subsystem
> > should tear itself down if loading the driver fails, which includes failing
> > because it doesn't exist.
> 
> I get now the real issue that you are trying to point out (did not
> earlier). Still, I think your approach to fix it needs some
> reconsideration.
> 
> Something that *does not exist* can never fail. That should be dead
> obvious.
> 
> If the SGX driver does not exit and KVM does not have SGX support
> compiled in, then the only logical conclusion that you can end up with
> is that neither the SGX core should exist in vmlinux in the first place.
> 
> This all summarizes to that I have to remove the INTEL_SGX_DRIVER
> kconfig flag. Its existence can only be considered when there >= 2
> in-kernel users for SGX.

That works too.

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 17:31 -0700, Sean Christopherson wrote:
> On Thu, Aug 22, 2019 at 03:29:40AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 21, 2019 at 10:24:00AM -0700, Sean Christopherson wrote:
> > > How does the current code not align with this approach?  The core subsystem
> > > should tear itself down if loading the driver fails, which includes failing
> > > because it doesn't exist.
> > 
> > I get now the real issue that you are trying to point out (did not
> > earlier). Still, I think your approach to fix it needs some
> > reconsideration.
> > 
> > Something that *does not exist* can never fail. That should be dead
> > obvious.
> > 
> > If the SGX driver does not exit and KVM does not have SGX support
> > compiled in, then the only logical conclusion that you can end up with
> > is that neither the SGX core should exist in vmlinux in the first place.
> > 
> > This all summarizes to that I have to remove the INTEL_SGX_DRIVER
> > kconfig flag. Its existence can only be considered when there >= 2
> > in-kernel users for SGX.
> 
> That works too.

It is only thing that fixes the issue. Leaving unused code to vmlinux
can be classified as a regression.

/Jarkko

Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER

[Jarkko Sakkinen]

On Thu, Aug 22, 2019 at 03:29:40AM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 21, 2019 at 10:24:00AM -0700, Sean Christopherson wrote:
> > How does the current code not align with this approach?  The core subsystem
> > should tear itself down if loading the driver fails, which includes failing
> > because it doesn't exist.
> 
> I get now the real issue that you are trying to point out (did not
> earlier). Still, I think your approach to fix it needs some
> reconsideration.
> 
> Something that *does not exist* can never fail. That should be dead
> obvious.
> 
> If the SGX driver does not exit and KVM does not have SGX support
> compiled in, then the only logical conclusion that you can end up with
> is that neither the SGX core should exist in vmlinux in the first place.
> 
> This all summarizes to that I have to remove the INTEL_SGX_DRIVER
> kconfig flag. Its existence can only be considered when there >= 2
> in-kernel users for SGX.

The subdirectory for the driver has clearly turned into legacy with only
two source files and one header file in it.

Even if/when we want to add INTEL_SGX_DRIVER back it is easiest use it
in the main makefile.

I prepared snippets to execute these changes.

This is for driver.h, ioctl.c, main.c (rename):

git filter-branch -f --index-filter 'git ls-files -s | \
    sed "s/sgx\/driver\/main\.c/sgx\/driver.c/" | \
    GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index --index-info && \
    mv "$GIT_INDEX_FILE.new" "$GIT_INDEX_FILE"' mainline/master..master

This for for Makefile (remove):

git filter-branch -f --index-filter 'git ls-files -s | \
    sed "/sgx\/driver\/Makefile/d" | \
    GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index --index-info && \
    mv "$GIT_INDEX_FILE.new" "$GIT_INDEX_FILE"' mainline/master..master

/Jarkko