| Message ID | 1504029396-3353-3-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 >
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
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.
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
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
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
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.
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.
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
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
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
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);
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(-)