[PATCH 0/3] Add kdump support for the SEV enabled guest

[PATCH 0/3] Add kdump support for the SEV enabled guest

[Lianbo Jiang]

For the AMD SEV machines, add kdump support when the SEV is enabled.

Test tools:
makedumpfile[v1.6.5]:
git://git.code.sf.net/p/makedumpfile/code
commit <d222b01e516b> ("Add support for AMD Secure Memory Encryption")
Note: This patch was merged into the devel branch.

crash-7.2.5: https://github.com/crash-utility/crash.git

kexec-tools-2.0.19:
git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git
commit <942d813cda35> ("Fix for the kmem '-i' option on Linux 5.0")
http://lists.infradead.org/pipermail/kexec/2019-March/022576.html
Note: The second kernel cann't boot without this patch. 

kernel:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
commit <f261c4e529da> ("Merge branch 'akpm' (patches from Andrew)")

Test steps:
[1] load the vmlinux and initrd for kdump
# kexec -p /boot/vmlinuz-5.0.0+ --initrd=/boot/initramfs-5.0.0+kdump.img --command-line="BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.0.0+ ro resume=UUID=126c5e95-fc8b-48d6-a23b-28409198a52e console=ttyS0,115200 earlyprintk=serial irqpoll nr_cpus=1 reset_devices cgroup_disable=memory mce=off numa=off udev.children-max=2 panic=10 rootflags=nofail acpi_no_memhotplug transparent_hugepage=never disable_cpu_apicid=0"

[2] trigger panic
# echo 1 > /proc/sys/kernel/sysrq
# echo c > /proc/sysrq-trigger

[3] check and parse the vmcore
# crash vmlinux /var/crash/127.0.0.1-2019-03-15-05\:03\:42/vmcore

Lianbo Jiang (3):
  kexec: Do not map the kexec area as decrypted when SEV is active
  kexec: Set the C-bit in the identity map page table when SEV is active
  kdump,proc/vmcore: Enable kdumping encrypted memory when SEV was
    active

 arch/x86/kernel/machine_kexec_64.c | 20 +++++++++++++++++---
 fs/proc/vmcore.c                   |  6 +++---
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Lianbo Jiang]

Currently, the arch_kexec_post_{alloc,free}_pages unconditionally
maps the kexec area as decrypted. This works fine when SME is active.
Because in SME, the first kernel is loaded in decrypted area by the
BIOS, so the second kernel must be also loaded into the decrypted
memory.

When SEV is active, the first kernel is loaded into the encrypted
area, so the second kernel must be also loaded into the encrypted
memory. Lets make sure that arch_kexec_post_{alloc,free}_pages does
not clear the memory encryption mask from the kexec area when SEV
is active.

Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 arch/x86/kernel/machine_kexec_64.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index ceba408ea982..bcebf4993da4 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -566,7 +566,10 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
 	 * not encrypted because when we boot to the new kernel the
 	 * pages won't be accessed encrypted (initially).
 	 */
-	return set_memory_decrypted((unsigned long)vaddr, pages);
+	if (sme_active())
+		return set_memory_decrypted((unsigned long)vaddr, pages);
+
+	return 0;
 }
 
 void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
@@ -575,5 +578,6 @@ void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
 	 * If SME is active we need to reset the pages back to being
 	 * an encrypted mapping before freeing them.
 	 */
-	set_memory_encrypted((unsigned long)vaddr, pages);
+	if (sme_active())
+		set_memory_encrypted((unsigned long)vaddr, pages);
 }
-- 
2.17.1

[PATCH 2/3] kexec: Set the C-bit in the identity map page table when SEV is active

[Lianbo Jiang]

When SEV is active, the second kernel image is loaded into the
encrypted memory. Lets make sure that when kexec builds the
identity mapping page table it adds the memory encryption mask(C-bit).

Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 arch/x86/kernel/machine_kexec_64.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index bcebf4993da4..8c58d1864500 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -56,6 +56,7 @@ static int init_transition_pgtable(struct kimage *image, pgd_t *pgd)
 	pte_t *pte;
 	unsigned long vaddr, paddr;
 	int result = -ENOMEM;
+	pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
 
 	vaddr = (unsigned long)relocate_kernel;
 	paddr = __pa(page_address(image->control_code_page)+PAGE_SIZE);
