Re: [PATCH v1 1/3] x86/mm: Centralize PMD flags in sme_encrypt_kernel()

[Tom Lendacky]

On 12/20/2017 1:13 PM, Borislav Petkov wrote:
> On Thu, Dec 07, 2017 at 05:33:52PM -0600, Tom Lendacky wrote:
>> In preparation for encrypting more than just the kernel during early
>> boot processing, centralize the use of the PMD flag settings based
>> on the type of mapping desired.  When 4KB aligned encryption is added,
>> this will allow either PTE flags or large page PMD flags to be used
>> without requiring the caller to adjust.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> ---
>>  arch/x86/mm/mem_encrypt.c |  109 +++++++++++++++++++++++++--------------------
>>  1 file changed, 60 insertions(+), 49 deletions(-)
>>
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index d9a9e9f..2d8404b 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -464,6 +464,8 @@ void swiotlb_set_mem_attributes(void *vaddr, unsigned long size)
>>  	set_memory_decrypted((unsigned long)vaddr, size >> PAGE_SHIFT);
>>  }
>>  
>> +static void *pgtable_area;
> 
> Ewww, a global variable which gets manipulated by functions. Can we not
> do that pls?
> 
> sme_populate_pgd() used to return it. Why change that?

It was starting to get pretty hairy with all the parameters and what was
needed to be returned when the second patch was introduced.  I'll look at
what I can do to avoid the global, maybe pass in the address of the
variable for updating within the function or combining the parameters into
a struct.

> 
>> +
>>  static void __init sme_clear_pgd(pgd_t *pgd_base, unsigned long start,
>>  				 unsigned long end)
>>  {
>> @@ -484,10 +486,16 @@ static void __init sme_clear_pgd(pgd_t *pgd_base, unsigned long start,
>>  #define PGD_FLAGS	_KERNPG_TABLE_NOENC
>>  #define P4D_FLAGS	_KERNPG_TABLE_NOENC
>>  #define PUD_FLAGS	_KERNPG_TABLE_NOENC
>> -#define PMD_FLAGS	(__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>>  
>> -static void __init *sme_populate_pgd(pgd_t *pgd_base, void *pgtable_area,
>> -				     unsigned long vaddr, pmdval_t pmd_val)
>> +#define PMD_FLAGS_LARGE		(__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>> +
>> +#define PMD_FLAGS_DEC		PMD_FLAGS_LARGE
>> +#define PMD_FLAGS_DEC_WP	((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
>> +				 (_PAGE_PAT | _PAGE_PWT))
>> +#define PMD_FLAGS_ENC		(PMD_FLAGS_LARGE | _PAGE_ENC)
> 
> Align vertically.

Ok

Thanks,
Tom

> 
> Rest looks ok.
>