* [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; 13+ 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 related [flat|nested] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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 2019-08-21 17:24 ` Sean Christopherson 0 siblings, 1 reply; 13+ 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] 13+ messages in thread
* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER 2019-08-15 21:56 ` Jarkko Sakkinen @ 2019-08-21 17:24 ` Sean Christopherson 2019-08-22 0:29 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-08-21 17:24 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER 2019-08-21 17:24 ` Sean Christopherson @ 2019-08-22 0:29 ` Jarkko Sakkinen 2019-08-22 0:31 ` Sean Christopherson 2019-08-22 1:26 ` Jarkko Sakkinen 0 siblings, 2 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2019-08-22 0:29 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER 2019-08-22 0:29 ` Jarkko Sakkinen @ 2019-08-22 0:31 ` Sean Christopherson 2019-08-22 14:42 ` Jarkko Sakkinen 2019-08-22 1:26 ` Jarkko Sakkinen 1 sibling, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-08-22 0:31 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER 2019-08-22 0:31 ` Sean Christopherson @ 2019-08-22 14:42 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2019-08-22 14:42 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER 2019-08-22 0:29 ` Jarkko Sakkinen 2019-08-22 0:31 ` Sean Christopherson @ 2019-08-22 1:26 ` Jarkko Sakkinen 1 sibling, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2019-08-22 1:26 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-08-22 14:42 UTC | newest] Thread overview: 13+ 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 2019-08-21 17:24 ` Sean Christopherson 2019-08-22 0:29 ` Jarkko Sakkinen 2019-08-22 0:31 ` Sean Christopherson 2019-08-22 14:42 ` Jarkko Sakkinen 2019-08-22 1:26 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).