It's hard to add new content to this function because it is time
consuming to match fallback and its cause. Rename labels in a way
that the name of error label refers to the site where failure
happened. This way it is easier to keep on track what is going
on.
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v5:
* A new patch.
---
arch/x86/kernel/cpu/sgx/main.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 231c494dfd40..b8e1703eecae 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -914,12 +914,12 @@ static int __init sgx_init(void)
if (!sgx_page_reclaimer_init()) {
ret = -ENOMEM;
- goto err_page_cache;
+ goto err_reclaimer;
}
ret = misc_register(&sgx_dev_provision);
if (ret)
- goto err_kthread;
+ goto err_provision;
/*
* Always try to initialize the native *and* KVM drivers.
@@ -932,17 +932,17 @@ static int __init sgx_init(void)
ret = sgx_drv_init();
if (sgx_vepc_init() && ret)
- goto err_provision;
+ goto err_driver;
return 0;
-err_provision:
+err_driver:
misc_deregister(&sgx_dev_provision);
-err_kthread:
+err_provision:
kthread_stop(ksgxd_tsk);
-err_page_cache:
+err_reclaimer:
for (i = 0; i < sgx_nr_epc_sections; i++) {
vfree(sgx_epc_sections[i].pages);
memunmap(sgx_epc_sections[i].virt_addr);
--
2.32.0
The amount of SGX memory on the system is determined by the BIOS and it
varies wildly between systems. It can be from dozens of MB's on desktops
or VM's, up to many GB's on servers. Just like for regular memory, it is
sometimes useful to know the amount of usable SGX memory in the system.
Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
specific attribute group, and add an attribute for the amount of SGX
memory in bytes to each NUMA node:
/sys/devices/system/node/nodeX/x86/sgx_total_bytes
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v13:
* Rebased on top of tip/x86/sgx.
v12:
* Device ID is set to same as NUMA node ID. Use that to get the correct
NUMA node ID in sgx_total_bytes_show().
v11:
* Fix documentation and the commit message.
v10:
* Change DEVICE_ATTR_RO() to static (Greg K-H)
* Change the attribute name as sgx_total_bytes, and attribute group
name as "x86" (Dave).
* Add a new config flag HAVE_ARCH_NODE_DEV_GROUP to identify, whether
an arch exports arch specific attribute group for NUMA nodes.
v9:
* Fix racy initialization of sysfs attributes:
https://lore.kernel.org/linux-sgx/[email protected]/
v8:
* Fix a bug in sgx_numa_init(): node->dev should be only set after
sysfe_create_group(). Otherwise, sysfs_remove_group() will issue a
warning in sgx_numa_exit(), when sgx_create_group() is unsuccessful,
because the group does not exist.
v7:
* Shorten memory_size to size. The prefix makes the name only longer
but does not clarify things more than "size" would.
* Use device_attribute instead of kobj_attribute.
* Use named attribute group instead of creating raw kobject just for
the "sgx" subdirectory.
v6:
* Initialize node->size to zero in sgx_setup_epc_section(), when the
node is first accessed.
v5
* A new patch based on the discussion on
https://lore.kernel.org/linux-sgx/[email protected]/T/#t
---
Documentation/ABI/stable/sysfs-devices-node | 6 ++++++
arch/Kconfig | 4 ++++
arch/x86/Kconfig | 1 +
arch/x86/kernel/cpu/sgx/main.c | 20 ++++++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 1 +
drivers/base/node.c | 3 +++
include/linux/numa.h | 4 ++++
7 files changed, 39 insertions(+)
diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
index 484fc04bcc25..8db67aa472f1 100644
--- a/Documentation/ABI/stable/sysfs-devices-node
+++ b/Documentation/ABI/stable/sysfs-devices-node
@@ -176,3 +176,9 @@ Contact: Keith Busch <[email protected]>
Description:
The cache write policy: 0 for write-back, 1 for write-through,
other or unknown.
+
+What: /sys/devices/system/node/nodeX/x86/sgx_total_bytes
+Date: November 2021
+Contact: Jarkko Sakkinen <[email protected]>
+Description:
+ The total amount of SGX physical memory in bytes.
diff --git a/arch/Kconfig b/arch/Kconfig
index 26b8ed11639d..0a9dadb00b61 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1302,6 +1302,10 @@ config ARCH_HAS_PARANOID_L1D_FLUSH
config DYNAMIC_SIGFRAME
bool
+# Select, if arch has a named attribute group bound to NUMA device nodes.
+config HAVE_ARCH_NODE_DEV_GROUP
+ bool
+
source "kernel/gcov/Kconfig"
source "scripts/gcc-plugins/Kconfig"
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b9281fab4e3e..f2b699d12eb8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -269,6 +269,7 @@ config X86
select HAVE_ARCH_KCSAN if X86_64
select X86_FEATURE_NAMES if PROC_FS
select PROC_PID_ARCH_STATUS if PROC_FS
+ select HAVE_ARCH_NODE_DEV_GROUP if X86_SGX
imply IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
config INSTRUCTION_DECODER
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index b8e1703eecae..00af6f90d31a 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -825,9 +825,11 @@ static bool __init sgx_page_cache_init(void)
INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_page_list);
INIT_LIST_HEAD(&sgx_numa_nodes[nid].sgx_poison_page_list);
node_set(nid, sgx_numa_mask);
+ sgx_numa_nodes[nid].size = 0;
}
sgx_epc_sections[i].node = &sgx_numa_nodes[nid];
+ sgx_numa_nodes[nid].size += size;
sgx_nr_epc_sections++;
}
@@ -901,6 +903,24 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
}
EXPORT_SYMBOL_GPL(sgx_set_attribute);
+#ifdef CONFIG_NUMA
+static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
+}
+static DEVICE_ATTR_RO(sgx_total_bytes);
+
+static struct attribute *arch_node_dev_attrs[] = {
+ &dev_attr_sgx_total_bytes.attr,
+ NULL,
+};
+
+const struct attribute_group arch_node_dev_group = {
+ .name = "x86",
+ .attrs = arch_node_dev_attrs,
+};
+#endif /* CONFIG_NUMA */
+
static int __init sgx_init(void)
{
int ret;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 9ec3136c7800..0f17def9fe6f 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -44,6 +44,7 @@ struct sgx_epc_page {
struct sgx_numa_node {
struct list_head free_page_list;
struct list_head sgx_poison_page_list;
+ unsigned long size;
spinlock_t lock;
};
diff --git a/drivers/base/node.c b/drivers/base/node.c
index b5a4ba18f9f9..87acc47e8951 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -581,6 +581,9 @@ static const struct attribute_group node_dev_group = {
static const struct attribute_group *node_dev_groups[] = {
&node_dev_group,
+#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
+ &arch_node_dev_group,
+#endif
NULL
};
diff --git a/include/linux/numa.h b/include/linux/numa.h
index cb44cfe2b725..59df211d051f 100644
--- a/include/linux/numa.h
+++ b/include/linux/numa.h
@@ -58,4 +58,8 @@ static inline int phys_to_target_node(u64 start)
}
#endif
+#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
+extern const struct attribute_group arch_node_dev_group;
+#endif
+
#endif /* _LINUX_NUMA_H */
--
2.32.0
On Tue, Nov 16, 2021 at 06:21:16PM +0200, Jarkko Sakkinen wrote:
> The amount of SGX memory on the system is determined by the BIOS and it
> varies wildly between systems. It can be from dozens of MB's on desktops
> or VM's, up to many GB's on servers. Just like for regular memory, it is
> sometimes useful to know the amount of usable SGX memory in the system.
>
> Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> specific attribute group, and add an attribute for the amount of SGX
> memory in bytes to each NUMA node:
>
> /sys/devices/system/node/nodeX/x86/sgx_total_bytes
>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> v13:
> * Rebased on top of tip/x86/sgx.
>
> v12:
> * Device ID is set to same as NUMA node ID. Use that to get the correct
> NUMA node ID in sgx_total_bytes_show().
>
> v11:
> * Fix documentation and the commit message.
>
> v10:
> * Change DEVICE_ATTR_RO() to static (Greg K-H)
> * Change the attribute name as sgx_total_bytes, and attribute group
> name as "x86" (Dave).
> * Add a new config flag HAVE_ARCH_NODE_DEV_GROUP to identify, whether
> an arch exports arch specific attribute group for NUMA nodes.
>
> v9:
> * Fix racy initialization of sysfs attributes:
> https://lore.kernel.org/linux-sgx/[email protected]/
>
> v8:
> * Fix a bug in sgx_numa_init(): node->dev should be only set after
> sysfe_create_group(). Otherwise, sysfs_remove_group() will issue a
> warning in sgx_numa_exit(), when sgx_create_group() is unsuccessful,
> because the group does not exist.
>
> v7:
> * Shorten memory_size to size. The prefix makes the name only longer
> but does not clarify things more than "size" would.
> * Use device_attribute instead of kobj_attribute.
> * Use named attribute group instead of creating raw kobject just for
> the "sgx" subdirectory.
>
> v6:
> * Initialize node->size to zero in sgx_setup_epc_section(), when the
> node is first accessed.
>
> v5
> * A new patch based on the discussion on
> https://lore.kernel.org/linux-sgx/[email protected]/T/#t
> ---
> Documentation/ABI/stable/sysfs-devices-node | 6 ++++++
> arch/Kconfig | 4 ++++
> arch/x86/Kconfig | 1 +
> arch/x86/kernel/cpu/sgx/main.c | 20 ++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/sgx.h | 1 +
> drivers/base/node.c | 3 +++
> include/linux/numa.h | 4 ++++
> 7 files changed, 39 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index 484fc04bcc25..8db67aa472f1 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -176,3 +176,9 @@ Contact: Keith Busch <[email protected]>
> Description:
> The cache write policy: 0 for write-back, 1 for write-through,
> other or unknown.
> +
> +What: /sys/devices/system/node/nodeX/x86/sgx_total_bytes
> +Date: November 2021
> +Contact: Jarkko Sakkinen <[email protected]>
> +Description:
> + The total amount of SGX physical memory in bytes.
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 26b8ed11639d..0a9dadb00b61 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1302,6 +1302,10 @@ config ARCH_HAS_PARANOID_L1D_FLUSH
> config DYNAMIC_SIGFRAME
> bool
>
> +# Select, if arch has a named attribute group bound to NUMA device nodes.
> +config HAVE_ARCH_NODE_DEV_GROUP
> + bool
> +
> source "kernel/gcov/Kconfig"
>
> source "scripts/gcc-plugins/Kconfig"
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b9281fab4e3e..f2b699d12eb8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -269,6 +269,7 @@ config X86
> select HAVE_ARCH_KCSAN if X86_64
> select X86_FEATURE_NAMES if PROC_FS
> select PROC_PID_ARCH_STATUS if PROC_FS
> + select HAVE_ARCH_NODE_DEV_GROUP if X86_SGX
> imply IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
>
> config INSTRUCTION_DECODER
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index b8e1703eecae..00af6f90d31a 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -825,9 +825,11 @@ static bool __init sgx_page_cache_init(void)
> INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_page_list);
> INIT_LIST_HEAD(&sgx_numa_nodes[nid].sgx_poison_page_list);
> node_set(nid, sgx_numa_mask);
> + sgx_numa_nodes[nid].size = 0;
> }
>
> sgx_epc_sections[i].node = &sgx_numa_nodes[nid];
> + sgx_numa_nodes[nid].size += size;
>
> sgx_nr_epc_sections++;
> }
> @@ -901,6 +903,24 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
> }
> EXPORT_SYMBOL_GPL(sgx_set_attribute);
>
> +#ifdef CONFIG_NUMA
> +static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> +}
> +static DEVICE_ATTR_RO(sgx_total_bytes);
> +
> +static struct attribute *arch_node_dev_attrs[] = {
> + &dev_attr_sgx_total_bytes.attr,
> + NULL,
> +};
> +
> +const struct attribute_group arch_node_dev_group = {
> + .name = "x86",
> + .attrs = arch_node_dev_attrs,
> +};
> +#endif /* CONFIG_NUMA */
> +
> static int __init sgx_init(void)
> {
> int ret;
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 9ec3136c7800..0f17def9fe6f 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -44,6 +44,7 @@ struct sgx_epc_page {
> struct sgx_numa_node {
> struct list_head free_page_list;
> struct list_head sgx_poison_page_list;
> + unsigned long size;
> spinlock_t lock;
> };
>
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index b5a4ba18f9f9..87acc47e8951 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -581,6 +581,9 @@ static const struct attribute_group node_dev_group = {
>
> static const struct attribute_group *node_dev_groups[] = {
> &node_dev_group,
> +#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
> + &arch_node_dev_group,
> +#endif
> NULL
> };
>
> diff --git a/include/linux/numa.h b/include/linux/numa.h
> index cb44cfe2b725..59df211d051f 100644
> --- a/include/linux/numa.h
> +++ b/include/linux/numa.h
> @@ -58,4 +58,8 @@ static inline int phys_to_target_node(u64 start)
> }
> #endif
>
> +#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
> +extern const struct attribute_group arch_node_dev_group;
> +#endif
> +
> #endif /* _LINUX_NUMA_H */
> --
> 2.32.0
>
So what still needs to be done to this?
/Jarkko
On 11/16/21 8:21 AM, Jarkko Sakkinen wrote:
> The amount of SGX memory on the system is determined by the BIOS and it
> varies wildly between systems. It can be from dozens of MB's on desktops
> or VM's, up to many GB's on servers. Just like for regular memory, it is
> sometimes useful to know the amount of usable SGX memory in the system.
>
> Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> specific attribute group, and add an attribute for the amount of SGX
> memory in bytes to each NUMA node:
>
> /sys/devices/system/node/nodeX/x86/sgx_total_bytes
There's some context missing here:
This serves the same function for SGX memory as /proc/meminfo or
/sys/devices/system/node/nodeX/meminfo does for normal RAM. It
enumerates how much physical SGX memory is present so that you can size
enclaves on different systems.
This specific file (sgx_total_bytes) is needed today to help drive the
SGX selftests. The SGX selftests need to create overcommitted enclaves
which are larger than the physical SGX memory on the system. They
currently use a CPUID-based approach which can diverge from the actual
amount of SGX memory available. This file ensures that the selftests
can work efficiently and do not attempt stupid things like creating a
100,000 MB enclave on a system with 128 MB of SGX memory.
The nodeX/x86 directory is used because SGX is highly x86-specific.
It's very unlikely that any other architecture (or even non-Intel x86
vendor) will ever implement SGX. It needs its own directory (as opposed
to being in the nodeX/ "root") because this is expected to be the first
of a few different things that need to get exported. This avoids
cluttering the root with several "sgx_*" files.
How many of these files will there be? Just scanning /proc/meminfo,
these are the no-brainers that we have for RAM, but we need for SGX:
MemTotal: xxxx kB // sgx_total_bytes (this patch)
MemFree: yyyy kB // sgx_free_bytes
SwapTotal: zzzz kB // sgx_swapped_bytes
So, at *least* three. I think we will eventually end up needing
something more along the lines of a dozen.
On Tue, Dec 07, 2021 at 11:36:50AM -0800, Dave Hansen wrote:
> On 11/16/21 8:21 AM, Jarkko Sakkinen wrote:
> > The amount of SGX memory on the system is determined by the BIOS and it
> > varies wildly between systems. It can be from dozens of MB's on desktops
> > or VM's, up to many GB's on servers. Just like for regular memory, it is
> > sometimes useful to know the amount of usable SGX memory in the system.
> >
> > Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> > specific attribute group, and add an attribute for the amount of SGX
> > memory in bytes to each NUMA node:
> >
> > /sys/devices/system/node/nodeX/x86/sgx_total_bytes
>
> There's some context missing here:
I'll say. "it is sometimes useful" is not the proper argumentation for
adding new stuff.
> This serves the same function for SGX memory as /proc/meminfo or
> /sys/devices/system/node/nodeX/meminfo does for normal RAM. It
> enumerates how much physical SGX memory is present so that you can size
> enclaves on different systems.
>
> This specific file (sgx_total_bytes) is needed today to help drive the
> SGX selftests. The SGX selftests need to create overcommitted enclaves
> which are larger than the physical SGX memory on the system. They
> currently use a CPUID-based approach which can diverge from the actual
> amount of SGX memory available. This file ensures that the selftests
> can work efficiently and do not attempt stupid things like creating a
> 100,000 MB enclave on a system with 128 MB of SGX memory.
Thanks, that's exactly what I was missing. Please stick that explanation
somewhere prominent.
> The nodeX/x86 directory is used because SGX is highly x86-specific.
> It's very unlikely that any other architecture (or even non-Intel x86
> vendor) will ever implement SGX. It needs its own directory (as opposed
> to being in the nodeX/ "root") because this is expected to be the first
> of a few different things that need to get exported. This avoids
> cluttering the root with several "sgx_*" files.
>
> How many of these files will there be? Just scanning /proc/meminfo,
> these are the no-brainers that we have for RAM, but we need for SGX:
>
> MemTotal: xxxx kB // sgx_total_bytes (this patch)
> MemFree: yyyy kB // sgx_free_bytes
> SwapTotal: zzzz kB // sgx_swapped_bytes
>
> So, at *least* three. I think we will eventually end up needing
> something more along the lines of a dozen.
Oh well, I guess.
I was going to propose to make that
...nodeX/sgx/
but having it be arch-specific would mean that theoretically we could
add some non-sgx files there too, in the future, and if/when needed.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
I reworked the changelog quite a bit, addressing some of Borislav's
questions. No code changes, though.
The result is below. I've retained Greg's ack. I'll stick this in
tip/x86/sgx if this looks OK to everyone.
---
From: Jarkko Sakkinen <[email protected]>
== Problem ==
The amount of SGX memory on a system is determined by the BIOS and it
varies wildly between systems. It can be as small as dozens of MB's
and as large as many GB's on servers. Just like how applications need
to know how much regular RAM is available, enclave builders need to
know how much SGX memory an enclave can consume.
== Solution ==
Introduce a new sysfs file:
/sys/devices/system/node/nodeX/x86/sgx_total_bytes
to enumerate the amount of SGX memory available in each NUMA node.
This serves the same function for SGX as /proc/meminfo or
/sys/devices/system/node/nodeX/meminfo does for normal RAM.
'sgx_total_bytes' is needed today to help drive the SGX selftests.
SGX-specific swap code is exercised by creating overcommitted enclaves
which are larger than the physical SGX memory on the system. They
currently use a CPUID-based approach which can diverge from the actual
amount of SGX memory available. 'sgx_total_bytes' ensures that the
selftests can work efficiently and do not attempt stupid things like
creating a 100,000 MB enclave on a system with 128 MB of SGX memory.
== Implementation Details ==
Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an
arch specific attribute group, and add an attribute for the amount of
SGX memory in bytes to each NUMA node:
== ABI Design Discussion ==
As opposed to the per-node ABI, a single, global ABI was considered.
However, this would prevent enclaves from being able to size
themselves so that they fit on a single NUMA node. Essentially, a
single value would rule out NUMA optimizations for enclaves.
Create a new "x86/" directory inside each "nodeX/" sysfs directory.
'sgx_total_bytes' is expected to be the first of at least a few
sgx-specific files to be placed in the new directory. Just scanning
/proc/meminfo, these are the no-brainers that we have for RAM, but we
need for SGX:
MemTotal: xxxx kB // sgx_total_bytes (implemented here)
MemFree: yyyy kB // sgx_free_bytes
SwapTotal: zzzz kB // sgx_swapped_bytes
So, at *least* three. I think we will eventually end up needing
something more along the lines of a dozen. A new directory (as
opposed to being in the nodeX/ "root") directory avoids cluttering the
root with several "sgx_*" files.
Place the new file in a new "nodeX/x86/" directory because SGX is
highly x86-specific. It is very unlikely that any other architecture
(or even non-Intel x86 vendor) will ever implement SGX. Using "sgx/"
as opposed to "x86/" was also considered. But, there is a real chance
this can get used for other arch-specific purposes.
[ dhansen: rewrite changelog ]
Signed-off-by: Jarkko Sakkinen <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
Documentation/ABI/stable/sysfs-devices-node | 6 ++++++
arch/Kconfig | 4 ++++
arch/x86/Kconfig | 1 +
arch/x86/kernel/cpu/sgx/main.c | 20 ++++++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 1 +
drivers/base/node.c | 3 +++
include/linux/numa.h | 4 ++++
7 files changed, 39 insertions(+)
On Wed, Dec 08, 2021 at 11:38:11AM -0800, Dave Hansen wrote:
> I reworked the changelog quite a bit, addressing some of Borislav's
> questions. No code changes, though.
>
> The result is below. I've retained Greg's ack. I'll stick this in
> tip/x86/sgx if this looks OK to everyone.
It does, thanks for taking the time!
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/sgx branch of tip:
Commit-ID: 50468e4313355b161cac8a5155a45832995b7f25
Gitweb: https://git.kernel.org/tip/50468e4313355b161cac8a5155a45832995b7f25
Author: Jarkko Sakkinen <[email protected]>
AuthorDate: Tue, 16 Nov 2021 18:21:16 +02:00
Committer: Dave Hansen <[email protected]>
CommitterDate: Thu, 09 Dec 2021 07:02:22 -08:00
x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node
== Problem ==
The amount of SGX memory on a system is determined by the BIOS and it
varies wildly between systems. It can be as small as dozens of MB's
and as large as many GB's on servers. Just like how applications need
to know how much regular RAM is available, enclave builders need to
know how much SGX memory an enclave can consume.
== Solution ==
Introduce a new sysfs file:
/sys/devices/system/node/nodeX/x86/sgx_total_bytes
to enumerate the amount of SGX memory available in each NUMA node.
This serves the same function for SGX as /proc/meminfo or
/sys/devices/system/node/nodeX/meminfo does for normal RAM.
'sgx_total_bytes' is needed today to help drive the SGX selftests.
SGX-specific swap code is exercised by creating overcommitted enclaves
which are larger than the physical SGX memory on the system. They
currently use a CPUID-based approach which can diverge from the actual
amount of SGX memory available. 'sgx_total_bytes' ensures that the
selftests can work efficiently and do not attempt stupid things like
creating a 100,000 MB enclave on a system with 128 MB of SGX memory.
== Implementation Details ==
Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an
arch specific attribute group, and add an attribute for the amount of
SGX memory in bytes to each NUMA node:
== ABI Design Discussion ==
As opposed to the per-node ABI, a single, global ABI was considered.
However, this would prevent enclaves from being able to size
themselves so that they fit on a single NUMA node. Essentially, a
single value would rule out NUMA optimizations for enclaves.
Create a new "x86/" directory inside each "nodeX/" sysfs directory.
'sgx_total_bytes' is expected to be the first of at least a few
sgx-specific files to be placed in the new directory. Just scanning
/proc/meminfo, these are the no-brainers that we have for RAM, but we
need for SGX:
MemTotal: xxxx kB // sgx_total_bytes (implemented here)
MemFree: yyyy kB // sgx_free_bytes
SwapTotal: zzzz kB // sgx_swapped_bytes
So, at *least* three. I think we will eventually end up needing
something more along the lines of a dozen. A new directory (as
opposed to being in the nodeX/ "root") directory avoids cluttering the
root with several "sgx_*" files.
Place the new file in a new "nodeX/x86/" directory because SGX is
highly x86-specific. It is very unlikely that any other architecture
(or even non-Intel x86 vendor) will ever implement SGX. Using "sgx/"
as opposed to "x86/" was also considered. But, there is a real chance
this can get used for other arch-specific purposes.
[ dhansen: rewrite changelog ]
Signed-off-by: Jarkko Sakkinen <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
Documentation/ABI/stable/sysfs-devices-node | 6 ++++++-
arch/Kconfig | 4 ++++-
arch/x86/Kconfig | 1 +-
arch/x86/kernel/cpu/sgx/main.c | 20 ++++++++++++++++++++-
arch/x86/kernel/cpu/sgx/sgx.h | 1 +-
drivers/base/node.c | 3 +++-
include/linux/numa.h | 4 ++++-
7 files changed, 39 insertions(+)
diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
index 484fc04..8db67aa 100644
--- a/Documentation/ABI/stable/sysfs-devices-node
+++ b/Documentation/ABI/stable/sysfs-devices-node
@@ -176,3 +176,9 @@ Contact: Keith Busch <[email protected]>
Description:
The cache write policy: 0 for write-back, 1 for write-through,
other or unknown.
+
+What: /sys/devices/system/node/nodeX/x86/sgx_total_bytes
+Date: November 2021
+Contact: Jarkko Sakkinen <[email protected]>
+Description:
+ The total amount of SGX physical memory in bytes.
diff --git a/arch/Kconfig b/arch/Kconfig
index 26b8ed1..0a9dadb 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1302,6 +1302,10 @@ config ARCH_HAS_PARANOID_L1D_FLUSH
config DYNAMIC_SIGFRAME
bool
+# Select, if arch has a named attribute group bound to NUMA device nodes.
+config HAVE_ARCH_NODE_DEV_GROUP
+ bool
+
source "kernel/gcov/Kconfig"
source "scripts/gcc-plugins/Kconfig"
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b9281fa..f2b699d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -269,6 +269,7 @@ config X86
select HAVE_ARCH_KCSAN if X86_64
select X86_FEATURE_NAMES if PROC_FS
select PROC_PID_ARCH_STATUS if PROC_FS
+ select HAVE_ARCH_NODE_DEV_GROUP if X86_SGX
imply IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
config INSTRUCTION_DECODER
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 6036328..2857a49 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -825,9 +825,11 @@ static bool __init sgx_page_cache_init(void)
INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_page_list);
INIT_LIST_HEAD(&sgx_numa_nodes[nid].sgx_poison_page_list);
node_set(nid, sgx_numa_mask);
+ sgx_numa_nodes[nid].size = 0;
}
sgx_epc_sections[i].node = &sgx_numa_nodes[nid];
+ sgx_numa_nodes[nid].size += size;
sgx_nr_epc_sections++;
}
@@ -901,6 +903,24 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
}
EXPORT_SYMBOL_GPL(sgx_set_attribute);
+#ifdef CONFIG_NUMA
+static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
+}
+static DEVICE_ATTR_RO(sgx_total_bytes);
+
+static struct attribute *arch_node_dev_attrs[] = {
+ &dev_attr_sgx_total_bytes.attr,
+ NULL,
+};
+
+const struct attribute_group arch_node_dev_group = {
+ .name = "x86",
+ .attrs = arch_node_dev_attrs,
+};
+#endif /* CONFIG_NUMA */
+
static int __init sgx_init(void)
{
int ret;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 9ec3136..0f17def 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -44,6 +44,7 @@ struct sgx_epc_page {
struct sgx_numa_node {
struct list_head free_page_list;
struct list_head sgx_poison_page_list;
+ unsigned long size;
spinlock_t lock;
};
diff --git a/drivers/base/node.c b/drivers/base/node.c
index b5a4ba1..87acc47 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -581,6 +581,9 @@ static const struct attribute_group node_dev_group = {
static const struct attribute_group *node_dev_groups[] = {
&node_dev_group,
+#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
+ &arch_node_dev_group,
+#endif
NULL
};
diff --git a/include/linux/numa.h b/include/linux/numa.h
index cb44cfe..59df211 100644
--- a/include/linux/numa.h
+++ b/include/linux/numa.h
@@ -58,4 +58,8 @@ static inline int phys_to_target_node(u64 start)
}
#endif
+#ifdef CONFIG_HAVE_ARCH_NODE_DEV_GROUP
+extern const struct attribute_group arch_node_dev_group;
+#endif
+
#endif /* _LINUX_NUMA_H */
On Tue, 2021-12-07 at 11:36 -0800, Dave Hansen wrote:
> On 11/16/21 8:21 AM, Jarkko Sakkinen wrote:
> > The amount of SGX memory on the system is determined by the BIOS and it
> > varies wildly between systems. It can be from dozens of MB's on desktops
> > or VM's, up to many GB's on servers. Just like for regular memory, it is
> > sometimes useful to know the amount of usable SGX memory in the system.
> >
> > Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> > specific attribute group, and add an attribute for the amount of SGX
> > memory in bytes to each NUMA node:
> >
> > /sys/devices/system/node/nodeX/x86/sgx_total_bytes
>
> There's some context missing here:
>
> This serves the same function for SGX memory as /proc/meminfo or
> /sys/devices/system/node/nodeX/meminfo does for normal RAM. It
> enumerates how much physical SGX memory is present so that you can size
> enclaves on different systems.
>
> This specific file (sgx_total_bytes) is needed today to help drive the
> SGX selftests. The SGX selftests need to create overcommitted enclaves
> which are larger than the physical SGX memory on the system. They
> currently use a CPUID-based approach which can diverge from the actual
> amount of SGX memory available. This file ensures that the selftests
> can work efficiently and do not attempt stupid things like creating a
> 100,000 MB enclave on a system with 128 MB of SGX memory.
>
> The nodeX/x86 directory is used because SGX is highly x86-specific.
> It's very unlikely that any other architecture (or even non-Intel x86
> vendor) will ever implement SGX. It needs its own directory (as opposed
> to being in the nodeX/ "root") because this is expected to be the first
> of a few different things that need to get exported. This avoids
> cluttering the root with several "sgx_*" files.
>
> How many of these files will there be? Just scanning /proc/meminfo,
> these are the no-brainers that we have for RAM, but we need for SGX:
>
> MemTotal: xxxx kB // sgx_total_bytes (this patch)
> MemFree: yyyy kB // sgx_free_bytes
> SwapTotal: zzzz kB // sgx_swapped_bytes
>
> So, at *least* three. I think we will eventually end up needing
> something more along the lines of a dozen.
These three I've had in mind for the moment. The latter two will be
trivial to add now that we have pattern how to add the sysfs attribute
in the correct way.
/Jarkko
On Wed, 2021-12-08 at 11:38 -0800, Dave Hansen wrote:
> I reworked the changelog quite a bit, addressing some of Borislav's
> questions. No code changes, though.
>
> The result is below. I've retained Greg's ack. I'll stick this in
> tip/x86/sgx if this looks OK to everyone.
>
> ---
>
> From: Jarkko Sakkinen <[email protected]>
>
> == Problem ==
>
> The amount of SGX memory on a system is determined by the BIOS and it
> varies wildly between systems. It can be as small as dozens of MB's
> and as large as many GB's on servers. Just like how applications need
> to know how much regular RAM is available, enclave builders need to
> know how much SGX memory an enclave can consume.
>
> == Solution ==
>
> Introduce a new sysfs file:
>
> /sys/devices/system/node/nodeX/x86/sgx_total_bytes
>
> to enumerate the amount of SGX memory available in each NUMA node.
> This serves the same function for SGX as /proc/meminfo or
> /sys/devices/system/node/nodeX/meminfo does for normal RAM.
>
> 'sgx_total_bytes' is needed today to help drive the SGX selftests.
> SGX-specific swap code is exercised by creating overcommitted enclaves
> which are larger than the physical SGX memory on the system. They
> currently use a CPUID-based approach which can diverge from the actual
> amount of SGX memory available. 'sgx_total_bytes' ensures that the
> selftests can work efficiently and do not attempt stupid things like
> creating a 100,000 MB enclave on a system with 128 MB of SGX memory.
>
> == Implementation Details ==
>
> Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an
> arch specific attribute group, and add an attribute for the amount of
> SGX memory in bytes to each NUMA node:
>
> == ABI Design Discussion ==
>
> As opposed to the per-node ABI, a single, global ABI was considered.
> However, this would prevent enclaves from being able to size
> themselves so that they fit on a single NUMA node. Essentially, a
> single value would rule out NUMA optimizations for enclaves.
>
> Create a new "x86/" directory inside each "nodeX/" sysfs directory.
> 'sgx_total_bytes' is expected to be the first of at least a few
> sgx-specific files to be placed in the new directory. Just scanning
> /proc/meminfo, these are the no-brainers that we have for RAM, but we
> need for SGX:
>
> MemTotal: xxxx kB // sgx_total_bytes (implemented here)
> MemFree: yyyy kB // sgx_free_bytes
> SwapTotal: zzzz kB // sgx_swapped_bytes
>
> So, at *least* three. I think we will eventually end up needing
> something more along the lines of a dozen. A new directory (as
> opposed to being in the nodeX/ "root") directory avoids cluttering the
> root with several "sgx_*" files.
>
> Place the new file in a new "nodeX/x86/" directory because SGX is
> highly x86-specific. It is very unlikely that any other architecture
> (or even non-Intel x86 vendor) will ever implement SGX. Using "sgx/"
> as opposed to "x86/" was also considered. But, there is a real chance
> this can get used for other arch-specific purposes.
>
> [ dhansen: rewrite changelog ]
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> Documentation/ABI/stable/sysfs-devices-node | 6 ++++++
> arch/Kconfig | 4 ++++
> arch/x86/Kconfig | 1 +
> arch/x86/kernel/cpu/sgx/main.c | 20 ++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/sgx.h | 1 +
> drivers/base/node.c | 3 +++
> include/linux/numa.h | 4 ++++
> 7 files changed, 39 insertions(+)
Thank you! Looks good to me.
/Jarkko
Hi Jarkko,
On Tue, Nov 16, 2021 at 06:21:16PM +0200, Jarkko Sakkinen wrote:
> The amount of SGX memory on the system is determined by the BIOS and it
> varies wildly between systems. It can be from dozens of MB's on desktops
> or VM's, up to many GB's on servers. Just like for regular memory, it is
> sometimes useful to know the amount of usable SGX memory in the system.
>
> Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> specific attribute group, and add an attribute for the amount of SGX
> memory in bytes to each NUMA node:
>
> /sys/devices/system/node/nodeX/x86/sgx_total_bytes
>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Apologies if this has already been reported or fixed, I haven't seen
anything from a full search of lore.kernel.org.
With this patch in -next as commit 50468e431335 ("x86/sgx: Add an
attribute for the amount of SGX memory in a NUMA node"), this sysfs node
causes an OOPS for me on at least one of my test systems (Intel based):
# cat /sys/devices/system/node/node0/x86/sgx_total_bytes
fish: Job 1, 'cat /sys/devices/system/node/no…' terminated by signal SIGKILL (Forced quit)
# dmesg
[ 56.956995] BUG: kernel NULL pointer dereference, address: 0000000000000020
[ 56.957003] #PF: supervisor read access in kernel mode
[ 56.957006] #PF: error_code(0x0000) - not-present page
[ 56.957009] PGD 0 P4D 0
[ 56.957013] Oops: 0000 [#2] PREEMPT SMP PTI
[ 56.957017] CPU: 1 PID: 866 Comm: cat Tainted: G D 5.16.0-rc5-next-20211217-debug #1 15ae5b0f28a4b9b6343440ee595affa8e1b5cf57
[ 56.957022] Hardware name: ASUSTeK COMPUTER INC. Q302LA/Q302LA, BIOS Q302LA.203 05/15/2014
[ 56.957024] RIP: 0010:sgx_total_bytes_show+0x28/0x40
[ 56.957032] Code: 66 90 0f 1f 44 00 00 49 89 f8 48 89 d7 48 c7 c6 5f b1 52 a4 41 8b 80 98 02 00 00 48 8d 04 40 48 c1 e0 04 48 03 05 38 b1 3c 02 <48> 8b 50 20 e8 ff ee 3b 00 48 98 31 d2 89 d6 89 d7 41 89 d0 c3 0f
[ 56.957035] RSP: 0018:ffffb79c81447d60 EFLAGS: 00010246
[ 56.957039] RAX: 0000000000000000 RBX: ffffffffa4c30ec0 RCX: 0000000000000000
[ 56.957041] RDX: ffff95d502b95000 RSI: ffffffffa452b15f RDI: ffff95d502b95000
[ 56.957044] RBP: ffffffffa41567c0 R08: ffff95d5002a0400 R09: 0000000000000000
[ 56.957046] R10: 0000000000000000 R11: 0000000000000000 R12: ffffb79c81447e28
[ 56.957048] R13: ffffb79c81447e00 R14: ffff95d5041247a8 R15: 0000000000000001
[ 56.957051] FS: 00007f4ab25af600(0000) GS:ffff95d616e80000(0000) knlGS:0000000000000000
[ 56.957054] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 56.957056] CR2: 0000000000000020 CR3: 00000001029e8005 CR4: 00000000001706e0
[ 56.957060] Call Trace:
[ 56.957062] <TASK>
[ 56.957064] dev_attr_show+0x19/0x40
[ 56.957072] sysfs_kf_seq_show+0xa1/0x120
[ 56.957080] seq_read_iter+0x12e/0x4c0
[ 56.957085] new_sync_read+0x159/0x1f0
[ 56.957093] vfs_read+0xff/0x1a0
[ 56.957099] ksys_read+0x67/0xf0
[ 56.957105] do_syscall_64+0x5c/0x90
[ 56.957110] ? syscall_exit_to_user_mode+0x23/0x50
[ 56.957116] ? exc_page_fault+0x72/0x180
[ 56.957121] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 56.957128] RIP: 0033:0x7f4ab24d1862
[ 56.957131] Code: c0 e9 b2 fe ff ff 50 48 8d 3d 5a 29 0a 00 e8 55 e4 01 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
[ 56.957134] RSP: 002b:00007ffff14dac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[ 56.957137] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007f4ab24d1862
[ 56.957140] RDX: 0000000000020000 RSI: 00007f4ab20d8000 RDI: 0000000000000003
[ 56.957142] RBP: 00007f4ab20d8000 R08: 00007f4ab20d7010 R09: 0000000000000000
[ 56.957144] R10: 0000000000000022 R11: 0000000000000246 R12: 0000000000020000
[ 56.957146] R13: 0000000000000003 R14: 0000000000000000 R15: 00007ffff14daf18
[ 56.957150] </TASK>
[ 56.957152] Modules linked in: 8021q garp mrp stp llc snd_hda_codec_hdmi ccm
algif_aead cbc x86_pkg_temp_thermal des_generic intel_powerclamp libdes
coretemp ecb kvm_intel algif_skcipher kvm ax88796b irqbypass cmac
crct10dif_pclmul i915 crc32_pclmul md4 ghash_clmulni_intel algif_hash
snd_hda_codec_realtek af_alg snd_hda_codec_generic ledtrig_audio uvcvideo
asus_nb_wmi aesni_intel snd_hda_intel intel_spi_platform i2c_algo_bit iwlmvm
videobuf2_vmalloc asus_wmi snd_intel_dspcfg ttm snd_intel_sdw_acpi crypto_simd
joydev ak8975 intel_spi iTCO_wdt btusb mousedev spi_nor sparse_keymap
intel_pmc_bxt at24 cryptd videobuf2_memops iTCO_vendor_support mac80211
hid_multitouch mtd platform_profile btrtl intel_rapl_msr mei_hdcp snd_hda_codec
btbcm rapl libarc4 videobuf2_v4l2 drm_kms_helper intel_cstate
processor_thermal_device_pci_legacy asix intel_uncore syscopyarea selftests
btintel snd_hda_core videobuf2_common pcspkr iwlwifi snd_hwdep
processor_thermal_device vfat btmtk i2c_i801 sysfillrect usbnet
[ 56.957229] snd_pcm processor_thermal_rfim psmouse fat sysimgblt i2c_smbus
bluetooth videodev processor_thermal_mbox snd_timer processor_thermal_rapl
fb_sys_fops mii cfg80211 mei_me usbhid snd ecdh_generic mdio_devres mc
inv_mpu6050_i2c intel_rapl_common cec crc16 int340x_thermal_zone libphy lpc_ich
rfkill intel_gtt mei soundcore intel_soc_dts_iosf inv_mpu6050 acpi_als wmi
dell_smo8800 industrialio_triggered_buffer int3400_thermal video i2c_mux
kfifo_buf soc_button_array industrialio acpi_thermal_rel asus_wireless mac_hid
drm pkcs8_key_parser fuse bpf_preload ip_tables x_tables xfs libcrc32c
crc32c_generic serio_raw atkbd libps2 i8042 crc32c_intel xhci_pci
xhci_pci_renesas serio
[ 56.957286] CR2: 0000000000000020
[ 56.957289] ---[ end trace 0000000000000000 ]---
[ 56.957291] RIP: 0010:sgx_total_bytes_show+0x28/0x40
[ 56.957296] Code: 66 90 0f 1f 44 00 00 49 89 f8 48 89 d7 48 c7 c6 5f b1 52 a4 41 8b 80 98 02 00 00 48 8d 04 40 48 c1 e0 04 48 03 05 38 b1 3c 02 <48> 8b 50 20 e8 ff ee 3b 00 48 98 31 d2 89 d6 89 d7 41 89 d0 c3 0f
[ 56.957299] RSP: 0018:ffffb79c8118fd58 EFLAGS: 00010246
[ 56.957301] RAX: 0000000000000000 RBX: ffffffffa4c30ec0 RCX: 0000000000000000
[ 56.957304] RDX: ffff95d50403d000 RSI: ffffffffa452b15f RDI: ffff95d50403d000
[ 56.957306] RBP: ffffffffa41567c0 R08: ffff95d5002a0400 R09: 0000000000000000
[ 56.957308] R10: 0000000000000000 R11: 0000000000000000 R12: ffffb79c8118fe20
[ 56.957310] R13: ffffb79c8118fdf8 R14: ffff95d503fd0550 R15: 0000000000000001
[ 56.957312] FS: 00007f4ab25af600(0000) GS:ffff95d616e80000(0000) knlGS:0000000000000000
[ 56.957315] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 56.957317] CR2: 0000000000000020 CR3: 00000001029e8005 CR4: 00000000001706e0
If I can provide any further information or testing, let me know!
Cheers,
Nathan
On 12/17/21 11:12 AM, Nathan Chancellor wrote:
> With this patch in -next as commit 50468e431335 ("x86/sgx: Add an
> attribute for the amount of SGX memory in a NUMA node"), this sysfs node
> causes an OOPS for me on at least one of my test systems (Intel based):
Thanks for the report!
Could you share /proc/cpuinfo and a full dmesg from when this happens?
I'm curious if your system has SGX at all and if it had any issues
during initialization.
On Fri, Dec 17, 2021 at 01:17:42PM -0800, Dave Hansen wrote:
> On 12/17/21 11:12 AM, Nathan Chancellor wrote:
> > With this patch in -next as commit 50468e431335 ("x86/sgx: Add an
> > attribute for the amount of SGX memory in a NUMA node"), this sysfs node
> > causes an OOPS for me on at least one of my test systems (Intel based):
>
> Thanks for the report!
>
> Could you share /proc/cpuinfo and a full dmesg from when this happens?
> I'm curious if your system has SGX at all and if it had any issues
> during initialization.
Sure thing, find them attached (does not look like it does). Let me know
if there is any more information I can provide or test fixes.
Cheers,
Nathan
On Fri, Dec 17, 2021 at 12:12:06PM -0700, Nathan Chancellor wrote:
> Hi Jarkko,
>
> On Tue, Nov 16, 2021 at 06:21:16PM +0200, Jarkko Sakkinen wrote:
> > The amount of SGX memory on the system is determined by the BIOS and it
> > varies wildly between systems. It can be from dozens of MB's on desktops
> > or VM's, up to many GB's on servers. Just like for regular memory, it is
> > sometimes useful to know the amount of usable SGX memory in the system.
> >
> > Introduce CONFIG_HAVE_ARCH_NODE_DEV_GROUP opt-in flag to expose an arch
> > specific attribute group, and add an attribute for the amount of SGX
> > memory in bytes to each NUMA node:
> >
> > /sys/devices/system/node/nodeX/x86/sgx_total_bytes
> >
> > Acked-by: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
>
> Apologies if this has already been reported or fixed, I haven't seen
> anything from a full search of lore.kernel.org.
>
> With this patch in -next as commit 50468e431335 ("x86/sgx: Add an
> attribute for the amount of SGX memory in a NUMA node"), this sysfs node
> causes an OOPS for me on at least one of my test systems (Intel based):
>
> # cat /sys/devices/system/node/node0/x86/sgx_total_bytes
> fish: Job 1, 'cat /sys/devices/system/node/no…' terminated by signal SIGKILL (Forced quit)
>
> # dmesg
> [ 56.956995] BUG: kernel NULL pointer dereference, address: 0000000000000020
> [ 56.957003] #PF: supervisor read access in kernel mode
> [ 56.957006] #PF: error_code(0x0000) - not-present page
> [ 56.957009] PGD 0 P4D 0
> [ 56.957013] Oops: 0000 [#2] PREEMPT SMP PTI
> [ 56.957017] CPU: 1 PID: 866 Comm: cat Tainted: G D 5.16.0-rc5-next-20211217-debug #1 15ae5b0f28a4b9b6343440ee595affa8e1b5cf57
> [ 56.957022] Hardware name: ASUSTeK COMPUTER INC. Q302LA/Q302LA, BIOS Q302LA.203 05/15/2014
> [ 56.957024] RIP: 0010:sgx_total_bytes_show+0x28/0x40
> [ 56.957032] Code: 66 90 0f 1f 44 00 00 49 89 f8 48 89 d7 48 c7 c6 5f b1 52 a4 41 8b 80 98 02 00 00 48 8d 04 40 48 c1 e0 04 48 03 05 38 b1 3c 02 <48> 8b 50 20 e8 ff ee 3b 00 48 98 31 d2 89 d6 89 d7 41 89 d0 c3 0f
> [ 56.957035] RSP: 0018:ffffb79c81447d60 EFLAGS: 00010246
> [ 56.957039] RAX: 0000000000000000 RBX: ffffffffa4c30ec0 RCX: 0000000000000000
> [ 56.957041] RDX: ffff95d502b95000 RSI: ffffffffa452b15f RDI: ffff95d502b95000
> [ 56.957044] RBP: ffffffffa41567c0 R08: ffff95d5002a0400 R09: 0000000000000000
> [ 56.957046] R10: 0000000000000000 R11: 0000000000000000 R12: ffffb79c81447e28
> [ 56.957048] R13: ffffb79c81447e00 R14: ffff95d5041247a8 R15: 0000000000000001
> [ 56.957051] FS: 00007f4ab25af600(0000) GS:ffff95d616e80000(0000) knlGS:0000000000000000
> [ 56.957054] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 56.957056] CR2: 0000000000000020 CR3: 00000001029e8005 CR4: 00000000001706e0
> [ 56.957060] Call Trace:
> [ 56.957062] <TASK>
> [ 56.957064] dev_attr_show+0x19/0x40
> [ 56.957072] sysfs_kf_seq_show+0xa1/0x120
> [ 56.957080] seq_read_iter+0x12e/0x4c0
> [ 56.957085] new_sync_read+0x159/0x1f0
> [ 56.957093] vfs_read+0xff/0x1a0
> [ 56.957099] ksys_read+0x67/0xf0
> [ 56.957105] do_syscall_64+0x5c/0x90
> [ 56.957110] ? syscall_exit_to_user_mode+0x23/0x50
> [ 56.957116] ? exc_page_fault+0x72/0x180
> [ 56.957121] entry_SYSCALL_64_after_hwframe+0x44/0xae
> [ 56.957128] RIP: 0033:0x7f4ab24d1862
> [ 56.957131] Code: c0 e9 b2 fe ff ff 50 48 8d 3d 5a 29 0a 00 e8 55 e4 01 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
> [ 56.957134] RSP: 002b:00007ffff14dac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> [ 56.957137] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007f4ab24d1862
> [ 56.957140] RDX: 0000000000020000 RSI: 00007f4ab20d8000 RDI: 0000000000000003
> [ 56.957142] RBP: 00007f4ab20d8000 R08: 00007f4ab20d7010 R09: 0000000000000000
> [ 56.957144] R10: 0000000000000022 R11: 0000000000000246 R12: 0000000000020000
> [ 56.957146] R13: 0000000000000003 R14: 0000000000000000 R15: 00007ffff14daf18
> [ 56.957150] </TASK>
> [ 56.957152] Modules linked in: 8021q garp mrp stp llc snd_hda_codec_hdmi ccm
> algif_aead cbc x86_pkg_temp_thermal des_generic intel_powerclamp libdes
> coretemp ecb kvm_intel algif_skcipher kvm ax88796b irqbypass cmac
> crct10dif_pclmul i915 crc32_pclmul md4 ghash_clmulni_intel algif_hash
> snd_hda_codec_realtek af_alg snd_hda_codec_generic ledtrig_audio uvcvideo
> asus_nb_wmi aesni_intel snd_hda_intel intel_spi_platform i2c_algo_bit iwlmvm
> videobuf2_vmalloc asus_wmi snd_intel_dspcfg ttm snd_intel_sdw_acpi crypto_simd
> joydev ak8975 intel_spi iTCO_wdt btusb mousedev spi_nor sparse_keymap
> intel_pmc_bxt at24 cryptd videobuf2_memops iTCO_vendor_support mac80211
> hid_multitouch mtd platform_profile btrtl intel_rapl_msr mei_hdcp snd_hda_codec
> btbcm rapl libarc4 videobuf2_v4l2 drm_kms_helper intel_cstate
> processor_thermal_device_pci_legacy asix intel_uncore syscopyarea selftests
> btintel snd_hda_core videobuf2_common pcspkr iwlwifi snd_hwdep
> processor_thermal_device vfat btmtk i2c_i801 sysfillrect usbnet
> [ 56.957229] snd_pcm processor_thermal_rfim psmouse fat sysimgblt i2c_smbus
> bluetooth videodev processor_thermal_mbox snd_timer processor_thermal_rapl
> fb_sys_fops mii cfg80211 mei_me usbhid snd ecdh_generic mdio_devres mc
> inv_mpu6050_i2c intel_rapl_common cec crc16 int340x_thermal_zone libphy lpc_ich
> rfkill intel_gtt mei soundcore intel_soc_dts_iosf inv_mpu6050 acpi_als wmi
> dell_smo8800 industrialio_triggered_buffer int3400_thermal video i2c_mux
> kfifo_buf soc_button_array industrialio acpi_thermal_rel asus_wireless mac_hid
> drm pkcs8_key_parser fuse bpf_preload ip_tables x_tables xfs libcrc32c
> crc32c_generic serio_raw atkbd libps2 i8042 crc32c_intel xhci_pci
> xhci_pci_renesas serio
> [ 56.957286] CR2: 0000000000000020
> [ 56.957289] ---[ end trace 0000000000000000 ]---
> [ 56.957291] RIP: 0010:sgx_total_bytes_show+0x28/0x40
> [ 56.957296] Code: 66 90 0f 1f 44 00 00 49 89 f8 48 89 d7 48 c7 c6 5f b1 52 a4 41 8b 80 98 02 00 00 48 8d 04 40 48 c1 e0 04 48 03 05 38 b1 3c 02 <48> 8b 50 20 e8 ff ee 3b 00 48 98 31 d2 89 d6 89 d7 41 89 d0 c3 0f
> [ 56.957299] RSP: 0018:ffffb79c8118fd58 EFLAGS: 00010246
> [ 56.957301] RAX: 0000000000000000 RBX: ffffffffa4c30ec0 RCX: 0000000000000000
> [ 56.957304] RDX: ffff95d50403d000 RSI: ffffffffa452b15f RDI: ffff95d50403d000
> [ 56.957306] RBP: ffffffffa41567c0 R08: ffff95d5002a0400 R09: 0000000000000000
> [ 56.957308] R10: 0000000000000000 R11: 0000000000000000 R12: ffffb79c8118fe20
> [ 56.957310] R13: ffffb79c8118fdf8 R14: ffff95d503fd0550 R15: 0000000000000001
> [ 56.957312] FS: 00007f4ab25af600(0000) GS:ffff95d616e80000(0000) knlGS:0000000000000000
> [ 56.957315] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 56.957317] CR2: 0000000000000020 CR3: 00000001029e8005 CR4: 00000000001706e0
>
> If I can provide any further information or testing, let me know!
Dave, when is the fix going to be applied [*]?
>
> Cheers,
> Nathan
[*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
BR,
Jarkko
On 12/28/21 3:45 PM, Jarkko Sakkinen wrote:
>> If I can provide any further information or testing, let me know!
> Dave, when is the fix going to be applied [*]?
>
>> Cheers,
>> Nathan
> [*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
Greg preferred hiding the file as opposed to faking a number in there.
Any testing of the attached would be appreciated.
On Sat, Jan 01, 2022 at 08:54:51PM -0800, Dave Hansen wrote:
> On 12/28/21 3:45 PM, Jarkko Sakkinen wrote:
> >> If I can provide any further information or testing, let me know!
> > Dave, when is the fix going to be applied [*]?
> >
> >> Cheers,
> >> Nathan
> > [*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
>
> Greg preferred hiding the file as opposed to faking a number in there.
> Any testing of the attached would be appreciated.
>
> From: Dave Hansen <[email protected]>
>
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
>
> https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
>
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally. However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
>
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
>
> To fix this, hide the entire nodeX/x86/ attribute group on
> systems without SGX support using the ->is_visible attribute
> group callback.
>
> Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
> Reported-by: Nathan Chancellor <[email protected]>
> CC: Greg Kroah-Hartman <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
On both my older Intel based test system and AMD based test system, the
sgx_total_bytes node is no longer visible.
$ ls -al /sys/devices/system/node/node0/x86/
Tested-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
> ---
>
> b/arch/x86/kernel/cpu/sgx/main.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr 2021-12-20 07:56:38.309584807 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c 2021-12-20 08:17:28.997705149 -0800
> @@ -910,6 +910,16 @@ static ssize_t sgx_total_bytes_show(stru
> }
> static DEVICE_ATTR_RO(sgx_total_bytes);
>
> +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
> + struct attribute * attr, int idx)
> +{
> + /* Make all x86/ attributes invisible when SGX is not initialized: */
> + if (nodes_empty(sgx_numa_mask))
> + return 0;
> +
> + return attr->mode;
> +}
> +
> static struct attribute *arch_node_dev_attrs[] = {
> &dev_attr_sgx_total_bytes.attr,
> NULL,
> @@ -918,6 +928,7 @@ static struct attribute *arch_node_dev_a
> const struct attribute_group arch_node_dev_group = {
> .name = "x86",
> .attrs = arch_node_dev_attrs,
> + .is_visible = arch_node_attr_is_visible,
> };
> #endif /* CONFIG_NUMA */
>
> _
On 1/1/22 8:54 PM, Dave Hansen wrote:
> On 12/28/21 3:45 PM, Jarkko Sakkinen wrote:
>>> If I can provide any further information or testing, let me know!
>> Dave, when is the fix going to be applied [*]?
>>
>>> Cheers,
>>> Nathan
>> [*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
> Greg preferred hiding the file as opposed to faking a number in there.
> Any testing of the attached would be appreciated.
Well, that didn't work on real SGX hardware. The sysfs node code calls
the ->is_visible() callback before SGX is initialized.
I'll send out and updated version shortly that uses sysfs_update_group()
instead.
On Sat, Jan 01, 2022 at 08:54:51PM -0800, Dave Hansen wrote:
> On 12/28/21 3:45 PM, Jarkko Sakkinen wrote:
> >> If I can provide any further information or testing, let me know!
> > Dave, when is the fix going to be applied [*]?
> >
> >> Cheers,
> >> Nathan
> > [*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
>
> Greg preferred hiding the file as opposed to faking a number in there.
> Any testing of the attached would be appreciated.
>
> From: Dave Hansen <[email protected]>
>
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
>
> https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
>
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally. However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
>
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
>
> To fix this, hide the entire nodeX/x86/ attribute group on
> systems without SGX support using the ->is_visible attribute
> group callback.
>
> Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
> Reported-by: Nathan Chancellor <[email protected]>
> CC: Greg Kroah-Hartman <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Dave Hansen <[email protected]>
> ---
>
> b/arch/x86/kernel/cpu/sgx/main.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr 2021-12-20 07:56:38.309584807 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c 2021-12-20 08:17:28.997705149 -0800
> @@ -910,6 +910,16 @@ static ssize_t sgx_total_bytes_show(stru
> }
> static DEVICE_ATTR_RO(sgx_total_bytes);
>
> +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
> + struct attribute * attr, int idx)
> +{
> + /* Make all x86/ attributes invisible when SGX is not initialized: */
> + if (nodes_empty(sgx_numa_mask))
> + return 0;
> +
> + return attr->mode;
> +}
> +
> static struct attribute *arch_node_dev_attrs[] = {
> &dev_attr_sgx_total_bytes.attr,
> NULL,
> @@ -918,6 +928,7 @@ static struct attribute *arch_node_dev_a
> const struct attribute_group arch_node_dev_group = {
> .name = "x86",
> .attrs = arch_node_dev_attrs,
> + .is_visible = arch_node_attr_is_visible,
> };
> #endif /* CONFIG_NUMA */
>
> _
I'm compiling now kernel with this applied, reporting soon but the fix
looks good to me.
/Jarkko
On Thu, Jan 06, 2022 at 09:18:35PM +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 01, 2022 at 08:54:51PM -0800, Dave Hansen wrote:
> > On 12/28/21 3:45 PM, Jarkko Sakkinen wrote:
> > >> If I can provide any further information or testing, let me know!
> > > Dave, when is the fix going to be applied [*]?
> > >
> > >> Cheers,
> > >> Nathan
> > > [*] https://lore.kernel.org/linux-sgx/[email protected]/T/#m831a01bdde347f9e0af2c973986fae0499718201
> >
> > Greg preferred hiding the file as opposed to faking a number in there.
> > Any testing of the attached would be appreciated.
>
> >
> > From: Dave Hansen <[email protected]>
> >
> > Nathan Chancellor reported an oops when aceessing the
> > 'sgx_total_bytes' sysfs file:
> >
> > https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> >
> > The sysfs output code accesses the sgx_numa_nodes[] array
> > unconditionally. However, this array is allocated during SGX
> > initialization, which only occurs on systems where SGX is
> > supported.
> >
> > If the sysfs file is accessed on systems without SGX support,
> > sgx_numa_nodes[] is NULL and an oops occurs.
> >
> > To fix this, hide the entire nodeX/x86/ attribute group on
> > systems without SGX support using the ->is_visible attribute
> > group callback.
> >
> > Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
> > Reported-by: Nathan Chancellor <[email protected]>
> > CC: Greg Kroah-Hartman <[email protected]>
> > Cc: Jarkko Sakkinen <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Dave Hansen <[email protected]>
> > ---
> >
> > b/arch/x86/kernel/cpu/sgx/main.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> > --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr 2021-12-20 07:56:38.309584807 -0800
> > +++ b/arch/x86/kernel/cpu/sgx/main.c 2021-12-20 08:17:28.997705149 -0800
> > @@ -910,6 +910,16 @@ static ssize_t sgx_total_bytes_show(stru
> > }
> > static DEVICE_ATTR_RO(sgx_total_bytes);
> >
> > +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
> > + struct attribute * attr, int idx)
> > +{
> > + /* Make all x86/ attributes invisible when SGX is not initialized: */
> > + if (nodes_empty(sgx_numa_mask))
> > + return 0;
> > +
> > + return attr->mode;
> > +}
> > +
> > static struct attribute *arch_node_dev_attrs[] = {
> > &dev_attr_sgx_total_bytes.attr,
> > NULL,
> > @@ -918,6 +928,7 @@ static struct attribute *arch_node_dev_a
> > const struct attribute_group arch_node_dev_group = {
> > .name = "x86",
> > .attrs = arch_node_dev_attrs,
> > + .is_visible = arch_node_attr_is_visible,
> > };
> > #endif /* CONFIG_NUMA */
> >
> > _
>
> I'm compiling now kernel with this applied, reporting soon but the fix
> looks good to me.
Tested-by: Jarkko Sakkinen <[email protected]>
Did two tests in a virtual machine:
1. Without the patch reproduced the crash.
2. With the patch verified that the crash does not occur.
/Jarkko