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

On Wed, 2021-09-01 at 17:47 +1200, Kai Huang wrote:

quoted

On Wed, 01 Sep 2021 08:41:12 +0300 Jarkko Sakkinen wrote:

quoted

On Wed, 2021-09-01 at 17:33 +1200, Kai Huang wrote:

quoted

On Wed, 01 Sep 2021 05:02:45 +0300 Jarkko Sakkinen wrote:

quoted

On Sat, 2021-08-28 at 00:03 +1200, Kai Huang wrote:

quoted

quoted

quoted

quoted

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