@@ -92,7 +93,11 @@ static int init_transition_pgtable(struct kimage *image, pgd_t *pgd)
 		set_pmd(pmd, __pmd(__pa(pte) | _KERNPG_TABLE));
 	}
 	pte = pte_offset_kernel(pmd, vaddr);
-	set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, PAGE_KERNEL_EXEC_NOENC));
+
+	if (sev_active())
+		prot = PAGE_KERNEL_EXEC;
+
+	set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
 	return 0;
 err:
 	return result;
@@ -129,6 +134,11 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
 	level4p = (pgd_t *)__va(start_pgtable);
 	clear_page(level4p);
 
+	if (sev_active()) {
+		info.page_flag |= _PAGE_ENC;
+		info.kernpg_flag = _KERNPG_TABLE;
+	}
+
 	if (direct_gbpages)
 		info.direct_gbpages = true;
 
-- 
2.17.1

[PATCH 3/3] kdump,proc/vmcore: Enable kdumping encrypted memory when SEV was active

[Lianbo Jiang]

In the kdump kernel, the memory of first kernel needs to be dumped
into the vmcore file.

It is similar to the SME, if SEV is enabled in the first kernel, the
old memory has to be remapped with memory encryption mask in order to
access it properly. Following commit 992b649a3f01 ("kdump, proc/vmcore:
Enable kdumping encrypted memory with SME enabled") took care of the
SME case but it uses sme_active() which checks for SME only. Lets use
the mem_encrypt_active() which returns true when either of them are
active.

Unlike the SME, the first kernel is loaded into the encrypted memory
when SEV was enabled, hence the kernel elf header must be remapped as
encrypted in order to access it properly.

Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 fs/proc/vmcore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 3fe90443c1bb..cda6c1922e4f 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -165,7 +165,7 @@ void __weak elfcorehdr_free(unsigned long long addr)
  */
 ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0, false);
+	return read_from_oldmem(buf, count, ppos, 0, sev_active());
 }
 
 /*
@@ -173,7 +173,7 @@ ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
  */
 ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
 {
-	return read_from_oldmem(buf, count, ppos, 0, sme_active());
+	return read_from_oldmem(buf, count, ppos, 0, mem_encrypt_active());
 }
 
 /*
@@ -373,7 +373,7 @@ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
 					    buflen);
 			start = m->paddr + *fpos - m->offset;
 			tmp = read_from_oldmem(buffer, tsz, &start,
-					       userbuf, sme_active());
+					       userbuf, mem_encrypt_active());
 			if (tmp < 0)
 				return tmp;
 			buflen -= tsz;
-- 
2.17.1

Re: [PATCH 0/3] Add kdump support for the SEV enabled guest

[lijiang]

在 2019年03月15日 18:32, Lianbo Jiang 写道:
> For the AMD SEV machines, add kdump support when the SEV is enabled.
> 
> Test tools:
> makedumpfile[v1.6.5]:
> git://git.code.sf.net/p/makedumpfile/code
> commit <d222b01e516b> ("Add support for AMD Secure Memory Encryption")
> Note: This patch was merged into the devel branch.
> 
> crash-7.2.5: https://github.com/crash-utility/crash.git

commit <942d813cda35> ("Fix for the "kmem -i" option on Linux 5.0 and later kernels")

> 
> kexec-tools-2.0.19:
> git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git
> commit <942d813cda35> ("Fix for the kmem '-i' option on Linux 5.0")
> http://lists.infradead.org/pipermail/kexec/2019-March/022576.html
> Note: The second kernel cann't boot without this patch. 
> 
> kernel:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> commit <f261c4e529da> ("Merge branch 'akpm' (patches from Andrew)")
> 
> Test steps:
> [1] load the vmlinux and initrd for kdump
> # kexec -p /boot/vmlinuz-5.0.0+ --initrd=/boot/initramfs-5.0.0+kdump.img --command-line="BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.0.0+ ro resume=UUID=126c5e95-fc8b-48d6-a23b-28409198a52e console=ttyS0,115200 earlyprintk=serial irqpoll nr_cpus=1 reset_devices cgroup_disable=memory mce=off numa=off udev.children-max=2 panic=10 rootflags=nofail acpi_no_memhotplug transparent_hugepage=never disable_cpu_apicid=0"
> 
> [2] trigger panic
> # echo 1 > /proc/sys/kernel/sysrq
> # echo c > /proc/sysrq-trigger
> 
> [3] check and parse the vmcore
> # crash vmlinux /var/crash/127.0.0.1-2019-03-15-05\:03\:42/vmcore
> 
> Lianbo Jiang (3):
>   kexec: Do not map the kexec area as decrypted when SEV is active
>   kexec: Set the C-bit in the identity map page table when SEV is active
>   kdump,proc/vmcore: Enable kdumping encrypted memory when SEV was
>     active
> 
>  arch/x86/kernel/machine_kexec_64.c | 20 +++++++++++++++++---
>  fs/proc/vmcore.c                   |  6 +++---
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Borislav Petkov]

> Subject: Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

The tip tree preferred format for patch subject prefixes is
'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
'genirq/core:'. Please do not use file names or complete file paths as
prefix. 'git log path/to/file' should give you a reasonable hint in most
cases.

On Fri, Mar 15, 2019 at 06:32:01PM +0800, Lianbo Jiang wrote:
> Currently, the arch_kexec_post_{alloc,free}_pages unconditionally

Please end function names with parentheses.

> maps the kexec area as decrypted. This works fine when SME is active.
> Because in SME, the first kernel is loaded in decrypted area by the
> BIOS, so the second kernel must be also loaded into the decrypted
> memory.
> 
> When SEV is active, the first kernel is loaded into the encrypted
> area, so the second kernel must be also loaded into the encrypted
> memory. Lets make sure that arch_kexec_post_{alloc,free}_pages does
> not clear the memory encryption mask from the kexec area when SEV
> is active.

Hold on, wait a minute!

Why do we even need this? As usual, you guys never explain what the big
picture is. So you mention SEV, which sounds to me like you want to be
able to kexec the SEV *guest*. Yes?

First of all, why?

Then, if so...

> Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
>  arch/x86/kernel/machine_kexec_64.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index ceba408ea982..bcebf4993da4 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -566,7 +566,10 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
>  	 * not encrypted because when we boot to the new kernel the
>  	 * pages won't be accessed encrypted (initially).
>  	 */
> -	return set_memory_decrypted((unsigned long)vaddr, pages);
> +	if (sme_active())
> +		return set_memory_decrypted((unsigned long)vaddr, pages);

