[PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

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

Fixes: b3754e5d3da3 ("x86/sgx: Move provisioning device creation out of SGX driver")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

---
v3:
* Since CONFIG_X86_SGX_KVM depends on CONFIG_X86_SGX surround everything
  with CONFIG_X86_SGX config flag.
  Link: https://lore.kernel.org/linux-sgx/YR6Bs2twT4EK%2FjUK@google.com/
---
 arch/x86/include/asm/sgx.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 05f3e21f01a7..996e56590a10 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -365,6 +365,11 @@ struct sgx_sigstruct {
  * comment!
  */
 
+#ifdef CONFIG_X86_SGX
+
+int sgx_set_attribute(unsigned long *allowed_attributes,
+		      unsigned int attribute_fd);
+
 #ifdef CONFIG_X86_SGX_KVM
 int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
 		     int *trapnr);
@@ -372,7 +377,6 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
 		   void __user *secs, u64 *lepubkeyhash, int *trapnr);
 #endif
 
-int sgx_set_attribute(unsigned long *allowed_attributes,
-		      unsigned int attribute_fd);
+#endif /* CONFIG_X86_SGX */
 
 #endif /* _ASM_X86_SGX_H */
-- 
2.25.1

[PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[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 <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(&section->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) { }
-- 
2.25.1

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Randy Dunlap]

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.

-- 
~Randy

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Dave Hansen]

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.

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Randy Dunlap]

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.

-- 
~Randy

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Kai Huang]

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(&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?

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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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(&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?

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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Kai Huang]

> > > -/* 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);
> > >  	}
> > >  
> > > +	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.

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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(&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?
> > 
> > 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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Kai Huang]

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(&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?
> > > 
> > > 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.
>

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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(&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?
> > > > 
> > > > 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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Kai Huang]

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(&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?
> > > > > 
> > > > > 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).

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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(&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?
> > > > > > 
> > > > > > 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

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Kai Huang]

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(&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?
> > > > > > > 
> > > > > > > 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;
 }

Re: [PATCH v3 2/2] x86/sgx: Add SGX_MemTotal to /proc/meminfo

[Jarkko Sakkinen]

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(&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?
> > > > > > > > 
> > > > > > > > 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

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Borislav Petkov]

On Thu, Aug 26, 2021 at 02:52:32AM +0300, Jarkko Sakkinen wrote:
> 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?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Jarkko Sakkinen]

On Thu, 2021-08-26 at 11:58 +0200, Borislav Petkov wrote:
> On Thu, Aug 26, 2021 at 02:52:32AM +0300, Jarkko Sakkinen wrote:
> > 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?

You're right, fixes tag is not necessary.

I made this change because I'm including the header to set_memory.c, and
IMHO it is better to make sure when possible that we get compilation errors
than linker errors, if for some reason kernel did not have SGX support.

It's also incoherent that KVM specific functions are compilation flagged but
sgx_set_attribute() is not.

/Jarkko

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Borislav Petkov]

On Thu, Aug 26, 2021 at 07:08:07PM +0300, Jarkko Sakkinen wrote:
> 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.

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

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Jarkko Sakkinen]

On Thu, 2021-08-26 at 18:35 +0200, Borislav Petkov wrote:
> On Thu, Aug 26, 2021 at 07:08:07PM +0300, Jarkko Sakkinen wrote:
> > 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.
> 
> > It's also incoherent that KVM specific functions are compilation flagged
> 
> They don't really have to be - they're just declarations.

Let me check.

Is your preference is to have in set_memory.c:

#ifdef CONFIG_X86_SGX
#include <asm/sgx.h>
#endif

instead of doing this in uapi/asm/sgx.h?

/Jarkko

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Borislav Petkov]

On Thu, Aug 26, 2021 at 08:11:27PM +0300, Jarkko Sakkinen wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Jarkko Sakkinen]

On Thu, 2021-08-26 at 19:24 +0200, Borislav Petkov wrote:
> On Thu, Aug 26, 2021 at 08:11:27PM +0300, Jarkko Sakkinen wrote:
> > 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/20210818132509.545997-1-jarkko@kernel.org/

Should I just squash them again into one patch?

I did the split because of earlier review comments.

/Jarkko

Re: [PATCH v3 1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute()

[Borislav Petkov]

On Thu, Aug 26, 2021 at 08:31:13PM +0300, Jarkko Sakkinen wrote:
> The actual problem is to use it in set_memory.c.

So I asked you in the first mail:

"Please explain what exactly is this fixing."

> This the unsplit version:
> 
> https://lore.kernel.org/linux-sgx/20210818132509.545997-1-jarkko@kernel.org/

But you're still feeding me some pieces of the puzzle piecemeal.

> Should I just squash them again into one patch?

You should explain *why* you're fixing whatever you're fixing and your
commit message should explain exactly how the bug is triggered so that
the reader of that commit message can reproduce it on their end.

Otherwise it'll get ignored until you do it right.

-- 
Regards/Gruss,
    Boris.

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