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

[PATCH v6 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 v6 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.
Calculate these values by summing up EPC section sizes for each node
during the driver initalization.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---

v6:
* Initialize node->size to zero in sgx_setup_epc_section(), when the
  node is first accessed. The bug report:
  https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@intel.com/

v5:
* A new patch based on the discussion on
  https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t

 Documentation/x86/sgx.rst      | 14 ++++++
 arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
 3 files changed, 107 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..4f1e3b5e3d14 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
 			spin_lock_init(&sgx_numa_nodes[nid].lock);
 			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_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++;
 	}
@@ -790,6 +792,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 +889,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 +917,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 v6 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Greg Kroah-Hartman]

On Tue, Sep 28, 2021 at 06:13:50AM +0300, 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.
> Calculate these values by summing up EPC section sizes for each node
> during the driver initalization.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> 
> v6:
> * Initialize node->size to zero in sgx_setup_epc_section(), when the
>   node is first accessed. The bug report:
>   https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@intel.com/
> 
> v5:
> * A new patch based on the discussion on
>   https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> 
>  Documentation/x86/sgx.rst      | 14 ++++++

sysfs files have to be documented in Documentation/ABI/ so that they can
be automatically checked, and added to the documentation output
properly.  Please do that here as well.


>  arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
>  3 files changed, 107 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..4f1e3b5e3d14 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
>  			spin_lock_init(&sgx_numa_nodes[nid].lock);
>  			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_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++;
>  	}
> @@ -790,6 +792,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)

Why are you dealing with a "raw" kobject?  Shouldn't you have a device
and use a device attribute?

> +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);

You just "broke" the tree by putting a raw kobject below a struct
device.  Please do not do that.

> +		if (!node->kobj) {
> +			sgx_numa_exit();
> +			return false;
> +		}
> +
> +		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);

And you raced with userspace and lost.

Wait, you have a kobject _just_ for a subdirectory name?  Why?  Use a
named attribute group, that's exactly what that is for.

Properly attach your attributes to the device you have, don't do extra
work and complex code that you do not have to at all.

thanks,

greg k-h

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

[Jarkko Sakkinen]

On Tue, 2021-09-28 at 06:54 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 28, 2021 at 06:13:50AM +0300, 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.
> > Calculate these values by summing up EPC section sizes for each node
> > during the driver initalization.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > 
> > v6:
> > * Initialize node->size to zero in sgx_setup_epc_section(), when the
> >   node is first accessed. The bug report:
> >   https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@intel.com/
> > 
> > v5:
> > * A new patch based on the discussion on
> >   https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> > 
> >  Documentation/x86/sgx.rst      | 14 ++++++
> 
> sysfs files have to be documented in Documentation/ABI/ so that they can
> be automatically checked, and added to the documentation output
> properly.  Please do that here as well.

Right, I'll document it to sysfs-devices-node.

> >  arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
> >  3 files changed, 107 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..4f1e3b5e3d14 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
> >  			spin_lock_init(&sgx_numa_nodes[nid].lock);
> >  			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_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++;
> >  	}
> > @@ -790,6 +792,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)
> 
> Why are you dealing with a "raw" kobject?  Shouldn't you have a device
> and use a device attribute?
> 
> > +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);
> 
> You just "broke" the tree by putting a raw kobject below a struct
> device.  Please do not do that.

I looked how hugetlb was implemented as a starting point, since it is
existing mainline code, i.e. in mm/hugetlb.c:

- hugetlb_register_node()
- hugetlb_register_all_nodes()
- HSTATE_ATTR_RO()

hugetlb code attaches raw kobjects to the node device, by using this
(perhaps anti)pattern.

> 
> > +		if (!node->kobj) {
> > +			sgx_numa_exit();
> > +			return false;
> > +		}
> > +
> > +		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);
> 
> And you raced with userspace and lost.
> 
> Wait, you have a kobject _just_ for a subdirectory name?  Why?  Use a
> named attribute group, that's exactly what that is for.
> 
> Properly attach your attributes to the device you have, don't do extra
> work and complex code that you do not have to at all.

Here the reference was hugetlb_sysfs_init() and hugetlb_sysfs_add_hstate().
Agreed, that a named group would make a lot more sense.

> thanks,
> 
> greg k-h

/Jarkko

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

[Greg Kroah-Hartman]