... then this looks yucky. Because, you're adding an sme_active() check here
but then __set_memory_enc_dec() checks

	if (!mem_encrypt_active())

and heads will spin from all the checking of memory encryption aspects.

So this would need a rework so that there are no multiple confusing
checks.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[lijiang]

在 2019年03月24日 23:00, Borislav Petkov 写道:
>> Subject: Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active
> 
> The tip tree preferred format for patch subject prefixes is
> 'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
> 'genirq/core:'. Please do not use file names or complete file paths as
> prefix. 'git log path/to/file' should give you a reasonable hint in most
> cases.

Fine, thanks for your advice.

> 
> On Fri, Mar 15, 2019 at 06:32:01PM +0800, Lianbo Jiang wrote:
>> Currently, the arch_kexec_post_{alloc,free}_pages unconditionally
> 
> Please end function names with parentheses.

Ok, i will improve them next post.

> 
>> maps the kexec area as decrypted. This works fine when SME is active.
>> Because in SME, the first kernel is loaded in decrypted area by the
>> BIOS, so the second kernel must be also loaded into the decrypted
>> memory.
>>
>> When SEV is active, the first kernel is loaded into the encrypted
>> area, so the second kernel must be also loaded into the encrypted
>> memory. Lets make sure that arch_kexec_post_{alloc,free}_pages does
>> not clear the memory encryption mask from the kexec area when SEV
>> is active.
> 
> Hold on, wait a minute!
> 
> Why do we even need this? As usual, you guys never explain what the big
> picture is. So you mention SEV, which sounds to me like you want to be
> able to kexec the SEV *guest*. Yes?

Yes. Just like the physical machines support kdump, the virtual machines also
need kdump. When a virtual machine panic, we also need to dump its memory for
analysis.

> 
> First of all, why?

For the SEV virtual machine, the memory is also encrypted. When SEV is enabled,
the first kernel is loaded into the encrypted area. Unlike the SME, the first
kernel is loaded into the decrypted area.

Because of this difference between SME and SEV, we need to properly map the kexec
memory area in order to correctly access it.

