[intel-sgx-kernel-dev,2/2] intel_sgx: remove option to build as a module
diff mbox

Message ID 1504029396-3353-3-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Aug. 29, 2017, 5:56 p.m. UTC
Build SGX support directly into the kernel, utilizing syscore_ops
instead of dev_pm_ops to hook into the suspend flow.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/Kconfig                         |  4 +-
 arch/x86/kernel/cpu/intel_sgx/Makefile   | 13 +++---
 arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
 3 files changed, 24 insertions(+), 67 deletions(-)

Comments

Kai Huang Aug. 30, 2017, 4:37 a.m. UTC | #1
On Tue, 2017-08-29 at 10:56 -0700, Sean Christopherson wrote:
> Build SGX support directly into the kernel, utilizing syscore_ops
> instead of dev_pm_ops to hook into the suspend flow.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/Kconfig                         |  4 +-
>  arch/x86/kernel/cpu/intel_sgx/Makefile   | 13 +++---
>  arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------
> ------------
>  3 files changed, 24 insertions(+), 67 deletions(-)
> 
...

> -
> -module_platform_driver(sgx_drv);
> -
> -MODULE_LICENSE("Dual BSD/GPL");
> +device_initcall(sgx_init);

As SGX is not treated as device anymore, maybe using arch_initcall, or
late_initcall would be better?

Using syscore_ops looks good to me.

Thanks,
-Kai
Jarkko Sakkinen Aug. 30, 2017, 10:05 a.m. UTC | #2
Not going to take this.

/Jarkko

