[PATCH v5 1/2] x86/sgx: Rename fallback labels in sgx_init()

[PATCH v5 1/2] x86/sgx: Rename fallback labels in sgx_init()

[Jarkko Sakkinen]

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 <jarkko@kernel.org>
---
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 63d3de02bbcc..a6e313f1a82d 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -803,12 +803,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.
@@ -821,17 +821,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.25.1

[PATCH v5 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Jarkko Sakkinen]

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.

Add an attribute for the amount of SGX memory in bytes to each NUMA
node. The path is /sys/devices/system/node/node[0-9]*/sgx/memory_size.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

---
v5: A new patch based on the discussion at
    https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
---
 Documentation/x86/sgx.rst      | 14 ++++++
 arch/x86/kernel/cpu/sgx/main.c | 90 ++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
 3 files changed, 106 insertions(+)

diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
index dd0ac96ff9ef..f9d9cfa6dbf9 100644
--- a/Documentation/x86/sgx.rst
+++ b/Documentation/x86/sgx.rst
@@ -250,3 +250,17 @@ user wants to deploy SGX applications both on the host and in guests
 on the same machine, the user should reserve enough EPC (by taking out
 total virtual EPC size of all SGX VMs from the physical EPC size) for
 host SGX applications so they can run with acceptable performance.
+
+Per NUMA node SGX attributes
+============================
+
+NUMA nodes devices expose SGX specific attributes in the following path:
+
+	/sys/devices/system/node/node[0-9]*/sgx/
+
+Attributes
+----------
+
+memory_size
+                Total available physical SGX memory, also known as Enclave
+                Page Cache (EPC), in bytes.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index a6e313f1a82d..c43b5a0120c1 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -717,6 +717,7 @@ static bool __init sgx_page_cache_init(void)
 		}
 
 		sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
+		sgx_numa_nodes[nid].size += size;
 
 		sgx_nr_epc_sections++;
 	}
@@ -790,6 +791,87 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+#ifdef CONFIG_NUMA
+static void sgx_numa_exit(void)
+{
+	int nid;
+
+	for (nid = 0; nid < num_possible_nodes(); nid++) {
+		if (!sgx_numa_nodes[nid].kobj)
+			continue;
+
+		kobject_put(sgx_numa_nodes[nid].kobj);
+	}
+}
+
+#define SGX_NODE_ATTR_RO(_name) \
+	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
+
+static ssize_t memory_size_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	unsigned long size = 0;
+	int nid;
+
+	for (nid = 0; nid < num_possible_nodes(); nid++) {
+		if (kobj == sgx_numa_nodes[nid].kobj) {
+			size = sgx_numa_nodes[nid].size;
+			break;
+		}
+	}
+
+	return sysfs_emit(buf, "%lu\n", size);
+}
+SGX_NODE_ATTR_RO(memory_size);
+
+static struct attribute *sgx_node_attrs[] = {
+	&memory_size_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group sgx_node_attr_group = {
+	.attrs = sgx_node_attrs,
+};
+
+static bool sgx_numa_init(void)
+{
+	struct sgx_numa_node *node;
+	struct device *dev;
+	int nid;
+	int ret;
+
+	for (nid = 0; nid < num_possible_nodes(); nid++) {
+		if (!sgx_numa_nodes[nid].size)
+			continue;
+
+		node = &sgx_numa_nodes[nid];
+		dev = &node_devices[nid]->dev;
+
+		node->kobj = kobject_create_and_add("sgx", &dev->kobj);
+		if (!node->kobj) {
+			sgx_numa_exit();
+			return false;
+		}
+
+		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);
+		if (ret) {
+			sgx_numa_exit();
+			return false;
+		}
+	}
+
+	return true;
+}
+#else
+static inline void sgx_numa_exit(void)
+{
+}
+
+static inline bool sgx_numa_init(void)
+{
+	return true;
+}
+#endif /* CONFIG_NUMA */
+
 static int __init sgx_init(void)
 {
 	int ret;
@@ -806,6 +888,11 @@ static int __init sgx_init(void)
 		goto err_reclaimer;
 	}
 
+	if (!sgx_numa_init()) {
+		ret = -ENOMEM;
+		goto err_numa_nodes;
+	}
+
 	ret = misc_register(&sgx_dev_provision);
 	if (ret)
 		goto err_provision;
@@ -829,6 +916,9 @@ static int __init sgx_init(void)
 	misc_deregister(&sgx_dev_provision);
 
 err_provision:
+	sgx_numa_exit();
+
+err_numa_nodes:
 	kthread_stop(ksgxd_tsk);
 
 err_reclaimer:
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 4628acec0009..c2c5e7c66d21 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -39,6 +39,8 @@ struct sgx_epc_page {
  */
 struct sgx_numa_node {
 	struct list_head free_page_list;
+	struct kobject *kobj;
+	unsigned long size;
 	spinlock_t lock;
 };
 
-- 
2.25.1

Re: [PATCH v5 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Reinette Chatre]

Hi Jarkko,

On 9/13/2021 8:04 PM, 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.
> 
> Add an attribute for the amount of SGX memory in bytes to each NUMA
> node. The path is /sys/devices/system/node/node[0-9]*/sgx/memory_size.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> ---
> v5: A new patch based on the discussion at
>      https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> ---
>   Documentation/x86/sgx.rst      | 14 ++++++
>   arch/x86/kernel/cpu/sgx/main.c | 90 ++++++++++++++++++++++++++++++++++
>   arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
>   3 files changed, 106 insertions(+)
> 


...

> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index a6e313f1a82d..c43b5a0120c1 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -717,6 +717,7 @@ static bool __init sgx_page_cache_init(void)
>   		}
>   
>   		sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
> +		sgx_numa_nodes[nid].size += size;
>   
>   		sgx_nr_epc_sections++;
>   	}