On Tue, Sep 28, 2021 at 09:20:41PM +0300, Jarkko Sakkinen wrote:
> On Tue, 2021-09-28 at 06:54 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Sep 28, 2021 at 06:13:50AM +0300, 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.
> > > Calculate these values by summing up EPC section sizes for each node
> > > during the driver initalization.
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > 
> > > v6:
> > > * Initialize node->size to zero in sgx_setup_epc_section(), when the
> > >   node is first accessed. The bug report:
> > >   https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@intel.com/
> > > 
> > > v5:
> > > * A new patch based on the discussion on
> > >   https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> > > 
> > >  Documentation/x86/sgx.rst      | 14 ++++++
> > 
> > sysfs files have to be documented in Documentation/ABI/ so that they can
> > be automatically checked, and added to the documentation output
> > properly.  Please do that here as well.
> 
> Right, I'll document it to sysfs-devices-node.
> 
> > >  arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
> > >  arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
> > >  3 files changed, 107 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..4f1e3b5e3d14 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
> > >  			spin_lock_init(&sgx_numa_nodes[nid].lock);
> > >  			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_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++;
> > >  	}
> > > @@ -790,6 +792,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)
> > 
> > Why are you dealing with a "raw" kobject?  Shouldn't you have a device
> > and use a device attribute?
> > 
> > > +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);
> > 
> > You just "broke" the tree by putting a raw kobject below a struct
> > device.  Please do not do that.
> 
> I looked how hugetlb was implemented as a starting point, since it is
> existing mainline code, i.e. in mm/hugetlb.c:
> 
> - hugetlb_register_node()
> - hugetlb_register_all_nodes()
> - HSTATE_ATTR_RO()
> 
> hugetlb code attaches raw kobjects to the node device, by using this
> (perhaps anti)pattern.

Never look at the memory code for how to use sysfs "properly" :)

It deals with raw kobjects as it is not tied into the driver model at
all.  You are, so do not try to route around it, userspace will not be
happy with you if you do so.

> > 
> > > +		if (!node->kobj) {
> > > +			sgx_numa_exit();
> > > +			return false;
> > > +		}
> > > +
> > > +		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);
> > 
> > And you raced with userspace and lost.
> > 
> > Wait, you have a kobject _just_ for a subdirectory name?  Why?  Use a
> > named attribute group, that's exactly what that is for.
> > 
> > Properly attach your attributes to the device you have, don't do extra
> > work and complex code that you do not have to at all.
> 
> Here the reference was hugetlb_sysfs_init() and hugetlb_sysfs_add_hstate().
> Agreed, that a named group would make a lot more sense.

Again, ignore all sysfs code for the memory subsystem please :)

thanks,

greg k-h

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

[Jarkko Sakkinen]

On Wed, 2021-09-29 at 09:22 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 28, 2021 at 09:20:41PM +0300, Jarkko Sakkinen wrote:
> > On Tue, 2021-09-28 at 06:54 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Sep 28, 2021 at 06:13:50AM +0300, 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.
> > > > Calculate these values by summing up EPC section sizes for each node
> > > > during the driver initalization.
> > > > 
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > 
> > > > v6:
> > > > * Initialize node->size to zero in sgx_setup_epc_section(), when the
> > > >   node is first accessed. The bug report:
> > > >   https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@intel.com/
> > > > 
> > > > v5:
> > > > * A new patch based on the discussion on
> > > >   https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/T/#t
> > > > 
> > > >  Documentation/x86/sgx.rst      | 14 ++++++
> > > 
> > > sysfs files have to be documented in Documentation/ABI/ so that they can
> > > be automatically checked, and added to the documentation output
> > > properly.  Please do that here as well.
> > 
> > Right, I'll document it to sysfs-devices-node.
> > 
> > > >  arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
> > > >  arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
> > > >  3 files changed, 107 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..4f1e3b5e3d14 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
> > > >  			spin_lock_init(&sgx_numa_nodes[nid].lock);
> > > >  			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_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++;
> > > >  	}
> > > > @@ -790,6 +792,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)
> > > 
> > > Why are you dealing with a "raw" kobject?  Shouldn't you have a device
> > > and use a device attribute?
> > > 
> > > > +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);
> > > 
> > > You just "broke" the tree by putting a raw kobject below a struct
> > > device.  Please do not do that.
> > 
> > I looked how hugetlb was implemented as a starting point, since it is
> > existing mainline code, i.e. in mm/hugetlb.c:
> > 
> > - hugetlb_register_node()
> > - hugetlb_register_all_nodes()
> > - HSTATE_ATTR_RO()
> > 
> > hugetlb code attaches raw kobjects to the node device, by using this
> > (perhaps anti)pattern.
> 
> Never look at the memory code for how to use sysfs "properly" :)
> 
> It deals with raw kobjects as it is not tied into the driver model at
> all.  You are, so do not try to route around it, userspace will not be
> happy with you if you do so.
> 
> > > > +		if (!node->kobj) {
> > > > +			sgx_numa_exit();
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);
> > > 
> > > And you raced with userspace and lost.
> > > 
> > > Wait, you have a kobject _just_ for a subdirectory name?  Why?  Use a
> > > named attribute group, that's exactly what that is for.
> > > 
> > > Properly attach your attributes to the device you have, don't do extra
> > > work and complex code that you do not have to at all.
> > 
> > Here the reference was hugetlb_sysfs_init() and hugetlb_sysfs_add_hstate().
> > Agreed, that a named group would make a lot more sense.
> 
> Again, ignore all sysfs code for the memory subsystem please :)

OK, cool, thank you. I can do that.

> 
> thanks,
> 
> greg k-h

/Jarkko