[PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Jarkko Sakkinen]

Now that the sanitization process will make pages available by calling
sgx_free_epc_page(), sgx_setup_epc_section() should not touch to
sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the
number of actual free pages. Simply, remove the statement.

Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 arch/x86/kernel/cpu/sgx/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 13a7599ce7d4..7df7048cb1c9 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 
-	sgx_nr_free_pages += nr_pages;
 	return true;
 }
 
-- 
2.31.1

[PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Jarkko Sakkinen]

Add two debugs attributes:

* /sys/kernel/debug/x86/sgx_nr_all_pages
* /sys/kernel/debug/x86/sgx_nr_free_pages

These provide useful statistics for testing purposes.

E.g. on a NUC7CJYH2, when no enclaves are running, and EPC set to 32 MB:

$ sudo cat /sys/kernel/debug/x86/sgx_nr_all_pages
5632

$ sudo cat /sys/kernel/debug/x86/sgx_nr_free_pages
5632

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

v2:
* Use debugfs_create_ulong():
  https://lore.kernel.org/linux-sgx/57c18e08-3e36-b5b3-aaba-9a21b75a1613@intel.com/

 arch/x86/kernel/cpu/sgx/main.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 7df7048cb1c9..07bad864c531 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*  Copyright(c) 2016-20 Intel Corporation. */
 
+#include <linux/debugfs.h>
 #include <linux/freezer.h>
 #include <linux/highmem.h>
 #include <linux/kthread.h>
@@ -25,7 +26,10 @@ static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
 static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
-/* The free page list lock protected variables prepend the lock. */
+/* The number of EPC pages in total in all nodes. */
+static unsigned long sgx_nr_all_pages;
+
+/* The number of free EPC pages in all nodes. */
 static unsigned long sgx_nr_free_pages;
 
 /* Nodes with one or more EPC sections. */
@@ -657,6 +661,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 
+	sgx_nr_all_pages += nr_pages;
+
 	return true;
 }
 
@@ -750,6 +756,11 @@ static int __init sgx_init(void)
 	if (ret)
 		goto err_kthread;
 
+#ifdef CONFIG_DEBUG_FS
+	debugfs_create_ulong("sgx_nr_all_pages", 0400, arch_debugfs_dir, NULL, &sgx_nr_all_pages);
+	debugfs_create_ulong("sgx_nr_free_pages", 0400, arch_debugfs_dir, NULL, &sgx_nr_free_pages);
+#endif /* CONFIG_DEBUG_FS */
+
 	return 0;
 
 err_kthread:
-- 
2.31.1

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Borislav Petkov]

On Tue, Apr 06, 2021 at 02:26:53AM +0300, Jarkko Sakkinen wrote:
> Add two debugs attributes:
> 
> * /sys/kernel/debug/x86/sgx_nr_all_pages
> * /sys/kernel/debug/x86/sgx_nr_free_pages

Definitely under /sys/kernel/debug/x86/sgx/...
				      ^^^^^^

> These provide useful statistics for testing purposes.

Testing what exactly?

Also, if those are EPC pages, why isn't "epc" in the name?

> E.g. on a NUC7CJYH2, when no enclaves are running, and EPC set to 32 MB:
> 
> $ sudo cat /sys/kernel/debug/x86/sgx_nr_all_pages
> 5632
> 
> $ sudo cat /sys/kernel/debug/x86/sgx_nr_free_pages
> 5632

I have no clue what that is useful for. You want to account how many of
the EPC pages on all nodes are in use? What for?

Are those globally useful for people? If so, they need to go to sysfs
along with documentation what they do.

If not, you can keep this patch in your tree for your own testing.

> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> 
> v2:
> * Use debugfs_create_ulong():
>   https://lore.kernel.org/linux-sgx/57c18e08-3e36-b5b3-aaba-9a21b75a1613@intel.com/
> 
>  arch/x86/kernel/cpu/sgx/main.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 7df7048cb1c9..07bad864c531 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*  Copyright(c) 2016-20 Intel Corporation. */
>  
> +#include <linux/debugfs.h>
>  #include <linux/freezer.h>
>  #include <linux/highmem.h>
>  #include <linux/kthread.h>
> @@ -25,7 +26,10 @@ static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
>  static LIST_HEAD(sgx_active_page_list);
>  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
>  
> -/* The free page list lock protected variables prepend the lock. */
> +/* The number of EPC pages in total in all nodes. */
> +static unsigned long sgx_nr_all_pages;
> +
> +/* The number of free EPC pages in all nodes. */
>  static unsigned long sgx_nr_free_pages;