On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> Build SGX support directly into the kernel, utilizing syscore_ops
> instead of dev_pm_ops to hook into the suspend flow.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/Kconfig                         |  4 +-
>  arch/x86/kernel/cpu/intel_sgx/Makefile   | 13 +++---
>  arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
>  3 files changed, 24 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index fb8c91f..5f4d414 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
>  	  If unsure, say y.
>  
>  config INTEL_SGX
> -	tristate "Intel(R) SGX Driver"
> -	default n
> +	prompt "Intel SGX (Secure Guard Extensions)"
> +	def_bool n
>  	depends on X86
>  	select MMU_NOTIFIER
>  	---help---
> diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile b/arch/x86/kernel/cpu/intel_sgx/Makefile
> index b87f4e1..71b85b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/Makefile
> +++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
> @@ -2,11 +2,8 @@
>  # Intel SGX
>  #
>  
> -obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
> -
> -intel_sgx-$(CONFIG_INTEL_SGX) += \
> -	sgx_ioctl.o \
> -	sgx_main.o \
> -	sgx_page_cache.o \
> -	sgx_util.o \
> -	sgx_vma.o \
> +obj-y += sgx_ioctl.o \
> +	 sgx_main.o \
> +	 sgx_page_cache.o \
> +	 sgx_util.o \
> +	 sgx_vma.o \
> diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> index 50d9af8..5f411b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> +++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> @@ -59,22 +59,12 @@
>   */
>  
>  #include "sgx.h"
> -#include <linux/acpi.h>
>  #include <linux/file.h>
>  #include <linux/highmem.h>
>  #include <linux/miscdevice.h>
> -#include <linux/module.h>
> -#include <linux/suspend.h>
>  #include <linux/hashtable.h>
>  #include <linux/kthread.h>
> -#include <linux/platform_device.h>
> -
> -#define DRV_DESCRIPTION "Intel SGX Driver"
> -#define DRV_VERSION "0.10"
> -
> -MODULE_DESCRIPTION(DRV_DESCRIPTION);
> -MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
> -MODULE_VERSION(DRV_VERSION);
> +#include <linux/syscore_ops.h>
>  
>  /*
>   * Global data.
> @@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
>  	.mode   = 0666,
>  };
>  
> -static int sgx_pm_suspend(struct device *dev)
> +static int sgx_pm_suspend(void)
>  {
>  	struct sgx_tgid_ctx *ctx;
>  	struct sgx_encl *encl;
> @@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
>  
> -static int sgx_dev_init(struct device *dev)
> +static struct syscore_ops sgx_syscore_ops = {
> +	.suspend	= sgx_pm_suspend,
> +};
> +
> +static int __init sgx_dev_init(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  	unsigned long pa;
> @@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
>  	int ret;
>  	int i;
>  
> -	pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n");
> -
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
>  		return -ENODEV;
>  
> @@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
>  		pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
>  		size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
>  
> -		dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
> +		pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
>  
>  		sgx_epc_banks[i].pa = pa;
>  		sgx_epc_banks[i].size = size;
> @@ -238,10 +229,11 @@ static int sgx_dev_init(struct device *dev)
>  	if (!sgx_add_page_wq) {
>  		pr_err("intel_sgx: alloc_workqueue() failed\n");
>  		ret = -ENOMEM;
> -		goto out_iounmap;
> +		goto out_pagecache;
>  	}
>  
> -	sgx_dev.parent = dev;
> +	register_syscore_ops(&sgx_syscore_ops);
> +
>  	ret = misc_register(&sgx_dev);
>  	if (ret) {
>  		pr_err("intel_sgx: misc_register() failed\n");
> @@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
>  
>  	return 0;
>  out_workqueue:
> +	unregister_syscore_ops(&sgx_syscore_ops);
>  	destroy_workqueue(sgx_add_page_wq);
> +out_pagecache:
> +	sgx_page_cache_teardown();
>  out_iounmap:
>  #ifdef CONFIG_X86_64
>  	for (i = 0; i < sgx_nr_epc_banks; i++)
> @@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
>  	return ret;
>  }
>  
> -static int sgx_drv_probe(struct platform_device *pdev)
> +static int __init sgx_init(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  	int i;
> @@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
>  #endif
>  	sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
>  
> -	return sgx_dev_init(&pdev->dev);
> +	return sgx_dev_init();
>  }
>  
> -static int sgx_drv_remove(struct platform_device *pdev)
> -{
> -	int i;
> -
> -	misc_deregister(&sgx_dev);
> -	destroy_workqueue(sgx_add_page_wq);
> -#ifdef CONFIG_X86_64
> -	for (i = 0; i < sgx_nr_epc_banks; i++)
> -		iounmap((void *)sgx_epc_banks[i].va);
> -#endif
> -	sgx_page_cache_teardown();
> -
> -	return 0;
> -}
> -
> -#ifdef CONFIG_ACPI
> -static struct acpi_device_id sgx_device_ids[] = {
> -	{"INT0E0C", 0},
> -	{"", 0},
> -};
> -MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
> -#endif
> -
> -static struct platform_driver sgx_drv = {
> -	.probe = sgx_drv_probe,
> -	.remove = sgx_drv_remove,
> -	.driver = {
> -		.name			= "intel_sgx",
> -		.pm			= &sgx_drv_pm,
> -		.acpi_match_table	= ACPI_PTR(sgx_device_ids),
> -	},
> -};
> -
> -module_platform_driver(sgx_drv);
> -
> -MODULE_LICENSE("Dual BSD/GPL");
> +device_initcall(sgx_init);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Jarkko Sakkinen Aug. 30, 2017, 6:47 p.m. UTC | #3
On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> Build SGX support directly into the kernel, utilizing syscore_ops
> instead of dev_pm_ops to hook into the suspend flow.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I think you are looking this only from container perspective. For
desktop distribution it *does* make sense to be able to compile
this as a module.

/Jarkko

> ---
>  arch/x86/Kconfig                         |  4 +-
>  arch/x86/kernel/cpu/intel_sgx/Makefile   | 13 +++---
>  arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
>  3 files changed, 24 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index fb8c91f..5f4d414 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
>  	  If unsure, say y.
>  
>  config INTEL_SGX
> -	tristate "Intel(R) SGX Driver"
> -	default n
> +	prompt "Intel SGX (Secure Guard Extensions)"
> +	def_bool n
>  	depends on X86
>  	select MMU_NOTIFIER
>  	---help---
> diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile b/arch/x86/kernel/cpu/intel_sgx/Makefile
> index b87f4e1..71b85b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/Makefile
> +++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
> @@ -2,11 +2,8 @@
>  # Intel SGX
>  #
>  
> -obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
> -
> -intel_sgx-$(CONFIG_INTEL_SGX) += \
> -	sgx_ioctl.o \
> -	sgx_main.o \
> -	sgx_page_cache.o \
> -	sgx_util.o \
> -	sgx_vma.o \
> +obj-y += sgx_ioctl.o \
> +	 sgx_main.o \
> +	 sgx_page_cache.o \
> +	 sgx_util.o \
> +	 sgx_vma.o \
> diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> index 50d9af8..5f411b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> +++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> @@ -59,22 +59,12 @@
>   */
>  
>  #include "sgx.h"
> -#include <linux/acpi.h>
>  #include <linux/file.h>
>  #include <linux/highmem.h>
>  #include <linux/miscdevice.h>
> -#include <linux/module.h>
> -#include <linux/suspend.h>
>  #include <linux/hashtable.h>
>  #include <linux/kthread.h>
> -#include <linux/platform_device.h>
> -
> -#define DRV_DESCRIPTION "Intel SGX Driver"
> -#define DRV_VERSION "0.10"
> -
> -MODULE_DESCRIPTION(DRV_DESCRIPTION);
> -MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
> -MODULE_VERSION(DRV_VERSION);
> +#include <linux/syscore_ops.h>
>  
>  /*
>   * Global data.
> @@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
>  	.mode   = 0666,
>  };
>  
> -static int sgx_pm_suspend(struct device *dev)
> +static int sgx_pm_suspend(void)
>  {
>  	struct sgx_tgid_ctx *ctx;
>  	struct sgx_encl *encl;
> @@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
>  
> -static int sgx_dev_init(struct device *dev)
> +static struct syscore_ops sgx_syscore_ops = {
> +	.suspend	= sgx_pm_suspend,
> +};
> +
> +static int __init sgx_dev_init(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  	unsigned long pa;
> @@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
>  	int ret;
>  	int i;
>  
> -	pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n");
> -
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
>  		return -ENODEV;
>  
> @@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
>  		pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
>  		size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
>  
> -		dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
> +		pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
>  
>  		sgx_epc_banks[i].pa = pa;
>  		sgx_epc_banks[i].size = size;
> @@ -238,10 +229,11 @@ static int sgx_dev_init(struct device *dev)
>  	if (!sgx_add_page_wq) {
>  		pr_err("intel_sgx: alloc_workqueue() failed\n");
>  		ret = -ENOMEM;
> -		goto out_iounmap;
> +		goto out_pagecache;
>  	}
>  
> -	sgx_dev.parent = dev;
> +	register_syscore_ops(&sgx_syscore_ops);
> +
>  	ret = misc_register(&sgx_dev);
>  	if (ret) {
>  		pr_err("intel_sgx: misc_register() failed\n");
> @@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
>  
>  	return 0;
>  out_workqueue:
> +	unregister_syscore_ops(&sgx_syscore_ops);
>  	destroy_workqueue(sgx_add_page_wq);
> +out_pagecache:
> +	sgx_page_cache_teardown();
>  out_iounmap:
>  #ifdef CONFIG_X86_64
>  	for (i = 0; i < sgx_nr_epc_banks; i++)
> @@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
>  	return ret;
>  }
>  
> -static int sgx_drv_probe(struct platform_device *pdev)
> +static int __init sgx_init(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  	int i;
> @@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
>  #endif
>  	sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
>  
> -	return sgx_dev_init(&pdev->dev);
> +	return sgx_dev_init();
>  }
>  
> -static int sgx_drv_remove(struct platform_device *pdev)
> -{
> -	int i;
> -
> -	misc_deregister(&sgx_dev);
> -	destroy_workqueue(sgx_add_page_wq);
> -#ifdef CONFIG_X86_64
> -	for (i = 0; i < sgx_nr_epc_banks; i++)
> -		iounmap((void *)sgx_epc_banks[i].va);
> -#endif
> -	sgx_page_cache_teardown();
> -
> -	return 0;
> -}
> -
> -#ifdef CONFIG_ACPI
> -static struct acpi_device_id sgx_device_ids[] = {
> -	{"INT0E0C", 0},
> -	{"", 0},
> -};
> -MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
> -#endif
> -
> -static struct platform_driver sgx_drv = {
> -	.probe = sgx_drv_probe,
> -	.remove = sgx_drv_remove,
> -	.driver = {
> -		.name			= "intel_sgx",
> -		.pm			= &sgx_drv_pm,
> -		.acpi_match_table	= ACPI_PTR(sgx_device_ids),
> -	},
> -};
> -
> -module_platform_driver(sgx_drv);
> -
> -MODULE_LICENSE("Dual BSD/GPL");
> +device_initcall(sgx_init);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Sean Christopherson Aug. 31, 2017, 4:52 p.m. UTC | #4
On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > Build SGX support directly into the kernel, utilizing syscore_ops
> > instead of dev_pm_ops to hook into the suspend flow.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I think you are looking this only from container perspective. For
> desktop distribution it *does* make sense to be able to compile
> this as a module.
> 
> /Jarkko

I don't disagree that building SGX as module is valuable.  Even for
server/container/VM environments it would be nice to have the driver
built as a loadable module.

The core issue is that taking a dependency on a loadable module is
Simply not possible for a cgroup, and while possible for KVM, is not
ideal.  Various workarounds are possible, but they are either ugly,
i.e. will never be accepted upstream, or don't really solve the
problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
basically forces distros to decide between the cgroup and building
SGX as a module.

I think Kai's idea to split (some of) the EPC management and the
core functionality, e.g. programing FLC MSRs, from the userspace
facing driver is probably the right long term direction.  This would
allow the EPC cgroup and KVM to take a dependency only on the core
SGX code.  Though as Kai said, this is a fairly significant code
change.  On the plus side, this change could be done after the
initial upstreaming of the SGX driver as it wouldn't be a breaking
change, e.g. distros wouldn't need to make changes to their kernel
configs.

If you're amenable (or at least not wholly opposed) to this idea,
I'll start working on a RFC patch.  


> 
> > ---
> >  arch/x86/Kconfig                         |  4 +-
> >  arch/x86/kernel/cpu/intel_sgx/Makefile   | 13 +++---
> >  arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
> >  3 files changed, 24 insertions(+), 67 deletions(-)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index fb8c91f..5f4d414 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
> >  	  If unsure, say y.
> >  
> >  config INTEL_SGX
> > -	tristate "Intel(R) SGX Driver"
> > -	default n
> > +	prompt "Intel SGX (Secure Guard Extensions)"
> > +	def_bool n
> >  	depends on X86
> >  	select MMU_NOTIFIER
> >  	---help---
> > diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile b/arch/x86/kernel/cpu/intel_sgx/Makefile
> > index b87f4e1..71b85b5 100644
> > --- a/arch/x86/kernel/cpu/intel_sgx/Makefile
> > +++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
> > @@ -2,11 +2,8 @@
> >  # Intel SGX
> >  #
> >  
> > -obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
> > -
> > -intel_sgx-$(CONFIG_INTEL_SGX) += \
> > -	sgx_ioctl.o \
> > -	sgx_main.o \
> > -	sgx_page_cache.o \
> > -	sgx_util.o \
> > -	sgx_vma.o \
> > +obj-y += sgx_ioctl.o \
> > +	 sgx_main.o \
> > +	 sgx_page_cache.o \
> > +	 sgx_util.o \
> > +	 sgx_vma.o \
> > diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> > index 50d9af8..5f411b5 100644
> > --- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> > +++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> > @@ -59,22 +59,12 @@
> >   */
> >  
> >  #include "sgx.h"
> > -#include <linux/acpi.h>
> >  #include <linux/file.h>
> >  #include <linux/highmem.h>
> >  #include <linux/miscdevice.h>
> > -#include <linux/module.h>
> > -#include <linux/suspend.h>
> >  #include <linux/hashtable.h>
> >  #include <linux/kthread.h>
> > -#include <linux/platform_device.h>
> > -
> > -#define DRV_DESCRIPTION "Intel SGX Driver"
> > -#define DRV_VERSION "0.10"
> > -
> > -MODULE_DESCRIPTION(DRV_DESCRIPTION);
> > -MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
> > -MODULE_VERSION(DRV_VERSION);
> > +#include <linux/syscore_ops.h>
> >  
> >  /*
> >   * Global data.
> > @@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
> >  	.mode   = 0666,
> >  };
> >  
> > -static int sgx_pm_suspend(struct device *dev)
> > +static int sgx_pm_suspend(void)
> >  {
> >  	struct sgx_tgid_ctx *ctx;
> >  	struct sgx_encl *encl;
> > @@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
> >  
> > -static int sgx_dev_init(struct device *dev)
> > +static struct syscore_ops sgx_syscore_ops = {
> > +	.suspend	= sgx_pm_suspend,
> > +};
> > +
> > +static int __init sgx_dev_init(void)
> >  {
> >  	unsigned int eax, ebx, ecx, edx;
> >  	unsigned long pa;
> > @@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
> >  	int ret;
> >  	int i;
> >  
> > -	pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n");
> > -
> >  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> >  		return -ENODEV;
> >  
> > @@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
> >  		pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
> >  		size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
> >  
> > -		dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
> > +		pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
> >  
> >  		sgx_epc_banks[i].pa = pa;
> >  		sgx_epc_banks[i].size = size;
> > @@ -238,10 +229,11 @@ static int sgx_dev_init(struct device *dev)
> >  	if (!sgx_add_page_wq) {
> >  		pr_err("intel_sgx: alloc_workqueue() failed\n");
> >  		ret = -ENOMEM;
> > -		goto out_iounmap;
> > +		goto out_pagecache;
> >  	}
> >  
> > -	sgx_dev.parent = dev;
> > +	register_syscore_ops(&sgx_syscore_ops);
> > +
> >  	ret = misc_register(&sgx_dev);
> >  	if (ret) {
> >  		pr_err("intel_sgx: misc_register() failed\n");
> > @@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
> >  
> >  	return 0;
> >  out_workqueue:
> > +	unregister_syscore_ops(&sgx_syscore_ops);
> >  	destroy_workqueue(sgx_add_page_wq);
> > +out_pagecache:
> > +	sgx_page_cache_teardown();
> >  out_iounmap:
> >  #ifdef CONFIG_X86_64
> >  	for (i = 0; i < sgx_nr_epc_banks; i++)
> > @@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
> >  	return ret;
> >  }
> >  
> > -static int sgx_drv_probe(struct platform_device *pdev)
> > +static int __init sgx_init(void)
> >  {
> >  	unsigned int eax, ebx, ecx, edx;
> >  	int i;
> > @@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
> >  #endif
> >  	sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
> >  
> > -	return sgx_dev_init(&pdev->dev);
> > +	return sgx_dev_init();
> >  }
> >  
> > -static int sgx_drv_remove(struct platform_device *pdev)
> > -{
> > -	int i;
> > -
> > -	misc_deregister(&sgx_dev);
> > -	destroy_workqueue(sgx_add_page_wq);
> > -#ifdef CONFIG_X86_64
> > -	for (i = 0; i < sgx_nr_epc_banks; i++)
> > -		iounmap((void *)sgx_epc_banks[i].va);
> > -#endif
> > -	sgx_page_cache_teardown();
> > -
> > -	return 0;
> > -}
> > -
> > -#ifdef CONFIG_ACPI
> > -static struct acpi_device_id sgx_device_ids[] = {
> > -	{"INT0E0C", 0},
> > -	{"", 0},
> > -};
> > -MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
> > -#endif
> > -
> > -static struct platform_driver sgx_drv = {
> > -	.probe = sgx_drv_probe,
> > -	.remove = sgx_drv_remove,
> > -	.driver = {
> > -		.name			= "intel_sgx",
> > -		.pm			= &sgx_drv_pm,
> > -		.acpi_match_table	= ACPI_PTR(sgx_device_ids),
> > -	},
> > -};
> > -
> > -module_platform_driver(sgx_drv);
> > -
> > -MODULE_LICENSE("Dual BSD/GPL");
> > +device_initcall(sgx_init);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > intel-sgx-kernel-dev mailing list
> > intel-sgx-kernel-dev@lists.01.org
> > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
>
Jarkko Sakkinen Sept. 2, 2017, 10:28 a.m. UTC | #5
On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > instead of dev_pm_ops to hook into the suspend flow.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I think you are looking this only from container perspective. For
> > desktop distribution it *does* make sense to be able to compile
> > this as a module.
> > 
> > /Jarkko
> 
> I don't disagree that building SGX as module is valuable.  Even for
> server/container/VM environments it would be nice to have the driver
> built as a loadable module.
> 
> The core issue is that taking a dependency on a loadable module is
> Simply not possible for a cgroup, and while possible for KVM, is not
> ideal.  Various workarounds are possible, but they are either ugly,
> i.e. will never be accepted upstream, or don't really solve the
> problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> basically forces distros to decide between the cgroup and building
> SGX as a module.

IMA depends on the TPM driver and requires it to be linked directly to
the kernel if it is enabled i.e. use 'y'. It's a much better solution
and there are already existing examples of this in the mainline kernel.

/Jarkko
Sean Christopherson Sept. 5, 2017, 1:42 p.m. UTC | #6
On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > instead of dev_pm_ops to hook into the suspend flow.
> > > >
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > >
> > > I think you are looking this only from container perspective. For
> > > desktop distribution it *does* make sense to be able to compile
> > > this as a module.
> > >
> > > /Jarkko
> >
> > I don't disagree that building SGX as module is valuable.  Even for
> > server/container/VM environments it would be nice to have the driver
> > built as a loadable module.
> >
> > The core issue is that taking a dependency on a loadable module is
> > Simply not possible for a cgroup, and while possible for KVM, is not
> > ideal.  Various workarounds are possible, but they are either ugly,
> > i.e. will never be accepted upstream, or don't really solve the
> > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > basically forces distros to decide between the cgroup and building
> > SGX as a module.
>
> IMA depends on the TPM driver and requires it to be linked directly to
> the kernel if it is enabled i.e. use 'y'. It's a much better solution
> and there are already existing examples of this in the mainline kernel.

It's a simpler solution, but I think taking that route will result in all
distros compiling the SGX driver as a built-in, which defeats the purpose
of allowing the SGX driver to be a loadable module.
Jarkko Sakkinen Sept. 5, 2017, 7:14 p.m. UTC | #7
On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
> On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > >
> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > >
> > > > I think you are looking this only from container perspective. For
> > > > desktop distribution it *does* make sense to be able to compile
> > > > this as a module.
> > > >
> > > > /Jarkko
> > >
> > > I don't disagree that building SGX as module is valuable.  Even for
> > > server/container/VM environments it would be nice to have the driver
> > > built as a loadable module.
> > >
> > > The core issue is that taking a dependency on a loadable module is
> > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > ideal.  Various workarounds are possible, but they are either ugly,
> > > i.e. will never be accepted upstream, or don't really solve the
> > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > basically forces distros to decide between the cgroup and building
> > > SGX as a module.
> >
> > IMA depends on the TPM driver and requires it to be linked directly to
> > the kernel if it is enabled i.e. use 'y'. It's a much better solution
> > and there are already existing examples of this in the mainline kernel.
> 
> It's a simpler solution, but I think taking that route will result in all
> distros compiling the SGX driver as a built-in, which defeats the purpose
> of allowing the SGX driver to be a loadable module.

So happens with TPM driver for most of the distros. It still gives
some flexibility and does not cause much harm.

It will also easier for me to maintain SGX tree, collect patches and
send pull requests for upper layer maintainers for future kernel
releases.

Sprinkling properly encapsulated code is really an antipattern.

/Jarkko
Jarkko Sakkinen Sept. 5, 2017, 7:28 p.m. UTC | #8
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
> > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > >
> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > >
> > > > > I think you are looking this only from container perspective. For
> > > > > desktop distribution it *does* make sense to be able to compile
> > > > > this as a module.
> > > > >
> > > > > /Jarkko
> > > >
> > > > I don't disagree that building SGX as module is valuable.  Even for
> > > > server/container/VM environments it would be nice to have the driver
> > > > built as a loadable module.
> > > >
> > > > The core issue is that taking a dependency on a loadable module is
> > > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > > ideal.  Various workarounds are possible, but they are either ugly,
> > > > i.e. will never be accepted upstream, or don't really solve the
> > > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > > basically forces distros to decide between the cgroup and building
> > > > SGX as a module.
> > >
> > > IMA depends on the TPM driver and requires it to be linked directly to
> > > the kernel if it is enabled i.e. use 'y'. It's a much better solution
> > > and there are already existing examples of this in the mainline kernel.
> > 
> > It's a simpler solution, but I think taking that route will result in all
> > distros compiling the SGX driver as a built-in, which defeats the purpose
> > of allowing the SGX driver to be a loadable module.
> 
> So happens with TPM driver for most of the distros. It still gives
> some flexibility and does not cause much harm.
> 
> It will also easier for me to maintain SGX tree, collect patches and
> send pull requests for upper layer maintainers for future kernel
> releases.
> 
> Sprinkling properly encapsulated code is really an antipattern.
> 
> /Jarkko

There's also things when you move into maintainer mode like backporting
bug fixes for stable kernels and stuff like this. Distro maintainers and
Greg K-H will probably like the current organization more...

/Jarkko
Kai Huang Sept. 5, 2017, 11:18 p.m. UTC | #9
On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > wrote:
> > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > J wrote:
> > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > Christopherson wrote:
> > > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > > syscore_ops
> > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > > 
> > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson
> > > > > > > @intel.com>
> > > > > > 
> > > > > > I think you are looking this only from container
> > > > > > perspective. For
> > > > > > desktop distribution it *does* make sense to be able to
> > > > > > compile
> > > > > > this as a module.
> > > > > > 
> > > > > > /Jarkko
> > > > > 
> > > > > I don't disagree that building SGX as module is
> > > > > valuable.  Even for
> > > > > server/container/VM environments it would be nice to have the
> > > > > driver
> > > > > built as a loadable module.
> > > > > 
> > > > > The core issue is that taking a dependency on a loadable
> > > > > module is
> > > > > Simply not possible for a cgroup, and while possible for KVM,
> > > > > is not
> > > > > ideal.  Various workarounds are possible, but they are either
> > > > > ugly,
> > > > > i.e. will never be accepted upstream, or don't really solve
> > > > > the
> > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > cgroup
> > > > > basically forces distros to decide between the cgroup and
> > > > > building
> > > > > SGX as a module.
> > > > 
> > > > IMA depends on the TPM driver and requires it to be linked
> > > > directly to
> > > > the kernel if it is enabled i.e. use 'y'. It's a much better
> > > > solution
> > > > and there are already existing examples of this in the mainline
> > > > kernel.
> > > 
> > > It's a simpler solution, but I think taking that route will
> > > result in all
> > > distros compiling the SGX driver as a built-in, which defeats the
> > > purpose
> > > of allowing the SGX driver to be a loadable module.
> > 
> > So happens with TPM driver for most of the distros. It still gives
> > some flexibility and does not cause much harm.
> > 
> > It will also easier for me to maintain SGX tree, collect patches
> > and
> > send pull requests for upper layer maintainers for future kernel
> > releases.
> > 
> > Sprinkling properly encapsulated code is really an antipattern.
> > 
> > /Jarkko
> 
> There's also things when you move into maintainer mode like
> backporting
> bug fixes for stable kernels and stuff like this. Distro maintainers
> and
> Greg K-H will probably like the current organization more...

Hi Jarkko,

From maintain's view, I think there's already plenty examples that one
maintainer maintains files under several folders. For example, KVM
maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
there any particular difficulty if part of SGX code is under, ex,
arch/x86/kernel/cpu/ ?

I am not sure about TPM but for SGX, but KVM actually only needs to
depend on part of your SGX code. Moving some core SGX functions to
arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
meantime, provides more flexibility (ie, still allow SGX driver to be a
loadable module).

Thanks,
-Kai

> 
> /Jarkko
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Sean Christopherson Sept. 6, 2017, 3:32 p.m. UTC | #10
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
> > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > >
> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > >
> > > > > I think you are looking this only from container perspective. For
> > > > > desktop distribution it *does* make sense to be able to compile
> > > > > this as a module.
> > > > >
> > > > > /Jarkko
> > > >
> > > > I don't disagree that building SGX as module is valuable.  Even for
> > > > server/container/VM environments it would be nice to have the driver
> > > > built as a loadable module.
> > > >
> > > > The core issue is that taking a dependency on a loadable module is
> > > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > > ideal.  Various workarounds are possible, but they are either ugly,
> > > > i.e. will never be accepted upstream, or don't really solve the
> > > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > > basically forces distros to decide between the cgroup and building
> > > > SGX as a module.
> > >
> > > IMA depends on the TPM driver and requires it to be linked directly to
> > > the kernel if it is enabled i.e. use 'y'. It's a much better solution
> > > and there are already existing examples of this in the mainline kernel.
> >
> > It's a simpler solution, but I think taking that route will result in all
> > distros compiling the SGX driver as a built-in, which defeats the purpose
> > of allowing the SGX driver to be a loadable module.
>
> So happens with TPM driver for most of the distros. It still gives
> some flexibility and does not cause much harm.
>
> It will also easier for me to maintain SGX tree, collect patches and
> send pull requests for upper layer maintainers for future kernel
> releases.
>
> Sprinkling properly encapsulated code is really an antipattern.

Once more SGX features are added I don't think it will be possible to fully
encapsulate the SGX driver as it stands today, e.g. VMM EPC oversubscription
needs different flows for swapping pages to/from the EPC.  Splitting out the
high-level EPC control code to a separate built-in module allows the native
driver and KVM to coexist without having to make invasive changes or break
encapsulation of the native driver.  It also gives the EPC cgroup a single
touchpoint without the native SGX driver having to be aware of KVM.
Sean Christopherson Sept. 6, 2017, 3:41 p.m. UTC | #11
On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
> On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > > wrote:
> > > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > > J wrote:
> > > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > > wrote:
> > > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > > Christopherson wrote:
> > > > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > > > syscore_ops
> > > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > > >
> > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson
> > > > > > > > @intel.com>
> > > > > > >
> > > > > > > I think you are looking this only from container
> > > > > > > perspective. For
> > > > > > > desktop distribution it *does* make sense to be able to
> > > > > > > compile
> > > > > > > this as a module.
> > > > > > >
> > > > > > > /Jarkko
> > > > > >
> > > > > > I don't disagree that building SGX as module is
> > > > > > valuable.  Even for
> > > > > > server/container/VM environments it would be nice to have the
> > > > > > driver
> > > > > > built as a loadable module.
> > > > > >
> > > > > > The core issue is that taking a dependency on a loadable
> > > > > > module is simply not possible for a cgroup, and while possible for KVM,
> > > > > > is not
> > > > > > ideal.  Various workarounds are possible, but they are either
> > > > > > ugly,
> > > > > > i.e. will never be accepted upstream, or don't really solve
> > > > > > the
> > > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > > cgroup
> > > > > > basically forces distros to decide between the cgroup and
> > > > > > building
> > > > > > SGX as a module.
> > > > >
> > > > > IMA depends on the TPM driver and requires it to be linked
> > > > > directly to
> > > > > the kernel if it is enabled i.e. use 'y'. It's a much better
> > > > > solution
> > > > > and there are already existing examples of this in the mainline
> > > > > kernel.
> > > >
> > > > It's a simpler solution, but I think taking that route will
> > > > result in all
> > > > distros compiling the SGX driver as a built-in, which defeats the
> > > > purpose
> > > > of allowing the SGX driver to be a loadable module.
> > >
> > > So happens with TPM driver for most of the distros. It still gives
> > > some flexibility and does not cause much harm.
> > >
> > > It will also easier for me to maintain SGX tree, collect patches
> > > and
> > > send pull requests for upper layer maintainers for future kernel
> > > releases.
> > >
> > > Sprinkling properly encapsulated code is really an antipattern.
> > >
> > > /Jarkko
> >
> > There's also things when you move into maintainer mode like
> > backporting
> > bug fixes for stable kernels and stuff like this. Distro maintainers
> > and
> > Greg K-H will probably like the current organization more...
>
> Hi Jarkko,
>
> From maintain's view, I think there's already plenty examples that one
> maintainer maintains files under several folders. For example, KVM
> maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
> there any particular difficulty if part of SGX code is under, ex,
> arch/x86/kernel/cpu/ ?
>
> I am not sure about TPM but for SGX, but KVM actually only needs to
> depend on part of your SGX code. Moving some core SGX functions to
> arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
> meantime, provides more flexibility (ie, still allow SGX driver to be a
> loadable module).

Removing the dependency on the SGX driver will also allow KVM to virtualize
SGX without SGX being exposed to the native userspace.  I think we need to
support this usage model it provides the most robust environment for static
EPC partitioning for VMs, i.e. the VMM doesn't need to take additional steps
to ensure native userspace enclaves don't steal EPC pages.
Jarkko Sakkinen Sept. 7, 2017, 4:15 p.m. UTC | #12
On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
> On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > > wrote:
> > > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > > J wrote:
> > > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > > wrote:
> > > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > > Christopherson wrote:
> > > > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > > > syscore_ops
> > > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > > > 
> > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson
> > > > > > > > @intel.com>
> > > > > > > 
> > > > > > > I think you are looking this only from container
> > > > > > > perspective. For
> > > > > > > desktop distribution it *does* make sense to be able to
> > > > > > > compile
> > > > > > > this as a module.
> > > > > > > 
> > > > > > > /Jarkko
> > > > > > 
> > > > > > I don't disagree that building SGX as module is
> > > > > > valuable.  Even for
> > > > > > server/container/VM environments it would be nice to have the
> > > > > > driver
> > > > > > built as a loadable module.
> > > > > > 
> > > > > > The core issue is that taking a dependency on a loadable
> > > > > > module is
> > > > > > Simply not possible for a cgroup, and while possible for KVM,
> > > > > > is not
> > > > > > ideal.  Various workarounds are possible, but they are either
> > > > > > ugly,
> > > > > > i.e. will never be accepted upstream, or don't really solve
> > > > > > the
> > > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > > cgroup
> > > > > > basically forces distros to decide between the cgroup and
> > > > > > building
> > > > > > SGX as a module.
> > > > > 
> > > > > IMA depends on the TPM driver and requires it to be linked
> > > > > directly to
> > > > > the kernel if it is enabled i.e. use 'y'. It's a much better
> > > > > solution
> > > > > and there are already existing examples of this in the mainline
> > > > > kernel.
> > > > 
> > > > It's a simpler solution, but I think taking that route will
> > > > result in all
> > > > distros compiling the SGX driver as a built-in, which defeats the
> > > > purpose
> > > > of allowing the SGX driver to be a loadable module.
> > > 
> > > So happens with TPM driver for most of the distros. It still gives
> > > some flexibility and does not cause much harm.
> > > 
> > > It will also easier for me to maintain SGX tree, collect patches
> > > and
> > > send pull requests for upper layer maintainers for future kernel
> > > releases.
> > > 
> > > Sprinkling properly encapsulated code is really an antipattern.
> > > 
> > > /Jarkko
> > 
> > There's also things when you move into maintainer mode like
> > backporting
> > bug fixes for stable kernels and stuff like this. Distro maintainers
> > and
> > Greg K-H will probably like the current organization more...
> 
> Hi Jarkko,
> 
> From maintain's view, I think there's already plenty examples that one
> maintainer maintains files under several folders. For example, KVM
> maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
> there any particular difficulty if part of SGX code is under, ex,
> arch/x86/kernel/cpu/ ?

There must have been a good reason to do this for kvm. There's not
reason to complicate thing for SGX.

> I am not sure about TPM but for SGX, but KVM actually only needs to
> depend on part of your SGX code. Moving some core SGX functions to
> arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
> meantime, provides more flexibility (ie, still allow SGX driver to be a
> loadable module).
> 
> Thanks,
> -Kai

Still don't understand why you can't just require to link SGX to vmlinux
when needed.

/Jarkko
Jarkko Sakkinen Sept. 7, 2017, 4:17 p.m. UTC | #13
On Wed, Sep 06, 2017 at 03:32:27PM +0000, Christopherson, Sean J wrote:
> On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
> > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > >
> > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > > >
> > > > > > I think you are looking this only from container perspective. For
> > > > > > desktop distribution it *does* make sense to be able to compile
> > > > > > this as a module.
> > > > > >
> > > > > > /Jarkko
> > > > >
> > > > > I don't disagree that building SGX as module is valuable.  Even for
> > > > > server/container/VM environments it would be nice to have the driver
> > > > > built as a loadable module.
> > > > >
> > > > > The core issue is that taking a dependency on a loadable module is
> > > > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > > > ideal.  Various workarounds are possible, but they are either ugly,
> > > > > i.e. will never be accepted upstream, or don't really solve the
> > > > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > > > basically forces distros to decide between the cgroup and building
> > > > > SGX as a module.
> > > >
> > > > IMA depends on the TPM driver and requires it to be linked directly to
> > > > the kernel if it is enabled i.e. use 'y'. It's a much better solution
> > > > and there are already existing examples of this in the mainline kernel.
> > >
> > > It's a simpler solution, but I think taking that route will result in all
> > > distros compiling the SGX driver as a built-in, which defeats the purpose
> > > of allowing the SGX driver to be a loadable module.
> >
> > So happens with TPM driver for most of the distros. It still gives
> > some flexibility and does not cause much harm.
> >
> > It will also easier for me to maintain SGX tree, collect patches and
> > send pull requests for upper layer maintainers for future kernel
> > releases.
> >
> > Sprinkling properly encapsulated code is really an antipattern.
> 
> Once more SGX features are added I don't think it will be possible to fully
> encapsulate the SGX driver as it stands today, e.g. VMM EPC oversubscription
> needs different flows for swapping pages to/from the EPC.  Splitting out the
> high-level EPC control code to a separate built-in module allows the native
> driver and KVM to coexist without having to make invasive changes or break
> encapsulation of the native driver.  It also gives the EPC cgroup a single
> touchpoint without the native SGX driver having to be aware of KVM.

Right now there is no need to complicate the architecture. You can
require SGX to be linked to vmlinux.

/Jarkko
Jarkko Sakkinen Sept. 7, 2017, 4:23 p.m. UTC | #14
On Wed, Sep 06, 2017 at 03:41:24PM +0000, Christopherson, Sean J wrote:
> On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
> > On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > > > wrote:
> > > > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > > > J wrote:
> > > > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > > > wrote:
> > > > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > > > Christopherson wrote:
> > > > > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > > > > syscore_ops
> > > > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson
> > > > > > > > > @intel.com>
> > > > > > > >
> > > > > > > > I think you are looking this only from container
> > > > > > > > perspective. For
> > > > > > > > desktop distribution it *does* make sense to be able to
> > > > > > > > compile
> > > > > > > > this as a module.
> > > > > > > >
> > > > > > > > /Jarkko
> > > > > > >
> > > > > > > I don't disagree that building SGX as module is
> > > > > > > valuable.  Even for
> > > > > > > server/container/VM environments it would be nice to have the
> > > > > > > driver
> > > > > > > built as a loadable module.
> > > > > > >
> > > > > > > The core issue is that taking a dependency on a loadable
> > > > > > > module is simply not possible for a cgroup, and while possible for KVM,
> > > > > > > is not
> > > > > > > ideal.  Various workarounds are possible, but they are either
> > > > > > > ugly,
> > > > > > > i.e. will never be accepted upstream, or don't really solve
> > > > > > > the
> > > > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > > > cgroup
> > > > > > > basically forces distros to decide between the cgroup and
> > > > > > > building
> > > > > > > SGX as a module.
> > > > > >
> > > > > > IMA depends on the TPM driver and requires it to be linked
> > > > > > directly to
> > > > > > the kernel if it is enabled i.e. use 'y'. It's a much better
> > > > > > solution
> > > > > > and there are already existing examples of this in the mainline
> > > > > > kernel.
> > > > >
> > > > > It's a simpler solution, but I think taking that route will
> > > > > result in all
> > > > > distros compiling the SGX driver as a built-in, which defeats the
> > > > > purpose
> > > > > of allowing the SGX driver to be a loadable module.
> > > >
> > > > So happens with TPM driver for most of the distros. It still gives
> > > > some flexibility and does not cause much harm.
> > > >
> > > > It will also easier for me to maintain SGX tree, collect patches
> > > > and
> > > > send pull requests for upper layer maintainers for future kernel
> > > > releases.
> > > >
> > > > Sprinkling properly encapsulated code is really an antipattern.
> > > >
> > > > /Jarkko
> > >
> > > There's also things when you move into maintainer mode like
> > > backporting
> > > bug fixes for stable kernels and stuff like this. Distro maintainers
> > > and
> > > Greg K-H will probably like the current organization more...
> >
> > Hi Jarkko,
> >
> > From maintain's view, I think there's already plenty examples that one
> > maintainer maintains files under several folders. For example, KVM
> > maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
> > there any particular difficulty if part of SGX code is under, ex,
> > arch/x86/kernel/cpu/ ?
> >
> > I am not sure about TPM but for SGX, but KVM actually only needs to
> > depend on part of your SGX code. Moving some core SGX functions to
> > arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
> > meantime, provides more flexibility (ie, still allow SGX driver to be a
> > loadable module).
> 
> Removing the dependency on the SGX driver will also allow KVM to virtualize
> SGX without SGX being exposed to the native userspace.  I think we need to
> support this usage model it provides the most robust environment for static
> EPC partitioning for VMs, i.e. the VMM doesn't need to take additional steps
> to ensure native userspace enclaves don't steal EPC pages.

Not sure if it makes sense to complicate things for things that do not
exist yet... I'll ignore this discussion up until there's an upstream
driver.

/Jarkko

Patch
diff mbox

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fb8c91f..5f4d414 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1788,8 +1788,8 @@  config X86_INTEL_MEMORY_PROTECTION_KEYS
 	  If unsure, say y.
 
 config INTEL_SGX
-	tristate "Intel(R) SGX Driver"
-	default n
+	prompt "Intel SGX (Secure Guard Extensions)"
+	def_bool n
 	depends on X86
 	select MMU_NOTIFIER
 	---help---
diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile b/arch/x86/kernel/cpu/intel_sgx/Makefile
index b87f4e1..71b85b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/Makefile
+++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
@@ -2,11 +2,8 @@ 
 # Intel SGX
 #
 
-obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
-
-intel_sgx-$(CONFIG_INTEL_SGX) += \
-	sgx_ioctl.o \
-	sgx_main.o \
-	sgx_page_cache.o \
-	sgx_util.o \
-	sgx_vma.o \
+obj-y += sgx_ioctl.o \
+	 sgx_main.o \
+	 sgx_page_cache.o \
+	 sgx_util.o \
+	 sgx_vma.o \
diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
index 50d9af8..5f411b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
+++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
@@ -59,22 +59,12 @@ 
  */
 
 #include "sgx.h"
