x86/sgx: Add the missing ifdef for sgx_set_attribute()

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 <[email protected]>

---
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(-)

Documentation/x86/sgx.rst +0 -0

index dd0ac96ff9ef..68ee171e1d8f 100644

Quoted 33 lines · 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 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 <[email protected]>

---
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(-)

Documentation/x86/sgx.rst +0 -0

index dd0ac96ff9ef..68ee171e1d8f 100644

Documentation/x86/sgx.rst user wants to deploy SGX applications both on the host and in guests · line 258

Quoted 9 lines

Documentation/x86/sgx.rst +6 -0

@@ -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.

Quoted 6 lines

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",

Quoted 13 lines · Jarkko Sakkinen

Quoted 6 lines

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

Quoted 8 lines

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.

Quoted 26 lines · Dave Hansen

Quoted 13 lines · Jarkko Sakkinen

Quoted 6 lines

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

Quoted 8 lines

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:

Quoted 4 lines

Documentation/x86/meminfo.rst

along with whatever other arches provide their own fields too.

Yes, either way works. Thanks.

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 <[email protected]>

---
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(-)

Documentation/x86/sgx.rst +6 -0

index dd0ac96ff9ef..68ee171e1d8f 100644
@@ -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.

arch/x86/include/asm/sgx.h +2 -0

index 996e56590a10..d8e526b5487b 100644
@@ -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);
 

arch/x86/kernel/cpu/sgx/main.c +4 -1

index 63d3de02bbcc..1fe26a8e80dc 100644
@@ -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. */

arch/x86/mm/pat/set_memory.c +0 -0

 }
 #else
 static inline void split_page_count(int level) { }

Signature · 1 line

2.25.1

Quoted 75 lines · 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 <[email protected]>

---
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(-)

Documentation/x86/sgx.rst +6 -0

index dd0ac96ff9ef..68ee171e1d8f 100644
@@ -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.

arch/x86/include/asm/sgx.h +2 -0

index 996e56590a10..d8e526b5487b 100644
@@ -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);
 

arch/x86/kernel/cpu/sgx/main.c +4 -1

index 63d3de02bbcc..1fe26a8e80dc 100644
@@ -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(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 2 lines

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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?

Quoted 4 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

 	return true;
 }
 

arch/x86/mm/pat/set_memory.c line 32

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +1 -0

index ad8a5c586a35..82bb09c298de 100644
@@ -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?

Quoted 13 lines

Quoted 13 lines

arch/x86/kernel/cpu/sgx/main.c +4 -1

-/* 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(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 2 lines

arch/x86/kernel/cpu/sgx/main.c +2 -0

+	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.

Quoted 5 lines

Quoted 5 lines

arch/x86/kernel/cpu/sgx/main.c +0 -0

 	return true;
 }
 

Quoted 12 lines

arch/x86/mm/pat/set_memory.c line 32

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +1 -0

index ad8a5c586a35..82bb09c298de 100644
@@ -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?

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

arch/x86/kernel/cpu/sgx/main.c +4 -1

-/* 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. */

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 4 lines

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.

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

arch/x86/kernel/cpu/sgx/main.c +4 -1

-/* 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. */

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 35 lines · On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

arch/x86/kernel/cpu/sgx/main.c +4 -1

-/* 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. */

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

arch/x86/kernel/cpu/sgx/main.c +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 41 lines · Kai Huang

Quoted 39 lines · On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

(quoted diff) +4 -1

-/* 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. */

Review static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

(quoted diff) +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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?

Quoted 1 line

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.

Quoted 44 lines · On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote:

Quoted 42 lines · Kai Huang

Quoted 31 lines · On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

(quoted diff) +4 -1

-/* 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. */

Review static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

(quoted diff) +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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).

Quoted 50 lines · Kai Huang

Quoted 48 lines · On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote:

Quoted 36 lines · Kai Huang

Quoted 35 lines · On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

(quoted diff) +4 -1

-/* 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. */

Review static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

(quoted diff) +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 51 lines · On Thu, 02 Sep 2021 15:15:51 +0300 Jarkko Sakkinen wrote:

Quoted 50 lines · Kai Huang

Quoted 48 lines · On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote:

Quoted 36 lines · Kai Huang

Quoted 35 lines · On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

Quoted 21 lines · Kai Huang

Quoted 21 lines

Quoted 20 lines

Quoted 9 lines

(quoted diff) +4 -1

-/* 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. */

Review static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, · line 663

Quoted 5 lines

(quoted diff) +2 -0

@@ -656,6 +659,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;
+

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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?

arch/x86/kernel/cpu/sgx/main.c +0 -0

index 63d3de02bbcc..270f6103b6c0 100644

Similarly as sgx_virt_*, decorate sgx_set_attribute() with ifdef, so that calling it without appropraite config flags, will cause a compilation error, and not a linking error.

Quoted 3 lines · Jarkko Sakkinen

Similarly as sgx_virt_*, decorate sgx_set_attribute() with ifdef, so that calling it without appropraite config flags, will cause a compilation error, and not a linking error.

Please explain what exactly is this fixing. IOW, how can I reproduce the failure?

I made this change because I'm including the header to set_memory.c, and

It's also incoherent that KVM specific functions are compilation flagged

Quoted 1 line · Jarkko Sakkinen

I made this change because I'm including the header to set_memory.c, and

This is something you're doing locally, I presume. If so, you can keep this patch local too.

Quoted 1 line

It's also incoherent that KVM specific functions are compilation flagged

They don't really have to be - they're just declarations.

Is your preference is to have in set_memory.c:

Quoted 1 line · Jarkko Sakkinen

Is your preference is to have in set_memory.c:

My preference is to take fixes only for actual problems which can be triggered with some config - not something you're doing locally.

The actual problem is to use it in set_memory.c.

This the unsplit version:

https://lore.kernel.org/linux-sgx/[email protected]/

Should I just squash them again into one patch?