Ok, you're fixing the comment here. Good.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Jarkko Sakkinen]

On Wed, Apr 07, 2021 at 05:56:36PM +0200, Borislav Petkov wrote:
> On Tue, Apr 06, 2021 at 02:26:53AM +0300, Jarkko Sakkinen wrote:
> > Add two debugs attributes:
> > 
> > * /sys/kernel/debug/x86/sgx_nr_all_pages
> > * /sys/kernel/debug/x86/sgx_nr_free_pages
> 
> Definitely under /sys/kernel/debug/x86/sgx/...
> 				      ^^^^^^
> 
> > These provide useful statistics for testing purposes.
> 
> Testing what exactly?
> 
> Also, if those are EPC pages, why isn't "epc" in the name?

When debugging the SGX code it is useful to quickly check the amount of
EPC pages, and also total amount of EPC available.

I left out "epc" because they are already prefixed with "sgx_".

> > E.g. on a NUC7CJYH2, when no enclaves are running, and EPC set to 32 MB:
> > 
> > $ sudo cat /sys/kernel/debug/x86/sgx_nr_all_pages
> > 5632
> > 
> > $ sudo cat /sys/kernel/debug/x86/sgx_nr_free_pages
> > 5632
> 
> I have no clue what that is useful for. You want to account how many of
> the EPC pages on all nodes are in use? What for?
> 
> Are those globally useful for people? If so, they need to go to sysfs
> along with documentation what they do.
> 
> If not, you can keep this patch in your tree for your own testing.

E.g. when stress testing this might be useful information to scale the
workload for example, or even sample the number of EPC pages. When
otherwise testing this might be useful to catch any leaks. I created
1/2 based on what I saw with these variables.

debugfs was my first shot, but for sure these could be sysfs.

> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > 
> > v2:
> > * Use debugfs_create_ulong():
> >   https://lore.kernel.org/linux-sgx/57c18e08-3e36-b5b3-aaba-9a21b75a1613@intel.com/
> > 
> >  arch/x86/kernel/cpu/sgx/main.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 7df7048cb1c9..07bad864c531 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*  Copyright(c) 2016-20 Intel Corporation. */
> >  
> > +#include <linux/debugfs.h>
> >  #include <linux/freezer.h>
> >  #include <linux/highmem.h>
> >  #include <linux/kthread.h>
> > @@ -25,7 +26,10 @@ static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
> >  static LIST_HEAD(sgx_active_page_list);
> >  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> >  
> > -/* The free page list lock protected variables prepend the lock. */
> > +/* The number of EPC pages in total in all nodes. */
> > +static unsigned long sgx_nr_all_pages;
> > +
> > +/* The number of free EPC pages in all nodes. */
> >  static unsigned long sgx_nr_free_pages;
> 
> Ok, you're fixing the comment here. Good.

Should that be part of the first patch?

> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

/Jarkko

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Borislav Petkov]

On Wed, Apr 07, 2021 at 07:09:11PM +0300, Jarkko Sakkinen wrote:
> I left out "epc" because they are already prefixed with "sgx_".

Are there any other "page" types which are going to be figurating in
some pseudofs or is "sgx" == "epc" in this case?

> debugfs was my first shot, but for sure these could be sysfs.

Ok, let's keep it in debugfs for now, it can always be made an ABI later
and moved to sysfs. But pls document what those are and what they do and
that when in debugfs, there are no guarantees that these interfaces will
be there in the future.

> Should that be part of the first patch?

Yes pls.

Thx.


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Jarkko Sakkinen]

