| Message ID | 20210405232653.33680-2-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section() | expand |
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.
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
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.
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
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.
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
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.
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
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(§ion->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:
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(-)