Re: [PATCH 1/4 V3] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

Tom Lendacky
2018-06-20T16:01:05Z

On 6/16/2018 3:27 AM, Lianbo Jiang wrote:
> It is convenient to remap the old memory encrypted to the second
> kernel by calling ioremap_encrypted().
>
> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
> Some changes:
> 1. remove the sme_active() check in __ioremap_caller().
> 2. put some logic into the early_memremap_pgprot_adjust() for
> early memremap.
>
> arch/x86/include/asm/io.h | 3 +++
> arch/x86/mm/ioremap.c | 28 ++++++++++++++++++++--------
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index f6e5b93..989d60b 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -192,6 +192,9 @@ extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
> #define ioremap_cache ioremap_cache
> extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
> #define ioremap_prot ioremap_prot
> +extern void __iomem *ioremap_encrypted(resource_size_t phys_addr,
> + unsigned long size);
> +#define ioremap_encrypted ioremap_encrypted
>
> /**
> * ioremap - map bus memory into CPU space
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index c63a545..e365fc4 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -24,6 +24,7 @@
> #include <asm/pgalloc.h>
> #include <asm/pat.h>
> #include <asm/setup.h>
> +#include <linux/crash_dump.h>
>
> #include "physaddr.h"
>
> @@ -131,7 +132,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
> * caller shouldn't need to know that small detail.
> */
> static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> - unsigned long size, enum page_cache_mode pcm, void *caller)
> + unsigned long size, enum page_cache_mode pcm,
> + void *caller, bool encrypted)
> {
> unsigned long offset, vaddr;
> resource_size_t last_addr;
> @@ -199,7 +201,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> * resulting mapping.
> */
> prot = PAGE_KERNEL_IO;
> - if (sev_active() && mem_flags.desc_other)
> + if ((sev_active() && mem_flags.desc_other) || encrypted)
> prot = pgprot_encrypted(prot);
>
> switch (pcm) {
> @@ -291,7 +293,7 @@ void __iomem *ioremap_nocache(resource_size_t phys_addr, unsigned long size)
> enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC_MINUS;
>
> return __ioremap_caller(phys_addr, size, pcm,
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL(ioremap_nocache);
>
> @@ -324,7 +326,7 @@ void __iomem *ioremap_uc(resource_size_t phys_addr, unsigned long size)
> enum page_cache_mode pcm = _PAGE_CACHE_MODE_UC;
>
> return __ioremap_caller(phys_addr, size, pcm,
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL_GPL(ioremap_uc);
>
> @@ -341,7 +343,7 @@ EXPORT_SYMBOL_GPL(ioremap_uc);
> void __iomem *ioremap_wc(resource_size_t phys_addr, unsigned long size)
> {
> return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WC,
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL(ioremap_wc);
>
> @@ -358,14 +360,21 @@ EXPORT_SYMBOL(ioremap_wc);
> void __iomem *ioremap_wt(resource_size_t phys_addr, unsigned long size)
> {
> return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WT,
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL(ioremap_wt);
>
> +void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size)
> +{
> + return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
> + __builtin_return_address(0), true);
> +}
> +EXPORT_SYMBOL(ioremap_encrypted);
> +
> void __iomem *ioremap_cache(resource_size_t phys_addr, unsigned long size)
> {
> return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WB,
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL(ioremap_cache);
>
> @@ -374,7 +383,7 @@ void __iomem *ioremap_prot(resource_size_t phys_addr, unsigned long size,
> {
> return __ioremap_caller(phys_addr, size,
> pgprot2cachemode(__pgprot(prot_val)),
> - __builtin_return_address(0));
> + __builtin_return_address(0), false);
> }
> EXPORT_SYMBOL(ioremap_prot);
>
> @@ -688,6 +697,9 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
> if (encrypted_prot && memremap_should_map_decrypted(phys_addr, size))
> encrypted_prot = false;
>
> + if (sme_active() && is_kdump_kernel())
> + encrypted_prot = false;
> +

This change doesn't seem to go with the patch description. Maybe it
should be a separate patch? Or provide more detail in the description
for this change.

What is this change addressing? What early_memremap() calls were failing
that required this? If this truly is required, you can probably move it
up into the "if (sme_active())" block a few lines higher in the function.

Thanks,
Tom

> return encrypted_prot ? pgprot_encrypted(prot)
> : pgprot_decrypted(prot);
> }
>