The above memory seems to be uninitialized at the time it is incremented.

I tried this out on a system that reports the following:

$ dmesg | grep EPC
[    7.252838] sgx: EPC section 0x1000c00000-0x107f7fffff
[    7.256921] sgx: EPC section 0x2000c00000-0x207fffffff

It shows unexpectedly large values:
$ cat /sys/devices/system/node/node*/sgx/memory_size
12421486739271732874
16308428754864105707

System reported sane values after adding this fixup:

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 3380390cc052..d73bbfbfc05d 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -621,7 +621,7 @@ static bool __init sgx_page_cache_init(void)
  	int nid;
  	int i;

-	sgx_numa_nodes = kmalloc_array(num_possible_nodes(), 
sizeof(*sgx_numa_nodes), GFP_KERNEL);
+	sgx_numa_nodes = kcalloc(num_possible_nodes(), 
sizeof(*sgx_numa_nodes), GFP_KERNEL);
  	if (!sgx_numa_nodes)
  		return false;


After fixup:
$ cat /sys/devices/system/node/node*/sgx/memory_size
2126512128
2134900736


Reinette

Re: [PATCH v5 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Jarkko Sakkinen]

On Wed, 2021-09-22 at 16:30 -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/13/2021 8:04 PM, 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.
> > 
> > Add an attribute for the amount of SGX memory in bytes to each NUMA
> > node. The path is /sys/devices/system/node/node[0-9]*/sgx/memory_size.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > ---
> > v5: A new patch based on the discussion at
> >      https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> > ---
> >   Documentation/x86/sgx.rst      | 14 ++++++
> >   arch/x86/kernel/cpu/sgx/main.c | 90 ++++++++++++++++++++++++++++++++++
> >   arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
> >   3 files changed, 106 insertions(+)
> > 
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index a6e313f1a82d..c43b5a0120c1 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -717,6 +717,7 @@ static bool __init sgx_page_cache_init(void)
> >   		}
> >   
> >   		sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
> > +		sgx_numa_nodes[nid].size += size;
> >   
> >   		sgx_nr_epc_sections++;
> >   	}
> 
> The above memory seems to be uninitialized at the time it is incremented.
> 
> I tried this out on a system that reports the following:
> 
> $ dmesg | grep EPC
> [    7.252838] sgx: EPC section 0x1000c00000-0x107f7fffff
> [    7.256921] sgx: EPC section 0x2000c00000-0x207fffffff
> 
> It shows unexpectedly large values:
> $ cat /sys/devices/system/node/node*/sgx/memory_size
> 12421486739271732874
> 16308428754864105707
> 
> System reported sane values after adding this fixup:
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 3380390cc052..d73bbfbfc05d 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -621,7 +621,7 @@ static bool __init sgx_page_cache_init(void)
>   	int nid;
>   	int i;
> 
> -	sgx_numa_nodes = kmalloc_array(num_possible_nodes(), 
> sizeof(*sgx_numa_nodes), GFP_KERNEL);
> +	sgx_numa_nodes = kcalloc(num_possible_nodes(), 
> sizeof(*sgx_numa_nodes), GFP_KERNEL);
>   	if (!sgx_numa_nodes)
>   		return false;
> 
> 
> After fixup:
> $ cat /sys/devices/system/node/node*/sgx/memory_size
> 2126512128
> 2134900736

Thanks! I did not experience in a VM.

So cat you pick these patches to your patch set, and squash
this fix to it?


/Jarkko

Re: [PATCH v5 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Reinette Chatre]

Hi Jarkko,

On 9/23/2021 1:30 PM, Jarkko Sakkinen wrote:
> 
> So cat you pick these patches to your patch set, and squash
> this fix to it?

My patch set is focused on SGX selftests while this series target the 
x86 tree. I assumed that this series would go into x86 separately and 
after they land we can proceed with the SGX selftest work.

Reinette

Re: [PATCH v5 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Jarkko Sakkinen]

On Thu, 2021-09-23 at 13:39 -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/23/2021 1:30 PM, Jarkko Sakkinen wrote:
> > So cat you pick these patches to your patch set, and squash
> > this fix to it?
> 
> My patch set is focused on SGX selftests while this series target the 
> x86 tree. I assumed that this series would go into x86 separately and 
> after they land we can proceed with the SGX selftest work.

But now your series has no chance to be applied, given that
it contains patches which have discarded given a superior
approach.

Anyway, node fields are initialized here:

		if (!node_isset(nid, sgx_numa_mask)) {
			spin_lock_init(&sgx_numa_nodes[nid].lock);
			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_page_list);
			node_set(nid, sgx_numa_mask);
		}

The correct way to fix the issue is to add

			sgx_numa_nodes[nid].size = 0;

Using kcalloc() would not be very sound, since you would wastefully
initialize the pre-existing fields of the struct two times: first
with zeros, and then with "real" values.

/Jarkko