> 
> Then, if so...
> 
>> Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> ---
>>  arch/x86/kernel/machine_kexec_64.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
>> index ceba408ea982..bcebf4993da4 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -566,7 +566,10 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
>>  	 * not encrypted because when we boot to the new kernel the
>>  	 * pages won't be accessed encrypted (initially).
>>  	 */
>> -	return set_memory_decrypted((unsigned long)vaddr, pages);
>> +	if (sme_active())
>> +		return set_memory_decrypted((unsigned long)vaddr, pages);
> 
> ... then this looks yucky. Because, you're adding an sme_active() check here
> but then __set_memory_enc_dec() checks

For the SEV virtual machine, it maps the kexec memroy area as encrypted, so, no need to invoke
this function to change anything.


> 
> 	if (!mem_encrypt_active())
> 
> and heads will spin from all the checking of memory encryption aspects.
> 
> So this would need a rework so that there are no multiple confusing
> checks.

About the three functions, here i copied their comment from the arch/x86/mm/mem_encrypt.c
Please refer to it.

/*
 * SME and SEV are very similar but they are not the same, so there are
 * times that the kernel will need to distinguish between SME and SEV. The
 * sme_active() and sev_active() functions are used for this.  When a
 * distinction isn't needed, the mem_encrypt_active() function can be used.
 *


Thanks.
Lianbo

> 
> Thx.
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Borislav Petkov]

On Mon, Mar 25, 2019 at 09:58:07AM +0800, lijiang wrote:
> For the SEV virtual machine, it maps the kexec memroy area as
> encrypted, so, no need to invoke this function to change anything.

Look at the code:

set_memory_decrypted->__set_memory_enc_dec

It already *does* invoke this function.

> > 	if (!mem_encrypt_active())
> > 
> > and heads will spin from all the checking of memory encryption aspects.
> > 
> > So this would need a rework so that there are no multiple confusing
> > checks.
> 
> About the three functions, here i copied their comment from the arch/x86/mm/mem_encrypt.c
> Please refer to it.

I know that comment - I have asked for it. Now you go and look at the
code again with your patch applied.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Singh, Brijesh]

Hi Boris,


On 3/25/19 1:37 AM, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 09:58:07AM +0800, lijiang wrote:
>> For the SEV virtual machine, it maps the kexec memroy area as
>> encrypted, so, no need to invoke this function to change anything.
> 
> Look at the code:
> 
> set_memory_decrypted->__set_memory_enc_dec
> 
> It already *does* invoke this function.
> 

By default all the memory regions are mapped encrypted. The
set_memory_{encrypt,decrypt}() is a generic function which can be
called explicitly to clear/set the encryption mask from the existing
memory mapping. The mem_encrypt_active() returns true if either SEV or 
SME is active. So the __set_memory_enc_dec() uses the
memory_encrypt_active() check to ensure that the function is no-op when
SME/SEV are not active.

Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
encryption mask from the kexec area. In case of SEV, we should not clear
the encryption mask.



>>> 	if (!mem_encrypt_active())
>>>
>>> and heads will spin from all the checking of memory encryption aspects.
>>>
>>> So this would need a rework so that there are no multiple confusing
>>> checks.
>>
>> About the three functions, here i copied their comment from the arch/x86/mm/mem_encrypt.c
>> Please refer to it.
> 
> I know that comment - I have asked for it. Now you go and look at the
> code again with your patch applied.
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Borislav Petkov]

On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
> By default all the memory regions are mapped encrypted. The
> set_memory_{encrypt,decrypt}() is a generic function which can be
> called explicitly to clear/set the encryption mask from the existing
> memory mapping. The mem_encrypt_active() returns true if either SEV or 
> SME is active. So the __set_memory_enc_dec() uses the
> memory_encrypt_active() check to ensure that the function is no-op when
> SME/SEV are not active.
> 
> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
> encryption mask from the kexec area. In case of SEV, we should not clear
> the encryption mask.

Brijesh, I know all that.

Please read what I said here at the end:

https://lkml.kernel.org/r/20190324150034.GH23289@zn.tnic

With this change, the code looks like this:

+       if (sme_active())
+               return set_memory_decrypted((unsigned long)vaddr, pages);

now in __set_memory_enc_dec via set_memory_decrypted():

        /* Nothing to do if memory encryption is not active */
        if (!mem_encrypt_active())
                return 0;


