On Sun, Sep 30, 2018 at 04:37:41PM +0800, lijiang wrote:
> In kdump kernel, the old memory needs to be dumped into vmcore file.
> If SME is enabled in the first kernel, the old memory has to be
> remapped with the memory encryption mask, which will be automatically
> decrypted when read from DRAM.
>
> For SME kdump, there are two cases that doesn't support:
Get rid of those two cases in the commit message.
>
> ----------------------------------------------
> | 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, it can't be decrypted.
> The root cause is that the encryption key is not visible to any software
> runnint on the CPU cores(AMD cpu with SME), and is randomly generated on
> eache system reset. That is to say, kdump kernel won't have a chance to
> get the encryption key. So the encrypted memory can not be decrypted
> unless SME is active.
>
> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
> On the one hand, the old memory is decrypted, the old memory can be dumped
> as usual, so SME doesn't need to be enabled in kdump kernel; On the other
> hand, it will increase the complexity of the code, because that 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.
>
> Signed-off-by: Lianbo Jiang <liji...@redhat.com>
> Reviewed-by: Tom Lendacky <thomas.lenda...@amd.com>
You cannot keep Reviewed-by: tags on patches which you change in a
non-trivial manner.
> ---
> Changes since v7:
> 1. Delete a file arch/x86/kernel/crash_dump_encrypt.c, and move the
> copy_oldmem_page_encrypted() to arch/x86/kernel/crash_dump_64.c, also
> rewrite some functions.(Suggested by Borislav)
> 2. Modify all code style issue.(Suggested by Borislav)
> 3. Remove a reduntant header file.(Suggested by Borislav)
> 4. Improve patch log.(Suggested by Borislav)
> 5. Modify compile error "fs/proc/vmcore.c:115: undefined reference
> to `copy_oldmem_page_encrypted'"
> 6. Modify compile error "arch/x86//kernel/crash_dump_64.c:93:9:
> error: redefinition of 'copy_oldmem_page_encrypted'"
>
> arch/x86/kernel/crash_dump_64.c | 65 ++++++++++++++++++++++++++++-----
> fs/proc/vmcore.c | 24 +++++++++---
> include/linux/crash_dump.h | 13 +++++++
> 3 files changed, 87 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/crash_dump_64.c b/arch/x86/kernel/crash_dump_64.c
> index 4f2e0778feac..6adbde592c44 100644
> --- a/arch/x86/kernel/crash_dump_64.c
> +++ b/arch/x86/kernel/crash_dump_64.c
> @@ -12,7 +12,7 @@
> #include <linux/io.h>
>
> /**
> - * copy_oldmem_page - copy one page from "oldmem"
> + * __copy_oldmem_page - copy one page from "old memory encrypted or
> decrypted"
Dammit, what's it with those "old memory encrypted or decrypted" in quotation
marks?! What is wrong with simply saying:
Copy one page of the old kernel's memory. If @encrypted is set, the old
memory will be remapped with the encryption mask.
How hard is that?!
> * @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)
> @@ -20,31 +20,78 @@
> * @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().
> + * @encrypted: if true, the old memory is encrypted.
> + * if false, the old memory is decrypted.
> *
> - * Copy a page from "oldmem". For this page, there is no pte mapped
> - * in the current kernel. We stitch up a pte, similar to kmap_atomic.
> + * Copy a page from "old memory encrypted or decrypted". For this page, there
> + * is no pte mapped in the current kernel. We stitch up a pte, similar to
> + * kmap_atomic.
> */
This function is static now - why does it need to keep the comments
above it? And you've duplicated almost the same comment *three* times
now. Why?
Have the whole comment *once* and only one line sentences over the other
functions explaining the difference only.
> -ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
> - size_t csize, unsigned long offset, int userbuf)
> +static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
> + unsigned long offset, int userbuf,
> + bool encrypted)
> {
> void *vaddr;
--
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
