| Message ID | 20190714143212.971-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Return 0 when !CONFIG_INTEL_SGX_DRIVER | expand |
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
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 >
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
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.
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
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.
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
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.
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
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.
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
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
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
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(-)