so you have:

	if (sme_active())

		...

		if (!mem_encrypt_active())


now maybe this is all clear to you and Tom but I betcha others will get
confused. Probably something like "well, what should be active now, SME,
SEV or memory encryption in general"?

I hope you're catching my drift.

So if you want to *not* decrypt memory in the SEV case, then doing something
like this should make it a bit more clear:


	if (sev_active())
		return;

	return set_memory_decrypted((unsigned long)vaddr, pages);

along with a comment *why* we're checking here.

But actually, I'd prefer if you had separate wrappers which are called
for SME and for SEV.

I'll let Tom chime in too.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Singh, Brijesh]

On 3/25/19 12:32 PM, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
>> By default all the memory regions are mapped encrypted. The
>> set_memory_{encrypt,decrypt}() is a generic function which can be
>> called explicitly to clear/set the encryption mask from the existing
>> memory mapping. The mem_encrypt_active() returns true if either SEV or
>> SME is active. So the __set_memory_enc_dec() uses the
>> memory_encrypt_active() check to ensure that the function is no-op when
>> SME/SEV are not active.
>>
>> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
>> encryption mask from the kexec area. In case of SEV, we should not clear
>> the encryption mask.
> 
> Brijesh, I know all that.
> 
> Please read what I said here at the end:
> 
> https://lkml.kernel.org/r/20190324150034.GH23289@zn.tnic
> 
> With this change, the code looks like this:
> 
> +       if (sme_active())
> +               return set_memory_decrypted((unsigned long)vaddr, pages);
> 
> now in __set_memory_enc_dec via set_memory_decrypted():
> 
>          /* Nothing to do if memory encryption is not active */
>          if (!mem_encrypt_active())
>                  return 0;
> 
> 
> so you have:
> 
> 	if (sme_active())
> 
> 		...
> 
> 		if (!mem_encrypt_active())
> 
> 
> now maybe this is all clear to you and Tom but I betcha others will get
> confused. Probably something like "well, what should be active now, SME,
> SEV or memory encryption in general"?
> 
> I hope you're catching my drift.
> 
> So if you want to *not* decrypt memory in the SEV case, then doing something
> like this should make it a bit more clear:
> 
> 
> 	if (sev_active())
> 		return;
> 
> 	return set_memory_decrypted((unsigned long)vaddr, pages);
> 


I see your point. I agree it can get confusing.


> along with a comment *why* we're checking here.
> 
> But actually, I'd prefer if you had separate wrappers which are called
> for SME and for SEV.


Just a thought, maybe we can move the above if(sev_active()) check up in
kernel/kexec_core.c because we don't need to set/clear the encryption
masks when SEV is active so there is no need to call the wrapper.


> 
> I'll let Tom chime in too.
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Lendacky, Thomas]

On 3/25/19 1:17 PM, Singh, Brijesh wrote:
> 
> 
> On 3/25/19 12:32 PM, Borislav Petkov wrote:
>> On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
>>> By default all the memory regions are mapped encrypted. The
>>> set_memory_{encrypt,decrypt}() is a generic function which can be
>>> called explicitly to clear/set the encryption mask from the existing
>>> memory mapping. The mem_encrypt_active() returns true if either SEV or
>>> SME is active. So the __set_memory_enc_dec() uses the
>>> memory_encrypt_active() check to ensure that the function is no-op when
>>> SME/SEV are not active.
>>>
>>> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
>>> encryption mask from the kexec area. In case of SEV, we should not clear
>>> the encryption mask.
>>
>> Brijesh, I know all that.
>>
>> Please read what I said here at the end:
>>
>> https://lkml.kernel.org/r/20190324150034.GH23289@zn.tnic
>>
>> With this change, the code looks like this:
>>
>> +       if (sme_active())
>> +               return set_memory_decrypted((unsigned long)vaddr, pages);
>>
>> now in __set_memory_enc_dec via set_memory_decrypted():
>>
>>          /* Nothing to do if memory encryption is not active */
>>          if (!mem_encrypt_active())
>>                  return 0;
>>
>>
>> so you have:
>>
>> 	if (sme_active())
>>
>> 		...
>>
>> 		if (!mem_encrypt_active())
>>
>>
>> now maybe this is all clear to you and Tom but I betcha others will get
>> confused. Probably something like "well, what should be active now, SME,
>> SEV or memory encryption in general"?
>>
>> I hope you're catching my drift.
>>
>> So if you want to *not* decrypt memory in the SEV case, then doing something
>> like this should make it a bit more clear:
>>
>>
>> 	if (sev_active())
>> 		return;
>>
>> 	return set_memory_decrypted((unsigned long)vaddr, pages);
>>
> 
> 
> I see your point. I agree it can get confusing.
> 
> 
>> along with a comment *why* we're checking here.
>>
>> But actually, I'd prefer if you had separate wrappers which are called
>> for SME and for SEV.
> 
> 
> Just a thought, maybe we can move the above if(sev_active()) check up in
> kernel/kexec_core.c because we don't need to set/clear the encryption
> masks when SEV is active so there is no need to call the wrapper.

