Subject: Re: [PATCH v1 2/3] x86/mm: Prepare sme_encrypt_kernel() for PAGE
 aligned encryption
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, Brijesh Singh <brijesh.singh@amd.com>,
        linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Thomas Gleixner <tglx@linutronix.de>
References: <20171207233342.29646.12858.stgit@tlendack-t1.amdoffice.net>
 <20171207233402.29646.77306.stgit@tlendack-t1.amdoffice.net>
 <20171221125800.wb5747hq4vbp5yhf@pd.tnic>
From: Tom Lendacky <thomas.lendacky@amd.com>
Message-ID: <4ff5706f-367d-2204-d753-63e78931fd66@amd.com>
Date: Thu, 21 Dec 2017 10:35:37 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20171221125800.wb5747hq4vbp5yhf@pd.tnic>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Dec 2017 16:35:40.8282 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a58bce13-fe58-47e5-4667-08d54890e041
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1144
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6793
Lines: 211

On 12/21/2017 6:58 AM, Borislav Petkov wrote:
> On Thu, Dec 07, 2017 at 05:34:02PM -0600, Tom Lendacky wrote:
>> In preparation for encrypting more than just the kernel, the encryption
>> support in sme_encrypt_kernel() needs to support 4KB page aligned
>> encryption instead of just 2MB large page aligned encryption.
> 
> ...
> 
>>  static void __init __sme_map_range(pgd_t *pgd, unsigned long vaddr,
>>  				   unsigned long vaddr_end,
>> -				   unsigned long paddr, pmdval_t pmd_flags)
>> +				   unsigned long paddr,
>> +				   pmdval_t pmd_flags, pteval_t pte_flags)
>>  {
>> -	while (vaddr < vaddr_end) {
>> -		sme_populate_pgd(pgd, vaddr, paddr, pmd_flags);
>> +	if (vaddr & ~PMD_PAGE_MASK) {
>> +		/* Start is not 2MB aligned, create PTE entries */
>> +		unsigned long pmd_start = ALIGN(vaddr, PMD_PAGE_SIZE);
>> +
>> +		while (vaddr < pmd_start) {
>> +			sme_populate_pgd(pgd, vaddr, paddr, pte_flags);
>> +
>> +			vaddr += PAGE_SIZE;
>> +			paddr += PAGE_SIZE;
>> +		}
>> +	}
> 
> This chunk...
> 
>> +
>> +	while (vaddr < (vaddr_end & PMD_PAGE_MASK)) {
>> +		sme_populate_pgd_large(pgd, vaddr, paddr, pmd_flags);
>>  
>>  		vaddr += PMD_PAGE_SIZE;
>>  		paddr += PMD_PAGE_SIZE;
>>  	}
>> +
>> +	if (vaddr_end & ~PMD_PAGE_MASK) {
>> +		/* End is not 2MB aligned, create PTE entries */
>> +		while (vaddr < vaddr_end) {
>> +			sme_populate_pgd(pgd, vaddr, paddr, pte_flags);
>> +
>> +			vaddr += PAGE_SIZE;
>> +			paddr += PAGE_SIZE;
>> +		}
>> +	}
> 
> ... and this chunk look like could be extracted in a wrapper as they're
> pretty similar.
> 

Should be doable, I'll take care of it.

>>  static void __init sme_map_range_encrypted(pgd_t *pgd,
>> @@ -582,7 +658,8 @@ static void __init sme_map_range_encrypted(pgd_t *pgd,
>>  					   unsigned long vaddr_end,
>>  					   unsigned long paddr)
>>  {
>> -	__sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_ENC);
>> +	__sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> +			PMD_FLAGS_ENC, PTE_FLAGS_ENC);
>>  }
>>  
>>  static void __init sme_map_range_decrypted(pgd_t *pgd,
>> @@ -590,7 +667,8 @@ static void __init sme_map_range_decrypted(pgd_t *pgd,
>>  					   unsigned long vaddr_end,
>>  					   unsigned long paddr)
>>  {
>> -	__sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_DEC);
>> +	__sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> +			PMD_FLAGS_DEC, PTE_FLAGS_DEC);
>>  }
>>  
>>  static void __init sme_map_range_decrypted_wp(pgd_t *pgd,
>> @@ -598,19 +676,20 @@ static void __init sme_map_range_decrypted_wp(pgd_t *pgd,
>>  					      unsigned long vaddr_end,
>>  					      unsigned long paddr)
>>  {
>> -	__sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_DEC_WP);
>> +	__sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> +			PMD_FLAGS_DEC_WP, PTE_FLAGS_DEC_WP);
>>  }
>>  
>>  static unsigned long __init sme_pgtable_calc(unsigned long len)
>>  {
>> -	unsigned long p4d_size, pud_size, pmd_size;
>> +	unsigned long p4d_size, pud_size, pmd_size, pte_size;
>>  	unsigned long total;
>>  
>>  	/*
>>  	 * Perform a relatively simplistic calculation of the pagetable
>> -	 * entries that are needed. That mappings will be covered by 2MB
>> -	 * PMD entries so we can conservatively calculate the required
>> -	 * number of P4D, PUD and PMD structures needed to perform the
>> +	 * entries that are needed. That mappings will be covered mostly
> 
> 				    Those
> 

Got it.

>> +	 * by 2MB PMD entries so we can conservatively calculate the required
>> +	 * number of P4D, PUD, PMD and PTE structures needed to perform the
>>  	 * mappings. Incrementing the count for each covers the case where
>>  	 * the addresses cross entries.
>>  	 */
>> @@ -626,8 +705,9 @@ static unsigned long __init sme_pgtable_calc(unsigned long len)
>>  	}
>>  	pmd_size = (ALIGN(len, PUD_SIZE) / PUD_SIZE) + 1;
>>  	pmd_size *= sizeof(pmd_t) * PTRS_PER_PMD;
>> +	pte_size = 2 * sizeof(pte_t) * PTRS_PER_PTE;
> 
> This needs the explanation from the commit message about the 2 extra
> pages, as a comment above it.

