Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section()

[Borislav Petkov <bp@alien8.de>]

On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote:
> Now that the sanitization process will make pages available by calling
> sgx_free_epc_page(), sgx_setup_epc_section() should not touch to
> sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the
> number of actual free pages. Simply, remove the statement.
> 
> Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 13a7599ce7d4..7df7048cb1c9 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -657,7 +657,6 @@ 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_free_pages += nr_pages;
>  	return true;
>  }
>  

First of all, I don't know how I didn't catch this:

/* The free page list lock protected variables prepend the lock. */
static unsigned long sgx_nr_free_pages;

I need some sort of translator to understand what this comment means. I
can guess what is trying to tell me.

Which leads to my question: what is sgx_nr_free_pages supposed to denote?

Because I understand the callpath

sgx_page_cache_init
...
for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
	...
	sgx_setup_epc_section
	...
		sgx_nr_free_pages += nr_pages;

as adding the number of pages of each new EPC section to the total
number of the free pages. Unless that variable accounts something else.

So what does this variable actually mean?

-- 
Regards/Gruss,
    Boris.

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