IIRC, the wrapper was created to avoid checking in the main kexec support
and move the check down to the arch specific support. So I don't think we
should move it.

I'm not sure about the separate wrappers, I like the above code where the
arch callback returns if sev_active() is true.

Maybe what would help is to describe why there is a difference between SME
and SEV in regards to kexec. During a traditional boot under SME, SME will
encrypt the kernel, so the SME kexec kernel also needs to be un-encrypted
in order to replicate a normal SME boot. During a traditional boot under
SEV, the kernel has already been loaded encrypted, so the SEV kexec kernel
needs to be encrypted in order to replicate a normal SEV boot.

Thanks,
Tom

> 
> 
>>
>> I'll let Tom chime in too.
>>
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[lijiang]

在 2019年03月26日 01:32, Borislav Petkov 写道:
> On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
>> By default all the memory regions are mapped encrypted. The
>> set_memory_{encrypt,decrypt}() is a generic function which can be
>> called explicitly to clear/set the encryption mask from the existing
>> memory mapping. The mem_encrypt_active() returns true if either SEV or 
>> SME is active. So the __set_memory_enc_dec() uses the
>> memory_encrypt_active() check to ensure that the function is no-op when
>> SME/SEV are not active.
>>
>> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
>> encryption mask from the kexec area. In case of SEV, we should not clear
>> the encryption mask.
> 
> Brijesh, I know all that.
> 
> Please read what I said here at the end:
> 
> https://lkml.kernel.org/r/20190324150034.GH23289@zn.tnic
> 
> With this change, the code looks like this:
> 
> +       if (sme_active())
> +               return set_memory_decrypted((unsigned long)vaddr, pages);
> 
> now in __set_memory_enc_dec via set_memory_decrypted():
> 
>         /* Nothing to do if memory encryption is not active */
>         if (!mem_encrypt_active())
>                 return 0;
> 
> 
> so you have:
> 
> 	if (sme_active())
> 
> 		...
> 
> 		if (!mem_encrypt_active())
> 
> 
> now maybe this is all clear to you and Tom but I betcha others will get
> confused. Probably something like "well, what should be active now, SME,
> SEV or memory encryption in general"?
> 
> I hope you're catching my drift.
> 
> So if you want to *not* decrypt memory in the SEV case, then doing something
> like this should make it a bit more clear:
> 
> 
> 	if (sev_active())
> 		return;
> 
> 	return set_memory_decrypted((unsigned long)vaddr, pages);
> 
> along with a comment *why* we're checking here.
It looks good to me. I will improve them next post.

Thank you, everyone.

Lianbo

> 
> But actually, I'd prefer if you had separate wrappers which are called
> for SME and for SEV.
> 
> I'll let Tom chime in too.
> 

Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Boris Petkov]

On March 25, 2019 8:59:28 PM GMT+01:00, "Lendacky, Thomas" <Thomas.Lendacky@amd.com> wrote:
>Maybe what would help is to describe why there is a difference between
>SME
>and SEV in regards to kexec. During a traditional boot under SME, SME
>will
>encrypt the kernel, so the SME kexec kernel also needs to be
>un-encrypted
>in order to replicate a normal SME boot. During a traditional boot
>under
>SEV, the kernel has already been loaded encrypted, so the SEV kexec
>kernel
>needs to be encrypted in order to replicate a normal SEV boot.


Yah, that should be in a comment above that function.

Thx.

-- 
Sent from a small device: formatting sux and brevity is inevitable.