On Wed, Apr 07, 2021 at 06:15:33PM +0200, Borislav Petkov wrote:
> On Wed, Apr 07, 2021 at 07:09:11PM +0300, Jarkko Sakkinen wrote:
> > I left out "epc" because they are already prefixed with "sgx_".
> 
> Are there any other "page" types which are going to be figurating in
> some pseudofs or is "sgx" == "epc" in this case?
> 
> > debugfs was my first shot, but for sure these could be sysfs.
> 
> Ok, let's keep it in debugfs for now, it can always be made an ABI later
> and moved to sysfs. But pls document what those are and what they do and
> that when in debugfs, there are no guarantees that these interfaces will
> be there in the future.

I think these attributes are quite useful information to have available so
I'd go actually doing sysfs attributes and create
Documentation/ABI/stable/sysfs-driver-sgx to document them.

Given that they would go then to the sysfs directory of the driver, then
probably the legit names for the attributes ought to be:

- nr_all_epc_pages
- nr_free_epc_pages

What do you think?

PS. One useful case that I forgot to mention is that I use these to give
idea what I gave EPC size in the BIOS. Now my EPC is set to 32 MB, and
these report 20 MB of EPC pages. It's because other metadata (e.g. EPCM
containing page attributes) is also stored in this area.

/Jarkko

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Borislav Petkov]

On Thu, Apr 08, 2021 at 11:52:40AM +0300, Jarkko Sakkinen wrote:
> I think these attributes are quite useful information to have available so
> I'd go actually doing sysfs attributes and create
> Documentation/ABI/stable/sysfs-driver-sgx to document them.

  testing/
        This directory documents interfaces that are felt to be stable,
        as the main development of this interface has been completed.

This sounds better for a start. From Documentation/ABI/README.

> Given that they would go then to the sysfs directory of the driver, then
> probably the legit names for the attributes ought to be:
> 
> - nr_all_epc_pages
> - nr_free_epc_pages
> 
> What do you think?

Sounds ok to me.

> PS. One useful case that I forgot to mention is that I use these to give
> idea what I gave EPC size in the BIOS. Now my EPC is set to 32 MB, and
> these report 20 MB of EPC pages. It's because other metadata (e.g. EPCM
> containing page attributes) is also stored in this area.

Just remember to put yourself in the user's shoes and think whether they
make sense to her/him.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Jarkko Sakkinen]

On Thu, Apr 08, 2021 at 11:52:40AM +0300, Jarkko Sakkinen wrote:
> On Wed, Apr 07, 2021 at 06:15:33PM +0200, Borislav Petkov wrote:
> > On Wed, Apr 07, 2021 at 07:09:11PM +0300, Jarkko Sakkinen wrote:
> > > I left out "epc" because they are already prefixed with "sgx_".
> > 
> > Are there any other "page" types which are going to be figurating in
> > some pseudofs or is "sgx" == "epc" in this case?
> > 
> > > debugfs was my first shot, but for sure these could be sysfs.
> > 
> > Ok, let's keep it in debugfs for now, it can always be made an ABI later
> > and moved to sysfs. But pls document what those are and what they do and
> > that when in debugfs, there are no guarantees that these interfaces will
> > be there in the future.
> 
> I think these attributes are quite useful information to have available so
> I'd go actually doing sysfs attributes and create
> Documentation/ABI/stable/sysfs-driver-sgx to document them.
> 
> Given that they would go then to the sysfs directory of the driver, then
> probably the legit names for the attributes ought to be:
> 
> - nr_all_epc_pages
> - nr_free_epc_pages
> 
> What do you think?

Actually I think read-only sysctl attributes would be a better idea.

The rationale for this is that we have two misc devices sgx_enclave and
sgx_provision, and these are global attributes even applicable to KVM.

It does not matter functionality-wise, but API-wise it'd look stupid to
directly associate to sgx_enclave.

I.e. I'd add something along the lines of 

static struct ctl_path x86_sysctl_path[] = {
        { .procname = "kernel", },
	{ .procname = "x86", },
	{ }
};

static struct ctl_table x86_sysctl_table[] = {
	{
		.procname       = "sgx_nr_all_pages",
		.mode           = 0444,
                /* rest ... */
	},
	{
		.procname       = "sgx_nr_free_pages",
		.mode           = 0444,
                /* rest ... */
	},
	{ }
};

And write Documentation/x86/proc.rst.

/Jarkko

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Borislav Petkov]

On Thu, Apr 08, 2021 at 12:13:21PM +0300, Jarkko Sakkinen wrote:
> Actually I think read-only sysctl attributes would be a better idea.

