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

[Jarkko Sakkinen <jarkko@kernel.org>]

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