Date: Sun, 7 Jan 2018 17:28:52 +0100
From: Borislav Petkov <bp@alien8.de>
To: Tom Lendacky <thomas.lendacky@amd.com>
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>
Subject: Re: [PATCH v2 3/5] x86/mm: Centralize PMD flags in
 sme_encrypt_kernel()
Message-ID: <20180107162852.ahfcorhvi2i7xjnr@pd.tnic>
References: <20171221220242.30632.5031.stgit@tlendack-t1.amdoffice.net>
 <20171221220312.30632.22231.stgit@tlendack-t1.amdoffice.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20171221220312.30632.22231.stgit@tlendack-t1.amdoffice.net>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org

On Thu, Dec 21, 2017 at 04:03:12PM -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@amd.com>
> ---
>  arch/x86/mm/mem_encrypt.c |  131 ++++++++++++++++++++++++++-------------------
>  1 file changed, 77 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 5a20696..9b180f8 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -468,31 +468,40 @@ struct sme_populate_pgd_data {
>  	void	*pgtable_area;
>  	pgd_t	*pgd;
>  
> -	pmdval_t pmd_val;
> +	pmdval_t pmd_flags;
> +	unsigned long paddr;
> +
>  	unsigned long vaddr;
> +	unsigned long vaddr_end;
>  };
>  
> -static void __init sme_clear_pgd(pgd_t *pgd_base, unsigned long start,
> -				 unsigned long end)
> +static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
>  {
>  	unsigned long pgd_start, pgd_end, pgd_size;
>  	pgd_t *pgd_p;
>  
> -	pgd_start = start & PGDIR_MASK;
> -	pgd_end = end & PGDIR_MASK;
> +	pgd_start = ppd->vaddr & PGDIR_MASK;
> +	pgd_end = ppd->vaddr_end & PGDIR_MASK;
>  
>  	pgd_size = (((pgd_end - pgd_start) / PGDIR_SIZE) + 1);
>  	pgd_size *= sizeof(pgd_t);

This is a strange way of writing this. I'd expect:

	pgd_size = (((pgd_end - pgd_start) / PGDIR_SIZE) + 1) * sizeof(pgd_t);

>  
> -	pgd_p = pgd_base + pgd_index(start);
> +	pgd_p = ppd->pgd + pgd_index(ppd->vaddr);
>  
>  	memset(pgd_p, 0, pgd_size);
>  }
>  
> -#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)
> +#define PGD_FLAGS		_KERNPG_TABLE_NOENC
> +#define P4D_FLAGS		_KERNPG_TABLE_NOENC
> +#define PUD_FLAGS		_KERNPG_TABLE_NOENC
> +
> +#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)
>  
>  static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
>  {
> @@ -561,7 +570,36 @@ static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
>  
>  	pmd_p += pmd_index(ppd->vaddr);
>  	if (!native_pmd_val(*pmd_p) || !(native_pmd_val(*pmd_p) & _PAGE_PSE))
> -		native_set_pmd(pmd_p, native_make_pmd(ppd->pmd_val));
> +		native_set_pmd(pmd_p,
> +			       native_make_pmd(ppd->paddr | ppd->pmd_flags));

Never do those ugly line breaks. Just let it stick out.

Otherwise, sme_encrypt_kernel() is starting to look quite readable :)

-- 
Regards/Gruss,
    Boris.

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