I still think debugfs is the right *start* for this: you play with them,
see what makes sense and what not, tweak them, etc, and then you cast
them in stone.

Not cast them in stone and see if anyone is even interested. So pls keep
them in debugfs for now - you can always do whatever, later, when it
turns out that those are useful.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] x86/sgx: Add sgx_nr_{all, free}_pages to the debugfs

[Jarkko Sakkinen]

On Thu, Apr 08, 2021 at 11:32:23AM +0200, Borislav Petkov wrote:
> On Thu, Apr 08, 2021 at 12:13:21PM +0300, Jarkko Sakkinen wrote:
> > Actually I think read-only sysctl attributes would be a better idea.
> 
> I still think debugfs is the right *start* for this: you play with them,
> see what makes sense and what not, tweak them, etc, and then you cast
> them in stone.

Yup. Also better to see what makes sense in the context of virtualization.
Too many (unknown) unknowns to make the right call.

> Not cast them in stone and see if anyone is even interested. So pls keep
> them in debugfs for now - you can always do whatever, later, when it
> turns out that those are useful.

I'll do that.

> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Borislav Petkov]

On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote:
> Now that the sanitization process will make pages available by calling
> sgx_free_epc_page(), sgx_setup_epc_section() should not touch to
> sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the
> number of actual free pages. Simply, remove the statement.
> 
> Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 13a7599ce7d4..7df7048cb1c9 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>  	}
>  
> -	sgx_nr_free_pages += nr_pages;
>  	return true;
>  }
>  

First of all, I don't know how I didn't catch this:

/* The free page list lock protected variables prepend the lock. */
static unsigned long sgx_nr_free_pages;

I need some sort of translator to understand what this comment means. I
can guess what is trying to tell me.

Which leads to my question: what is sgx_nr_free_pages supposed to denote?

Because I understand the callpath

sgx_page_cache_init
...
for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
	...
	sgx_setup_epc_section
	...
		sgx_nr_free_pages += nr_pages;

as adding the number of pages of each new EPC section to the total
number of the free pages. Unless that variable accounts something else.

So what does this variable actually mean?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Jarkko Sakkinen]

On Wed, Apr 07, 2021 at 05:49:34PM +0200, Borislav Petkov wrote:
> On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote:
> > Now that the sanitization process will make pages available by calling
> > sgx_free_epc_page(), sgx_setup_epc_section() should not touch to
> > sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the
> > number of actual free pages. Simply, remove the statement.
> > 
> > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 13a7599ce7d4..7df7048cb1c9 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
> >  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
> >  	}
> >  
> > -	sgx_nr_free_pages += nr_pages;
> >  	return true;
> >  }
> >  
> 
> First of all, I don't know how I didn't catch this:
> 
> /* The free page list lock protected variables prepend the lock. */
> static unsigned long sgx_nr_free_pages;
> 
> I need some sort of translator to understand what this comment means. I
> can guess what is trying to tell me.
> 
> Which leads to my question: what is sgx_nr_free_pages supposed to denote?
> 
> Because I understand the callpath
> 
> sgx_page_cache_init
> ...
> for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
> 	...
> 	sgx_setup_epc_section
> 	...
> 		sgx_nr_free_pages += nr_pages;
> 
> as adding the number of pages of each new EPC section to the total
> number of the free pages. Unless that variable accounts something else.
> 
> So what does this variable actually mean?

It's used for only to trigger watermark for reclaiming. I.e. causes
ksgxd to trigger. And it gives the number of total free EPC pages in
all NUMA nodes.

/Jarkko

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Borislav Petkov]

On Wed, Apr 07, 2021 at 07:03:47PM +0300, Jarkko Sakkinen wrote:
> > Which leads to my question: what is sgx_nr_free_pages supposed to denote?
> > 
> > Because I understand the callpath
> > 
> > sgx_page_cache_init
> > ...
> > for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
> > 	...
> > 	sgx_setup_epc_section
> > 	...
> > 		sgx_nr_free_pages += nr_pages;
> > 
> > as adding the number of pages of each new EPC section to the total
> > number of the free pages. Unless that variable accounts something else.
> > 
> > So what does this variable actually mean?
> 
> It's used for only to trigger watermark for reclaiming. I.e. causes
> ksgxd to trigger. And it gives the number of total free EPC pages in
> all NUMA nodes.