-#include <linux/acpi.h>
 #include <linux/file.h>
 #include <linux/highmem.h>
 #include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/suspend.h>
 #include <linux/hashtable.h>
 #include <linux/kthread.h>
-#include <linux/platform_device.h>
-
-#define DRV_DESCRIPTION "Intel SGX Driver"
-#define DRV_VERSION "0.10"
-
-MODULE_DESCRIPTION(DRV_DESCRIPTION);
-MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
-MODULE_VERSION(DRV_VERSION);
+#include <linux/syscore_ops.h>
 
 /*
  * Global data.
@@ -159,7 +149,7 @@  static struct miscdevice sgx_dev = {
 	.mode   = 0666,
 };
 
-static int sgx_pm_suspend(struct device *dev)
+static int sgx_pm_suspend(void)
 {
 	struct sgx_tgid_ctx *ctx;
 	struct sgx_encl *encl;
@@ -175,9 +165,12 @@  static int sgx_pm_suspend(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
 
-static int sgx_dev_init(struct device *dev)
+static struct syscore_ops sgx_syscore_ops = {
+	.suspend	= sgx_pm_suspend,
+};
+
+static int __init sgx_dev_init(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 	unsigned long pa;
@@ -186,8 +179,6 @@  static int sgx_dev_init(struct device *dev)
 	int ret;
 	int i;
 
-	pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n");
-
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
 		return -ENODEV;
 
@@ -199,7 +190,7 @@  static int sgx_dev_init(struct device *dev)
 		pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
 		size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
 
-		dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
+		pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
 
 		sgx_epc_banks[i].pa = pa;
 		sgx_epc_banks[i].size = size;
@@ -238,10 +229,11 @@  static int sgx_dev_init(struct device *dev)
 	if (!sgx_add_page_wq) {
 		pr_err("intel_sgx: alloc_workqueue() failed\n");
 		ret = -ENOMEM;
-		goto out_iounmap;
+		goto out_pagecache;
 	}
 
-	sgx_dev.parent = dev;
+	register_syscore_ops(&sgx_syscore_ops);
+
 	ret = misc_register(&sgx_dev);
 	if (ret) {
 		pr_err("intel_sgx: misc_register() failed\n");
@@ -250,7 +242,10 @@  static int sgx_dev_init(struct device *dev)
 
 	return 0;
 out_workqueue:
+	unregister_syscore_ops(&sgx_syscore_ops);
 	destroy_workqueue(sgx_add_page_wq);
+out_pagecache:
+	sgx_page_cache_teardown();
 out_iounmap:
 #ifdef CONFIG_X86_64
 	for (i = 0; i < sgx_nr_epc_banks; i++)
@@ -259,7 +254,7 @@  static int sgx_dev_init(struct device *dev)
 	return ret;
 }
 
-static int sgx_drv_probe(struct platform_device *pdev)
+static int __init sgx_init(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 	int i;
@@ -307,42 +302,7 @@  static int sgx_drv_probe(struct platform_device *pdev)
 #endif
 	sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
 
-	return sgx_dev_init(&pdev->dev);
+	return sgx_dev_init();
 }
 
-static int sgx_drv_remove(struct platform_device *pdev)
-{
-	int i;
-
-	misc_deregister(&sgx_dev);
-	destroy_workqueue(sgx_add_page_wq);
-#ifdef CONFIG_X86_64
-	for (i = 0; i < sgx_nr_epc_banks; i++)
-		iounmap((void *)sgx_epc_banks[i].va);
-#endif
-	sgx_page_cache_teardown();
-
-	return 0;
-}
-
-#ifdef CONFIG_ACPI
-static struct acpi_device_id sgx_device_ids[] = {
-	{"INT0E0C", 0},
-	{"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
-#endif
-
-static struct platform_driver sgx_drv = {
-	.probe = sgx_drv_probe,
-	.remove = sgx_drv_remove,
-	.driver = {
-		.name			= "intel_sgx",
-		.pm			= &sgx_drv_pm,
-		.acpi_match_table	= ACPI_PTR(sgx_device_ids),
-	},
-};
-
-module_platform_driver(sgx_drv);
-
-MODULE_LICENSE("Dual BSD/GPL");
+device_initcall(sgx_init);