Yup, I'll include the info about the (possible) extra PTE entries.

> 
>> -	total = p4d_size + pud_size + pmd_size;
>> +	total = p4d_size + pud_size + pmd_size + pte_size;
>>  
>>  	/*
>>  	 * Now calculate the added pagetable structures needed to populate
>> @@ -710,10 +790,13 @@ void __init sme_encrypt_kernel(void)
>>  
>>  	/*
>>  	 * The total workarea includes the executable encryption area and
>> -	 * the pagetable area.
>> +	 * the pagetable area. The start of the workarea is already 2MB
>> +	 * aligned, align the end of the workarea on a 2MB boundary so that
>> +	 * we don't try to create/allocate PTE entries from the workarea
>> +	 * before it is mapped.
>>  	 */
>>  	workarea_len = execute_len + pgtable_area_len;
>> -	workarea_end = workarea_start + workarea_len;
>> +	workarea_end = ALIGN(workarea_start + workarea_len, PMD_PAGE_SIZE);
>>  
>>  	/*
>>  	 * Set the address to the start of where newly created pagetable
>> diff --git a/arch/x86/mm/mem_encrypt_boot.S b/arch/x86/mm/mem_encrypt_boot.S
>> index 730e6d5..20cca86 100644
>> --- a/arch/x86/mm/mem_encrypt_boot.S
>> +++ b/arch/x86/mm/mem_encrypt_boot.S
>> @@ -120,23 +120,33 @@ ENTRY(__enc_copy)
>>  
>>  	wbinvd				/* Invalidate any cache entries */
>>  
>> +	push	%r12
>> +
>>  	/* Copy/encrypt 2MB at a time */
>> +	movq	$PMD_PAGE_SIZE, %r12
>>  1:
>> +	cmpq	%r12, %r9
>> +	jnb	2f
>> +	movq	%r9, %r12
>> +
>> +2:
>>  	movq	%r11, %rsi		/* Source - decrypted kernel */
>>  	movq	%r8, %rdi		/* Dest   - intermediate copy buffer */
>> -	movq	$PMD_PAGE_SIZE, %rcx	/* 2MB length */
>> +	movq	%r12, %rcx
>>  	rep	movsb
>>  
>>  	movq	%r8, %rsi		/* Source - intermediate copy buffer */
>>  	movq	%r10, %rdi		/* Dest   - encrypted kernel */
>> -	movq	$PMD_PAGE_SIZE, %rcx	/* 2MB length */
>> +	movq	%r12, %rcx
>>  	rep	movsb
>>  
>> -	addq	$PMD_PAGE_SIZE, %r11
>> -	addq	$PMD_PAGE_SIZE, %r10
>> -	subq	$PMD_PAGE_SIZE, %r9	/* Kernel length decrement */
>> +	addq	%r12, %r11
>> +	addq	%r12, %r10
>> +	subq	%r12, %r9		/* Kernel length decrement */
>>  	jnz	1b			/* Kernel length not zero? */
>>  
>> +	pop	%r12
>> +
>>  	/* Restore PAT register */
>>  	push	%rdx			/* Save original PAT value */
>>  	movl	$MSR_IA32_CR_PAT, %ecx
> 
> Right, for this here can you pls do a cleanup pre-patch which does
> 
> 	push
> 	push
> 	push
> 
> 	/* meat of the function */
> 
> 	pop
> 	pop
> 	pop
> 
> as now those pushes and pops are interspersed with the rest of the insns
> and that makes following through it harder. F17h has a stack engine so
> those streams of pushes and pops won't hurt perf and besides, this is
> not even perf-sensitive.
> 

Ok, I'll clean that up to move any pushes/pops to the beginning/end of
the function, as well as moving some register saving earlier which may
eliminate some push/pop instructions.

Thanks,
Tom

> Thx.
>