Re: [PATCH v1 2/3] x86/mm: Prepare sme_encrypt_kernel() for PAGE aligned encryption

[Borislav Petkov]

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.

>  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

> +	 * 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.

> -	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.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.