On Thu, Sep 27, 2018 at 03:19:54PM +0800, Lianbo Jiang wrote:
> In kdump kernel, we need to dump the old memory into vmcore file,if SME
> is enabled in the first kernel, we have to remap the old memory with the
> memory encryption mask, which will be automatically decrypted when we
> read from DRAM.
> 
> For SME kdump, there are two cases that doesn't support:

... and which are simply silly to support.

> 
>  ----------------------------------------------
> | first-kernel | second-kernel | kdump support |
> |      (mem_encrypt=on|off)    |   (yes|no)    |
> |--------------+---------------+---------------|
> |     on       |     on        |     yes       |
> |     off      |     off       |     yes       |
> |     on       |     off       |     no        |
> |     off      |     on        |     no        |
> |______________|_______________|_______________|
> 
> 1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
> In this case, because the old memory is encrypted, we can't decrypt the
> old memory.
> 
> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
> On the one hand, the old memory is unencrypted, the old memory can be dumped

s/unencrypted/decrypted/g

But I mentioned that already.

> as usual, we don't need to enable SME in kdump kernel; On the other hand, it
> will increase the complexity of the code, we will have to consider how to
> pass the SME flag from the first kernel to the kdump kernel, it is really
> too expensive to do this.
> 
> This patches are only for SME kdump, the patches don't support SEV kdump.

Please rewrite that commit message in passive voice. I.e., get rid of
that "we".

> 
> Signed-off-by: Lianbo Jiang <liji...@redhat.com>
> Reviewed-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/kernel/Makefile             |  1 +
>  arch/x86/kernel/crash_dump_encrypt.c | 53 ++++++++++++++++++++++++++++
>  fs/proc/vmcore.c                     | 21 +++++++----
>  include/linux/crash_dump.h           | 12 +++++++
>  4 files changed, 81 insertions(+), 6 deletions(-)
>  create mode 100644 arch/x86/kernel/crash_dump_encrypt.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 8824d01c0c35..dfbeae0e35ce 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_KEXEC_CORE)    += machine_kexec_$(BITS).o
>  obj-$(CONFIG_KEXEC_CORE)     += relocate_kernel_$(BITS).o crash.o
>  obj-$(CONFIG_KEXEC_FILE)     += kexec-bzimage64.o
>  obj-$(CONFIG_CRASH_DUMP)     += crash_dump_$(BITS).o
> +obj-$(CONFIG_AMD_MEM_ENCRYPT)        += crash_dump_encrypt.o

No no.

This will build even in the CONFIG_CRASH_DUMP=n case.

Why does this need to be even a separate compilation unit? It is a file
containing a single function?!?!

I would love to know what the logic behind this was...

>  obj-y                                += kprobes/
>  obj-$(CONFIG_MODULES)                += module.o
>  obj-$(CONFIG_DOUBLEFAULT)    += doublefault.o
> diff --git a/arch/x86/kernel/crash_dump_encrypt.c 
> b/arch/x86/kernel/crash_dump_encrypt.c
> new file mode 100644
> index 000000000000..e1b1a577f197
> --- /dev/null
> +++ b/arch/x86/kernel/crash_dump_encrypt.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *   Memory preserving reboot related code.
> + *
> + *   Created by: Lianbo Jiang (liji...@redhat.com)
> + *   Copyright (C) RedHat Corporation, 2018. All rights reserved
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/crash_dump.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +
> +/**
> + * copy_oldmem_page_encrypted - copy one page from "oldmem encrypted"
> + * @pfn: page frame number to be copied
> + * @buf: target memory address for the copy; this can be in kernel address
> + *   space or user address space (see @userbuf)
> + * @csize: number of bytes to copy
> + * @offset: offset in bytes into the page (based on pfn) to begin the copy
> + * @userbuf: if set, @buf is in user address space, use copy_to_user(),
> + *   otherwise @buf is in kernel address space, use memcpy().
> + *
> + * Copy a page from "oldmem encrypted". For this page, there is no pte

What is "oldmem encrypted"? Why can't you explain that in plain english?
Note that those comments are not write-only but are meant for other
people to read in the future.

> + * mapped in the current kernel. We stitch up a pte, similar to
> + * kmap_atomic.
> + */
> +
> +ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char *buf,
> +             size_t csize, unsigned long offset, int userbuf)

Align arguments on the opening brace.

> +{
> +     void  *vaddr;
> +
> +     if (!csize)
> +             return 0;
> +
> +     vaddr = (__force void *)ioremap_encrypted(pfn << PAGE_SHIFT,
> +                                               PAGE_SIZE);

Let it stick out.

> +     if (!vaddr)
> +             return -ENOMEM;
> +
> +     if (userbuf) {
> +             if (copy_to_user((void __user *)buf, vaddr + offset, csize)) {
> +                     iounmap((void __iomem *)vaddr);
> +                     return -EFAULT;
> +             }
> +     } else
> +             memcpy(buf, vaddr + offset, csize);
> +
> +     set_iounmap_nonlazy();
> +     iounmap((void __iomem *)vaddr);
> +     return csize;
> +}
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index cbde728f8ac6..3065c8bada6a 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -25,6 +25,9 @@
>  #include <linux/pagemap.h>
>  #include <linux/uaccess.h>
>  #include <asm/io.h>
> +#include <linux/io.h>
> +#include <linux/mem_encrypt.h>
> +#include <asm/pgtable.h>

Do you not see how the order of the include files is? First linux/ then
asm/, then local headers.

>  #include "internal.h"

And you don't need that if you drop that silly crash_dump_encrypt.c thing.

>  
>  /* List representing chunks of contiguous memory areas and their offsets in
> @@ -98,7 +101,8 @@ static int pfn_is_ram(unsigned long pfn)
>  
>  /* Reads a page from the oldmem device from given offset. */
>  static ssize_t read_from_oldmem(char *buf, size_t count,
> -                             u64 *ppos, int userbuf)
> +                             u64 *ppos, int userbuf,
> +                             bool encrypted)
>  {
>       unsigned long pfn, offset;
>       size_t nr_bytes;
> @@ -120,8 +124,11 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
>               if (pfn_is_ram(pfn) == 0)
>                       memset(buf, 0, nr_bytes);
>               else {
> -                     tmp = copy_oldmem_page(pfn, buf, nr_bytes,
> -                                             offset, userbuf);
> +                     tmp = encrypted ? copy_oldmem_page_encrypted(pfn,
> +                                         buf, nr_bytes, offset, userbuf)
> +                                     : copy_oldmem_page(pfn, buf, nr_bytes,
> +                                                        offset, userbuf);

Make that a simple if-else so that it can actually be readable.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Reply via email to