So the callpath I laid out above is adding the number of pages of each
section to the total free EPC pages number.

Why is that wrong and why is your patch needed?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Jarkko Sakkinen]

On Wed, Apr 07, 2021 at 06:18:11PM +0200, Borislav Petkov wrote:
> On Wed, Apr 07, 2021 at 07:03:47PM +0300, Jarkko Sakkinen wrote:
> > > Which leads to my question: what is sgx_nr_free_pages supposed to denote?
> > > 
> > > Because I understand the callpath
> > > 
> > > sgx_page_cache_init
> > > ...
> > > for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
> > > 	...
> > > 	sgx_setup_epc_section
> > > 	...
> > > 		sgx_nr_free_pages += nr_pages;
> > > 
> > > as adding the number of pages of each new EPC section to the total
> > > number of the free pages. Unless that variable accounts something else.
> > > 
> > > So what does this variable actually mean?
> > 
> > It's used for only to trigger watermark for reclaiming. I.e. causes
> > ksgxd to trigger. And it gives the number of total free EPC pages in
> > all NUMA nodes.
> 
> So the callpath I laid out above is adding the number of pages of each
> section to the total free EPC pages number.
> 
> Why is that wrong and why is your patch needed?
 
As part of "x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list"
pages are processed from a global list by ksgxd.

This in turn introduces change to sanitization:

-		if (!ret)
-			list_move(&page->list, &section->page_list);
-		else
+		if (!ret) {
+			/*
+			 * page is now sanitized.  Make it available via the SGX
+			 * page allocator:
+			 */
+			list_del(&page->list);
+			sgx_free_epc_page(page);
+		} else {
+			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
-
-		spin_unlock(&section->lock);
+		}

This is done for the reason that it is best to keep the logic to assign
available-for-use EPC pages to correct NUMA lists in a single location.

The regression is that the sgx_nr_free_pages is also incremented by
sgx_free_epc_pages(), and thus it ends up having double the number of
pages available.

/Jarkko

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Borislav Petkov]

On Thu, Apr 08, 2021 at 11:48:46AM +0300, Jarkko Sakkinen wrote:
> The regression is that the sgx_nr_free_pages is also incremented by
> sgx_free_epc_pages(), and thus it ends up having double the number of
> pages available.

So when you add a new EPC section with sgx_setup_epc_section(), those
new pages in "nr_pages" are initially not going to be accounted
anywhere? Or is that sgx_nr_all_pages? And you do that in your second
patch...

But those new pages coming in *are* free pages so they should be in the
free pages count too, IMHO.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Jarkko Sakkinen]

On Thu, Apr 08, 2021 at 10:56:40AM +0200, Borislav Petkov wrote:
> On Thu, Apr 08, 2021 at 11:48:46AM +0300, Jarkko Sakkinen wrote:
> > The regression is that the sgx_nr_free_pages is also incremented by
> > sgx_free_epc_pages(), and thus it ends up having double the number of
> > pages available.
> 
> So when you add a new EPC section with sgx_setup_epc_section(), those
> new pages in "nr_pages" are initially not going to be accounted
> anywhere? Or is that sgx_nr_all_pages? And you do that in your second
> patch...
> 
> But those new pages coming in *are* free pages so they should be in the
> free pages count too, IMHO.

They are not in the "free_page_list" before sanitization process has put
them to there. So in that way the count is also better in sync with this
fix.

It was even before NUMA patches kind of out-of-sync, i.e. free pages count
was incremented before putting them to the free list, but it didn't matter
that much because sanitization is fairly fast and it only prevented ksgxd
to trigger small time after the booth.

I think I'll send the fix for this now as a separate patch with a better
descriptino so that I can use more time to the attributes and implement
that properly at once.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Borislav Petkov]

On Thu, Apr 08, 2021 at 12:22:56PM +0300, Jarkko Sakkinen wrote:
> They are not in the "free_page_list" before sanitization process has put
> them to there. So in that way the count is also better in sync with this
> fix.

This is the bit of information I was looking for. This needs to be in
the commit message.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette