Re: [PATCH v2 2/2] x86/mm: Create an SME workarea in the kernel for early encryption

[Lendacky, Thomas]

On 6/17/19 6:02 AM, Borislav Petkov wrote:
> On Fri, Jun 14, 2019 at 09:15:19PM +0000, Lendacky, Thomas wrote:
>> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
>> index ca2252ca6ad7..a7aa65b44c71 100644
>> --- a/arch/x86/kernel/vmlinux.lds.S
>> +++ b/arch/x86/kernel/vmlinux.lds.S
>> @@ -387,6 +387,30 @@ SECTIONS
>>  	. = ALIGN(PAGE_SIZE);		/* keep VO_INIT_SIZE page aligned */
>>  	_end = .;
>>  
>> +#ifdef CONFIG_AMD_MEM_ENCRYPT
>> +	/*
>> +	 * SME workarea section: Lives outside of the kernel proper (_text -
>> +	 * _end) for performing in-place encryption of the kernel during boot.
>> +	 *
>> +	 * Resides after _end because even though the .brk section is after
>> +	 * __end_of_kernel_reserve, the .brk section is later reserved as a
>> +	 * part of the kernel. It is used in very early boot code and not
>> +	 * needed after that, so it is located after __end_of_kernel_reserve
>> +	 * so that it will be discarded and become part of the available
>> +	 * memory.
>> +	 *
>> +	 * Resides on a 2MB boundary to simplify the pagetable setup used for
>> +	 * the encryption.
>> +	 */
>> +	. = ALIGN(HPAGE_SIZE);
>> +	.sme : AT(ADDR(.sme) - LOAD_OFFSET) {
> 
> Should we call that section something more generic as
> 
> 	.early_scratch
> 
> or so?
> 
> Someone else might need something like that too, in the future...

Whoever uses it in the future could rename it if desired.  But I can do
that now. Is there a preferred name?  I can leave it as .early_scratch
or .early_workarea.

> 
> Also, the DISCARDS sections do get freed at runtime so why not make it
> part of the DISCARD section...?

I think it's easier to show the alignment requirements that SME has for
this section by having it be its own section.

> 
>> +		__sme_begin = .;
>> +		*(.sme)
>> +		. = ALIGN(HPAGE_SIZE);
>> +		__sme_end = .;
>> +	}
>> +#endif
>> +
>>  	STABS_DEBUG
>>  	DWARF_DEBUG
>>  
>> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
>> index 4aa9b1480866..c55c2ec8fb12 100644
>> --- a/arch/x86/mm/mem_encrypt_identity.c
>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>> @@ -73,6 +73,19 @@ struct sme_populate_pgd_data {
>>  	unsigned long vaddr_end;
>>  };
>>  
>> +/*
>> + * This work area lives in the .sme section, which lives outside of
>> + * the kernel proper. It is sized to hold the intermediate copy buffer
>> + * and more than enough pagetable pages.
>> + *
>> + * By using this section, the kernel can be encrypted in place and we
> 
> replace that "we" with an impartial passive formulation.

Ok.

> 
> Other than that, I like the commenting, very helpful!

I'll send out a V3 with the comments addressed (after giving a bit of time
for name suggestions).

Thanks,
Tom

> 
> Thx.
>