| Message ID | 20210825235234.153013-2-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute() | expand |
On 8/25/21 4:52 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 SGX_MemTotal field to /proc/meminfo, which shows the total amount of > usable SGX memory in the system. E.g. with 32 MB reserved for SGX from > BIOS, the printout would be: > > SGX_MemTotal: 22528 kB > > It is less than 32 MB because some of the space is reserved for Enclave > Page Cache Metadata (EPCM), which contains state variables for all the > pages in the Enclave Page Cache (EPC). The latter contains the pages, > which applications can use to create enclaves. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > v2: > * Move ifdef fix for sgx_set_attribute() to a separate patch. > --- > Documentation/x86/sgx.rst | 6 ++++++ > arch/x86/include/asm/sgx.h | 2 ++ > arch/x86/kernel/cpu/sgx/main.c | 7 ++++++- > arch/x86/mm/pat/set_memory.c | 5 +++++ > 4 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst > index dd0ac96ff9ef..68ee171e1d8f 100644 > --- a/Documentation/x86/sgx.rst > +++ b/Documentation/x86/sgx.rst > @@ -250,3 +250,9 @@ 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. > + > +Supplemental fields for /proc/meminfo > +===================================== > + > +SGX_MemTotal > + The total usable SGX protected memory in kilobytes. Hi, I would prefer to see this listed in Documentation/filesystems/proc.rst as an optional field, depending on CONFIG_X86_SGX. Or at least put a reference in proc.rst to this doc file and its supplemental fields. thanks.
On Thu, 26 Aug 2021 02:52:33 +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 SGX_MemTotal field to /proc/meminfo, which shows the total amount of > usable SGX memory in the system. E.g. with 32 MB reserved for SGX from > BIOS, the printout would be: > > SGX_MemTotal: 22528 kB > > It is less than 32 MB because some of the space is reserved for Enclave > Page Cache Metadata (EPCM), which contains state variables for all the > pages in the Enclave Page Cache (EPC). The latter contains the pages, > which applications can use to create enclaves. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > v2: > * Move ifdef fix for sgx_set_attribute() to a separate patch. > --- > Documentation/x86/sgx.rst | 6 ++++++ > arch/x86/include/asm/sgx.h | 2 ++ > arch/x86/kernel/cpu/sgx/main.c | 7 ++++++- > arch/x86/mm/pat/set_memory.c | 5 +++++ > 4 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst > index dd0ac96ff9ef..68ee171e1d8f 100644 > --- a/Documentation/x86/sgx.rst > +++ b/Documentation/x86/sgx.rst > @@ -250,3 +250,9 @@ 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. > + > +Supplemental fields for /proc/meminfo > +===================================== > + > +SGX_MemTotal > + The total usable SGX protected memory in kilobytes. > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index 996e56590a10..d8e526b5487b 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -367,6 +367,8 @@ struct sgx_sigstruct { > > #ifdef CONFIG_X86_SGX > > +extern unsigned long sgx_nr_all_pages; > + > int sgx_set_attribute(unsigned long *allowed_attributes, > unsigned int attribute_fd); > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 63d3de02bbcc..1fe26a8e80dc 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -28,7 +28,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 usable EPC pages in the system. */ > +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. */ > @@ -656,6 +659,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; > + EPC sections can be freed again in sgx_init() after they are successfully initialized, when any further initialization fails (i.e. when fails to create ksgxd, or fails to register /dev/sgx_provision). In which case, I think sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't reset it. Do you need to fix that too? > return true; > } > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > index ad8a5c586a35..82bb09c298de 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -29,6 +29,7 @@ > #include <asm/proto.h> > #include <asm/memtype.h> > #include <asm/set_memory.h> > +#include <asm/sgx.h> How about only include <asm/sgx.h> when CONFIG_X86_SGX is on, then you don't have to do #ifdef CONFIG_X86_SGX changes to sgx.h? > > #include "../mm_internal.h" > > @@ -116,6 +117,10 @@ void arch_report_meminfo(struct seq_file *m) > if (direct_gbpages) > seq_printf(m, "DirectMap1G: %8lu kB\n", > direct_pages_count[PG_LEVEL_1G] << 20); > + > +#if defined(CONFIG_X86_SGX) || defined(CONFIG_X86_SGX_KVM) > + seq_printf(m, "SGX_MemTotal: %8lu kB\n", sgx_nr_all_pages << 2); > +#endif CONFIG_X86_SGX_KVM depends on CONFIG_X86_SGX, so I don't think KVM part is required. Plus, even CONFIG_X86_SGX is on, EPC can be empty, i.e. when SGX FLC is not present and KVM SGX is off too, or when SGX itslef is not present at all. Do you need to add additional check, for instance, only print when sgx_nr_all_pages is not 0? > } > #else > static inline void split_page_count(int level) { } > -- > 2.25.1 >
On Wed, 2021-08-25 at 17:39 -0700, Randy Dunlap wrote: > On 8/25/21 4:52 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 SGX_MemTotal field to /proc/meminfo, which shows the total amount of > > usable SGX memory in the system. E.g. with 32 MB reserved for SGX from > > BIOS, the printout would be: > > > > SGX_MemTotal: 22528 kB > > > > It is less than 32 MB because some of the space is reserved for Enclave > > Page Cache Metadata (EPCM), which contains state variables for all the > > pages in the Enclave Page Cache (EPC). The latter contains the pages, > > which applications can use to create enclaves. > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > v2: > > * Move ifdef fix for sgx_set_attribute() to a separate patch. > > --- > > Documentation/x86/sgx.rst | 6 ++++++ > > arch/x86/include/asm/sgx.h | 2 ++ > > arch/x86/kernel/cpu/sgx/main.c | 7 ++++++- > > arch/x86/mm/pat/set_memory.c | 5 +++++ > > 4 files changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst > > index dd0ac96ff9ef..68ee171e1d8f 100644 > > --- a/Documentation/x86/sgx.rst > > +++ b/Documentation/x86/sgx.rst > > @@ -250,3 +250,9 @@ 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. > > + > > +Supplemental fields for /proc/meminfo > > +===================================== > > + > > +SGX_MemTotal > > + The total usable SGX protected memory in kilobytes. > > Hi, > > I would prefer to see this listed in Documentation/filesystems/proc.rst > as an optional field, depending on CONFIG_X86_SGX. > Or at least put a reference in proc.rst to this doc file and its > supplemental fields. > > thanks. I *can* put it there but I did have reason not to, i.e. these attributes are neither there: DirectMap4k: 3930904 kB DirectMap2M: 29440000 kB DirectMap1G: 1048576 kB And they are implemented in arch specific code. Actually they are undocumented, e.g. $ git grep DirectMap4k arch/powerpc/mm/book3s64/pgtable.c: seq_printf(m, "DirectMap4k: %8lu kB\n", arch/s390/mm/pageattr.c: seq_printf(m, "DirectMap4k: %8lu kB\n", arch/x86/mm/pat/set_memory.c: seq_printf(m, "DirectMap4k: %8lu kB\n", /Jarkko
On Thu, 2021-08-26 at 14:19 +1200, Kai Huang wrote: > On Thu, 26 Aug 2021 02:52:33 +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 SGX_MemTotal field to /proc/meminfo, which shows the total amount of > > usable SGX memory in the system. E.g. with 32 MB reserved for SGX from > > BIOS, the printout would be: > > > > SGX_MemTotal: 22528 kB > > > > It is less than 32 MB because some of the space is reserved for Enclave > > Page Cache Metadata (EPCM), which contains state variables for all the > > pages in the Enclave Page Cache (EPC). The latter contains the pages, > > which applications can use to create enclaves. > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > v2: > > * Move ifdef fix for sgx_set_attribute() to a separate patch. > > --- > > Documentation/x86/sgx.rst | 6 ++++++ > > arch/x86/include/asm/sgx.h | 2 ++ > > arch/x86/kernel/cpu/sgx/main.c | 7 ++++++- > > arch/x86/mm/pat/set_memory.c | 5 +++++ > > 4 files changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst > > index dd0ac96ff9ef..68ee171e1d8f 100644 > > --- a/Documentation/x86/sgx.rst > > +++ b/Documentation/x86/sgx.rst > > @@ -250,3 +250,9 @@ 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. > > + > > +Supplemental fields for /proc/meminfo > > +===================================== > > + > > +SGX_MemTotal > > + The total usable SGX protected memory in kilobytes. > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > index 996e56590a10..d8e526b5487b 100644 > > --- a/arch/x86/include/asm/sgx.h > > +++ b/arch/x86/include/asm/sgx.h > > @@ -367,6 +367,8 @@ struct sgx_sigstruct { > > > > #ifdef CONFIG_X86_SGX > > > > +extern unsigned long sgx_nr_all_pages; > > + > > int sgx_set_attribute(unsigned long *allowed_attributes, > > unsigned int attribute_fd); > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 63d3de02bbcc..1fe26a8e80dc 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -28,7 +28,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 usable EPC pages in the system. */ > > +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. */ > > @@ -656,6 +659,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; > > + > > EPC sections can be freed again in sgx_init() after they are successfully > initialized, when any further initialization fails (i.e. when fails to create > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > reset it. Do you need to fix that too? sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with the meminfo field better too. > > > return true; > > } > > > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > > index ad8a5c586a35..82bb09c298de 100644 > > --- a/arch/x86/mm/pat/set_memory.c > > +++ b/arch/x86/mm/pat/set_memory.c > > @@ -29,6 +29,7 @@ > > #include <asm/proto.h> > > #include <asm/memtype.h> > > #include <asm/set_memory.h> > > +#include <asm/sgx.h> > > How about only include <asm/sgx.h> when CONFIG_X86_SGX is on, then you don't > have to do #ifdef CONFIG_X86_SGX changes to sgx.h? Why do it that way instead of doing it once in sgx.h for every site that wants to include the file? /Jarkko
On 8/26/21 9:17 AM, Jarkko Sakkinen wrote: >> I would prefer to see this listed in Documentation/filesystems/proc.rst >> as an optional field, depending on CONFIG_X86_SGX. >> Or at least put a reference in proc.rst to this doc file and its >> supplemental fields. >> >> thanks. > I *can* put it there but I did have reason not to, i.e. these attributes > are neither there: > > DirectMap4k: 3930904 kB > DirectMap2M: 29440000 kB > DirectMap1G: 1048576 kB > > And they are implemented in arch specific code. > > Actually they are undocumented, e.g. > > $ git grep DirectMap4k > arch/powerpc/mm/book3s64/pgtable.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > arch/s390/mm/pageattr.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > arch/x86/mm/pat/set_memory.c: seq_printf(m, "DirectMap4k: %8lu kB\n", Yeah, we need to add some arch-specific sections to the documentation. That *could* just be a reference over to a new file: Documentation/x86/meminfo.rst along with whatever other arches provide their own fields too.
On 8/26/21 1:27 PM, Dave Hansen wrote: > On 8/26/21 9:17 AM, Jarkko Sakkinen wrote: >>> I would prefer to see this listed in Documentation/filesystems/proc.rst >>> as an optional field, depending on CONFIG_X86_SGX. >>> Or at least put a reference in proc.rst to this doc file and its >>> supplemental fields. >>> >>> thanks. >> I *can* put it there but I did have reason not to, i.e. these attributes >> are neither there: >> >> DirectMap4k: 3930904 kB >> DirectMap2M: 29440000 kB >> DirectMap1G: 1048576 kB >> >> And they are implemented in arch specific code. >> >> Actually they are undocumented, e.g. >> >> $ git grep DirectMap4k >> arch/powerpc/mm/book3s64/pgtable.c: seq_printf(m, "DirectMap4k: %8lu kB\n", >> arch/s390/mm/pageattr.c: seq_printf(m, "DirectMap4k: %8lu kB\n", >> arch/x86/mm/pat/set_memory.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > > Yeah, we need to add some arch-specific sections to the documentation. > That *could* just be a reference over to a new file: > > Documentation/x86/meminfo.rst > > along with whatever other arches provide their own fields too. > Yes, either way works. Thanks.
> > > -/* The free page list lock protected variables prepend the lock. */ > > > +/* The number of usable EPC pages in the system. */ > > > +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. */ > > > @@ -656,6 +659,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; > > > + > > > > EPC sections can be freed again in sgx_init() after they are successfully > > initialized, when any further initialization fails (i.e. when fails to create > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > reset it. Do you need to fix that too? > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > the meminfo field better too. I don't have preference on name. I just think if there's no actual user of EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print number of EPC pages. > > > > > > return true; > > > } > > > > > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > > > index ad8a5c586a35..82bb09c298de 100644 > > > --- a/arch/x86/mm/pat/set_memory.c > > > +++ b/arch/x86/mm/pat/set_memory.c > > > @@ -29,6 +29,7 @@ > > > #include <asm/proto.h> > > > #include <asm/memtype.h> > > > #include <asm/set_memory.h> > > > +#include <asm/sgx.h> > > > > How about only include <asm/sgx.h> when CONFIG_X86_SGX is on, then you don't > > have to do #ifdef CONFIG_X86_SGX changes to sgx.h? > > Why do it that way instead of doing it once in sgx.h for every site that wants > to include the file? Just my preference. You only need sgx_nr_all_pages here, while <asm/sgx.h> has bunch of others such as SGX data structures. It seems it's not worth to include <asm/sgx.h> directly.
On Thu, 2021-08-26 at 15:27 -0700, Randy Dunlap wrote: > On 8/26/21 1:27 PM, Dave Hansen wrote: > > On 8/26/21 9:17 AM, Jarkko Sakkinen wrote: > > > > I would prefer to see this listed in Documentation/filesystems/proc.rst > > > > as an optional field, depending on CONFIG_X86_SGX. > > > > Or at least put a reference in proc.rst to this doc file and its > > > > supplemental fields. > > > > > > > > thanks. > > > I *can* put it there but I did have reason not to, i.e. these attributes > > > are neither there: > > > > > > DirectMap4k: 3930904 kB > > > DirectMap2M: 29440000 kB > > > DirectMap1G: 1048576 kB > > > > > > And they are implemented in arch specific code. > > > > > > Actually they are undocumented, e.g. > > > > > > $ git grep DirectMap4k > > > arch/powerpc/mm/book3s64/pgtable.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > > > arch/s390/mm/pageattr.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > > > arch/x86/mm/pat/set_memory.c: seq_printf(m, "DirectMap4k: %8lu kB\n", > > > > Yeah, we need to add some arch-specific sections to the documentation. > > That *could* just be a reference over to a new file: > > > > Documentation/x86/meminfo.rst > > > > along with whatever other arches provide their own fields too. > > > > Yes, either way works. Thanks. I'm wondering why /sys/devices/system/node/nodeX/meminfo is not documented? At least I could not find its documentation anywhere. It would actually make more sense not to add anything at all /proc/meminfo but rather add SGX_MemTotal to /sys/devices/system/node/nodeX/meminfo because it is easy enough for user space to calculate the final value. It's just a sum of constants (no races). Given the per-node granularity, maybe the arch-specific documentation should be in Documentation/x86/node-meminfo.rst? So I'm thinking to drop /proc/meminfo change and report SGX stats only per-node. /Jarkko
On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > +/* The number of usable EPC pages in the system. */ > > > > +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. */ > > > > @@ -656,6 +659,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; > > > > + > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > initialized, when any further initialization fails (i.e. when fails to create > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > reset it. Do you need to fix that too? > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > the meminfo field better too. > > I don't have preference on name. I just think if there's no actual user of > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > number of EPC pages. I'd presume that you refer to the code, which prints the number of *bytes* in the system because code printing the number of pages does not exist in this patch set. I have troubles the decipher your statement. You think that only if both the driver and KVM are *both* enabled, only then it makes sense to have this information available for sysadmin? I don't get this logic, if I understood what you mean in the first place. /Jarkko
On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > +/* The number of usable EPC pages in the system. */ > > > > > +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. */ > > > > > @@ -656,6 +659,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; > > > > > + > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > reset it. Do you need to fix that too? > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > the meminfo field better too. > > > > I don't have preference on name. I just think if there's no actual user of > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > number of EPC pages. > > I'd presume that you refer to the code, which prints the number of *bytes* in > the system because code printing the number of pages does not exist in this > patch set. > > I have troubles the decipher your statement. > > You think that only if both the driver and KVM are *both* enabled, only then > it makes sense to have this information available for sysadmin? Only if at least one of them is enabled. > > I don't get this logic, if I understood what you mean in the first place. >
On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote: > On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > > +/* The number of usable EPC pages in the system. */ > > > > > > +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. */ > > > > > > @@ -656,6 +659,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; > > > > > > + > > > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > > reset it. Do you need to fix that too? > > > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > > the meminfo field better too. > > > > > > I don't have preference on name. I just think if there's no actual user of > > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > > number of EPC pages. > > > > I'd presume that you refer to the code, which prints the number of *bytes* in > > the system because code printing the number of pages does not exist in this > > patch set. > > > > I have troubles the decipher your statement. > > > > You think that only if both the driver and KVM are *both* enabled, only then > > it makes sense to have this information available for sysadmin? > > Only if at least one of them is enabled. OK, thank you, that does make sense. What would happen if neither is enabled is that SGX_MemTotal would state that there is zero bytes of EPC. I'll add a note to the commit message. It's useful because it give is easy programmatic way to check if SGX is enabled in Linux or not. /Jarkko
On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote: > On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote: > > On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > > > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > > > +/* The number of usable EPC pages in the system. */ > > > > > > > +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. */ > > > > > > > @@ -656,6 +659,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; > > > > > > > + > > > > > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > > > reset it. Do you need to fix that too? > > > > > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > > > the meminfo field better too. > > > > > > > > I don't have preference on name. I just think if there's no actual user of > > > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > > > number of EPC pages. > > > > > > I'd presume that you refer to the code, which prints the number of *bytes* in > > > the system because code printing the number of pages does not exist in this > > > patch set. > > > > > > I have troubles the decipher your statement. > > > > > > You think that only if both the driver and KVM are *both* enabled, only then > > > it makes sense to have this information available for sysadmin? > > > > Only if at least one of them is enabled. > > OK, thank you, that does make sense. > > What would happen if neither is enabled is that SGX_MemTotal would > state that there is zero bytes of EPC. This is the problem I pointed out at the beginning, that (if I read code correctly), it seems your current patch doesn't clear sgx_nr_all_pages when neither is enabled (in sgx_init() in sgx/main.c).
On Wed, 2021-09-01 at 17:47 +1200, Kai Huang wrote: > On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote: > > On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote: > > > On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > > > > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > > > > +/* The number of usable EPC pages in the system. */ > > > > > > > > +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. */ > > > > > > > > @@ -656,6 +659,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; > > > > > > > > + > > > > > > > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > > > > reset it. Do you need to fix that too? > > > > > > > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > > > > the meminfo field better too. > > > > > > > > > > I don't have preference on name. I just think if there's no actual user of > > > > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > > > > number of EPC pages. > > > > > > > > I'd presume that you refer to the code, which prints the number of *bytes* in > > > > the system because code printing the number of pages does not exist in this > > > > patch set. > > > > > > > > I have troubles the decipher your statement. > > > > > > > > You think that only if both the driver and KVM are *both* enabled, only then > > > > it makes sense to have this information available for sysadmin? > > > > > > Only if at least one of them is enabled. > > > > OK, thank you, that does make sense. > > > > What would happen if neither is enabled is that SGX_MemTotal would > > state that there is zero bytes of EPC. > > This is the problem I pointed out at the beginning, that (if I read code > correctly), it seems your current patch doesn't clear sgx_nr_all_pages when > neither is enabled (in sgx_init() in sgx/main.c). It's initialized to zero, so are you talking about fallback when something fails? /Jarkko
On Thu, 02 Sep 2021 15:15:51 +0300 Jarkko Sakkinen wrote: > On Wed, 2021-09-01 at 17:47 +1200, Kai Huang wrote: > > On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote: > > > On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote: > > > > On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > > > > > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > > > > > +/* The number of usable EPC pages in the system. */ > > > > > > > > > +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. */ > > > > > > > > > @@ -656,6 +659,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; > > > > > > > > > + > > > > > > > > > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > > > > > reset it. Do you need to fix that too? > > > > > > > > > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > > > > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > > > > > the meminfo field better too. > > > > > > > > > > > > I don't have preference on name. I just think if there's no actual user of > > > > > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > > > > > number of EPC pages. > > > > > > > > > > I'd presume that you refer to the code, which prints the number of *bytes* in > > > > > the system because code printing the number of pages does not exist in this > > > > > patch set. > > > > > > > > > > I have troubles the decipher your statement. > > > > > > > > > > You think that only if both the driver and KVM are *both* enabled, only then > > > > > it makes sense to have this information available for sysadmin? > > > > > > > > Only if at least one of them is enabled. > > > > > > OK, thank you, that does make sense. > > > > > > What would happen if neither is enabled is that SGX_MemTotal would > > > state that there is zero bytes of EPC. > > > > This is the problem I pointed out at the beginning, that (if I read code > > correctly), it seems your current patch doesn't clear sgx_nr_all_pages when > > neither is enabled (in sgx_init() in sgx/main.c). > > It's initialized to zero, so are you talking about fallback when something > fails? > > /Jarkko Yes, shouldn't you have something similar to below? diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 63d3de02bbcc..270f6103b6c0 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -836,6 +836,7 @@ static int __init sgx_init(void) vfree(sgx_epc_sections[i].pages); memunmap(sgx_epc_sections[i].virt_addr); } + sgx_nr_all_pages = 0; return ret; }
On Fri, 2021-09-03 at 09:56 +1200, Kai Huang wrote: > On Thu, 02 Sep 2021 15:15:51 +0300 Jarkko Sakkinen wrote: > > On Wed, 2021-09-01 at 17:47 +1200, Kai Huang wrote: > > > On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote: > > > > On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote: > > > > > On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote: > > > > > > On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote: > > > > > > > > > > -/* The free page list lock protected variables prepend the lock. */ > > > > > > > > > > +/* The number of usable EPC pages in the system. */ > > > > > > > > > > +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. */ > > > > > > > > > > @@ -656,6 +659,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; > > > > > > > > > > + > > > > > > > > > > > > > > > > > > EPC sections can be freed again in sgx_init() after they are successfully > > > > > > > > > initialized, when any further initialization fails (i.e. when fails to create > > > > > > > > > ksgxd, or fails to register /dev/sgx_provision). In which case, I think > > > > > > > > > sgx_nr_all_pages should also be cleared. But current sgx_init() seems doesn't > > > > > > > > > reset it. Do you need to fix that too? > > > > > > > > > > > > > > > > sgx_nr_all_pages tells just the total pages in the system, i.e. it's a constant. > > > > > > > > > > > > > > > > Maybe a rename to "sgx_nr_total_pages" would be a good idea? Would match with > > > > > > > > the meminfo field better too. > > > > > > > > > > > > > > I don't have preference on name. I just think if there's no actual user of > > > > > > > EPC (when both driver and KVM SGX cannot be enabled), it's pointless to print > > > > > > > number of EPC pages. > > > > > > > > > > > > I'd presume that you refer to the code, which prints the number of *bytes* in > > > > > > the system because code printing the number of pages does not exist in this > > > > > > patch set. > > > > > > > > > > > > I have troubles the decipher your statement. > > > > > > > > > > > > You think that only if both the driver and KVM are *both* enabled, only then > > > > > > it makes sense to have this information available for sysadmin? > > > > > > > > > > Only if at least one of them is enabled. > > > > > > > > OK, thank you, that does make sense. > > > > > > > > What would happen if neither is enabled is that SGX_MemTotal would > > > > state that there is zero bytes of EPC. > > > > > > This is the problem I pointed out at the beginning, that (if I read code > > > correctly), it seems your current patch doesn't clear sgx_nr_all_pages when > > > neither is enabled (in sgx_init() in sgx/main.c). > > > > It's initialized to zero, so are you talking about fallback when something > > fails? > > > > /Jarkko > > Yes, shouldn't you have something similar to below? > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 63d3de02bbcc..270f6103b6c0 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -836,6 +836,7 @@ static int __init sgx_init(void) > vfree(sgx_epc_sections[i].pages); > memunmap(sgx_epc_sections[i].virt_addr); > } > + sgx_nr_all_pages = 0; > > return ret; > } Yeah, something along the lines. Thanks, this was really good remark. The fallback path is clearly broken. /Jarkko
diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst index dd0ac96ff9ef..68ee171e1d8f 100644 --- a/Documentation/x86/sgx.rst +++ b/Documentation/x86/sgx.rst @@ -250,3 +250,9 @@ 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. + +Supplemental fields for /proc/meminfo +===================================== + +SGX_MemTotal + The total usable SGX protected memory in kilobytes. diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 996e56590a10..d8e526b5487b 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -367,6 +367,8 @@ struct sgx_sigstruct { #ifdef CONFIG_X86_SGX +extern unsigned long sgx_nr_all_pages; + int sgx_set_attribute(unsigned long *allowed_attributes, unsigned int attribute_fd); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 63d3de02bbcc..1fe26a8e80dc 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -28,7 +28,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 usable EPC pages in the system. */ +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. */ @@ -656,6 +659,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; } diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index ad8a5c586a35..82bb09c298de 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -29,6 +29,7 @@ #include <asm/proto.h> #include <asm/memtype.h> #include <asm/set_memory.h> +#include <asm/sgx.h> #include "../mm_internal.h" @@ -116,6 +117,10 @@ void arch_report_meminfo(struct seq_file *m) if (direct_gbpages) seq_printf(m, "DirectMap1G: %8lu kB\n", direct_pages_count[PG_LEVEL_1G] << 20); + +#if defined(CONFIG_X86_SGX) || defined(CONFIG_X86_SGX_KVM) + seq_printf(m, "SGX_MemTotal: %8lu kB\n", sgx_nr_all_pages << 2); +#endif } #else static inline void split_page_count(int level) { }
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 SGX_MemTotal field to /proc/meminfo, which shows the total amount of usable SGX memory in the system. E.g. with 32 MB reserved for SGX from BIOS, the printout would be: SGX_MemTotal: 22528 kB It is less than 32 MB because some of the space is reserved for Enclave Page Cache Metadata (EPCM), which contains state variables for all the pages in the Enclave Page Cache (EPC). The latter contains the pages, which applications can use to create enclaves. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v2: * Move ifdef fix for sgx_set_attribute() to a separate patch. --- Documentation/x86/sgx.rst | 6 ++++++ arch/x86/include/asm/sgx.h | 2 ++ arch/x86/kernel/cpu/sgx/main.c | 7 ++++++- arch/x86/mm/pat/set_memory.c | 5 +++++ 4 files changed, 19 insertions(+), 1 deletion(-)