Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing

[Thomas Gleixner]

On Wed, 21 Jun 2017, Tom Lendacky wrote:
> On 6/21/2017 10:38 AM, Thomas Gleixner wrote:
> > /*
> >  * Sanitize CPU configuration and retrieve the modifier
> >  * for the initial pgdir entry which will be programmed
> >  * into CR3. Depends on enabled SME encryption, normally 0.
> >  */
> > call __startup_secondary_64
> > 
> >  addq$(init_top_pgt - __START_KERNEL_map), %rax
> > 
> > You can hide that stuff in C-code nicely without adding any cruft to the
> > ASM code.
> > 
> 
> Moving the call to verify_cpu into the C-code might be quite a bit of
> change.  Currently, the verify_cpu code is included code and not a
> global function.

Ah. Ok. I missed that.

> I can still do the __startup_secondary_64() function and then look to
> incorporate verify_cpu into both __startup_64() and
> __startup_secondary_64() as a post-patch to this series.

Yes, just having __startup_secondary_64() for now and there the extra bits
for that encryption stuff is fine.

> At least the secondary path will have a base C routine to which
> modifications can be made in the future if needed.  How does that sound?

Sounds like a plan.

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

Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with memory encryption

[Tom Lendacky]

On 6/21/2017 11:59 AM, Borislav Petkov wrote:

On Wed, Jun 21, 2017 at 05:37:22PM +0200, Joerg Roedel wrote:

Do you mean this is like the last exception case in that document above:

"
   - Pointers to data structures in coherent memory which might be modified
 by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
 used by a network adapter, where that adapter changes pointers to
 indicate which descriptors have been processed, is an example of this
 type of situation."

?


So currently (without this patch) the build_completion_wait function
does not take a volatile parameter, only wait_on_sem() does.

Wait_on_sem() needs it because its purpose is to poll a memory location
which is changed by the iommu-hardware when its done with command
processing.


Right, the reason above - memory modifiable by an IO device. You could
add a comment there explaining the need for the volatile.


But the 'volatile' in build_completion_wait() looks unnecessary, because
the function does not poll the memory location. It only uses the
pointer, converts it to a physical address and writes it to the command
to be queued.


Ok.


Ok, so the (now) current version of the patch that doesn't change the
function signature is the right way to go.

Thanks,
Tom



Thanks.



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

Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing

[Tom Lendacky]

On 6/21/2017 10:38 AM, Thomas Gleixner wrote:

On Wed, 21 Jun 2017, Tom Lendacky wrote:

On 6/21/2017 2:16 AM, Thomas Gleixner wrote:

Why is this an unconditional function? Isn't the mask simply 0 when the MEM
ENCRYPT support is disabled?


I made it unconditional because of the call from head_64.S. I can't make
use of the C level static inline function and since the mask is not a
variable if CONFIG_AMD_MEM_ENCRYPT is not configured (#defined to 0) I
can't reference the variable directly.

I could create a #define in head_64.S that changes this to load rax with
the variable if CONFIG_AMD_MEM_ENCRYPT is configured or a zero if it's
not or add a #ifdef at that point in the code directly. Thoughts on
that?


See below.


That does not make any sense. Neither the call to sme_encrypt_kernel() nor
the following call to sme_get_me_mask().

__startup_64() is already C code, so why can't you simply call that from
__startup_64() in C and return the mask from there?


I was trying to keep it explicit as to what was happening, but I can
move those calls into __startup_64().


That's much preferred. And the return value wants to be documented in both
C and ASM code.


Will do.




I'll still need the call to sme_get_me_mask() in the secondary_startup_64
path, though (depending on your thoughts to the above response).


 call verify_cpu

 movq$(init_top_pgt - __START_KERNEL_map), %rax

So if you make that:

/*
 * Sanitize CPU configuration and retrieve the modifier
 * for the initial pgdir entry which will be programmed
 * into CR3. Depends on enabled SME encryption, normally 0.
 */
call __startup_secondary_64

 addq$(init_top_pgt - __START_KERNEL_map), %rax

You can hide that stuff in C-code nicely without adding any cruft to the
ASM code.



Moving the call to verify_cpu into the C-code might be quite a bit of
change.  Currently, the verify_cpu code is included code and not a
global function.  I can still do the __startup_secondary_64() function
and then look to incorporate verify_cpu into both __startup_64() and
__startup_secondary_64() as a post-patch to this series. At least the
secondary path will have a base C routine to which modifications can
be made in the future if needed.  How does that sound?

Thanks,
Tom


Thanks,

tglx



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

Re: [PATCH] s390/crash: Fix KEXEC_NOTE_BYTES definition

[Michael Holzheu]

Am Fri,  9 Jun 2017 10:17:05 +0800
schrieb Xunlei Pang :

> S390 KEXEC_NOTE_BYTES is not used by note_buf_t as before, which
> is now defined as follows:
> typedef u32 note_buf_t[CRASH_CORE_NOTE_BYTES/4];
> It was changed by the CONFIG_CRASH_CORE feature.
> 
> This patch gets rid of all the old KEXEC_NOTE_BYTES stuff, and
> renames KEXEC_NOTE_BYTES to CRASH_CORE_NOTE_BYTES for S390.
> 
> Fixes: 692f66f26a4c ("crash: move crashkernel parsing and vmcore related code 
> under CONFIG_CRASH_CORE")
> Cc: Dave Young 
> Cc: Dave Anderson 
> Cc: Hari Bathini 
> Cc: Gustavo Luiz Duarte 
> Signed-off-by: Xunlei Pang 

Hello Xunlei,

As you already know on s390 we create the ELF header in the new kernel.
Therefore we don't use the per-cpu buffers for ELF notes to store
the register state.

For RHEL7 we still store the registers in machine_kexec.c:add_elf_notes().
Though we also use the ELF header from new kernel ...

We assume your original problem with the "kmem -s" failure
was caused by the memory overwrite due to the invalid size of the
"crash_notes" per-cpu buffers.

Therefore your patch looks good for RHEL7 but for upstream we propose the
patch below.
---
[PATCH] s390/crash: Remove unused KEXEC_NOTE_BYTES

After commmit 692f66f26a4c19 ("crash: move crashkernel parsing and vmcore
related code under CONFIG_CRASH_CORE") the KEXEC_NOTE_BYTES macro is not
used anymore and for s390 we create the ELF header in the new kernel
anyway. Therefore remove the macro.

Reported-by: Xunlei Pang 
Reviewed-by: Mikhail Zaslonko 
Signed-off-by: Michael Holzheu 
---
 arch/s390/include/asm/kexec.h | 18 --
 include/linux/crash_core.h|  5 +
 include/linux/kexec.h |  9 -
 3 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
index 2f924bc30e35..dccf24ee26d3 100644
--- a/arch/s390/include/asm/kexec.h
+++ b/arch/s390/include/asm/kexec.h
@@ -41,24 +41,6 @@
 /* The native architecture */
 #define KEXEC_ARCH KEXEC_ARCH_S390
 
-/*
- * Size for s390x ELF notes per CPU
- *
- * Seven notes plus zero note at the end: prstatus, fpregset, timer,
- * tod_cmp, tod_reg, control regs, and prefix
- */
-#define KEXEC_NOTE_BYTES \
-   (ALIGN(sizeof(struct elf_note), 4) * 8 + \
-ALIGN(sizeof("CORE"), 4) * 7 + \
-ALIGN(sizeof(struct elf_prstatus), 4) + \
-ALIGN(sizeof(elf_fpregset_t), 4) + \
-ALIGN(sizeof(u64), 4) + \
-ALIGN(sizeof(u64), 4) + \
-ALIGN(sizeof(u32), 4) + \
-ALIGN(sizeof(u64) * 16, 4) + \
-ALIGN(sizeof(u32), 4) \
-   )
-
 /* Provide a dummy definition to avoid build failures. */
 static inline void crash_setup_regs(struct pt_regs *newregs,
struct pt_regs *oldregs) { }
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 541a197ba4a2..4090a42578a8 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -10,6 +10,11 @@
 #define CRASH_CORE_NOTE_NAME_BYTES ALIGN(sizeof(CRASH_CORE_NOTE_NAME), 4)
 #define CRASH_CORE_NOTE_DESC_BYTES ALIGN(sizeof(struct elf_prstatus), 4)
 
+/*
+ * The per-cpu notes area is a list of notes terminated by a "NULL"
+ * note header.  For kdump, the code in vmcore.c runs in the context
+ * of the second kernel to combine them into one note.
+ */
 #define CRASH_CORE_NOTE_BYTES ((CRASH_CORE_NOTE_HEAD_BYTES * 2) +  \
 CRASH_CORE_NOTE_NAME_BYTES +   \
 CRASH_CORE_NOTE_DESC_BYTES)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index c9481ebcbc0c..65888418fb69 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -63,15 +63,6 @@
 #define KEXEC_CORE_NOTE_NAME   CRASH_CORE_NOTE_NAME
 
 /*
- * The per-cpu notes area is a list of notes terminated by a "NULL"
- * note header.  For kdump, the code in vmcore.c runs in the context
- * of the second kernel to combine them into one note.
- */
-#ifndef KEXEC_NOTE_BYTES
-#define KEXEC_NOTE_BYTES   CRASH_CORE_NOTE_BYTES
-#endif
-
-/*
  * This structure is used to hold the arguments that are used when loading
  * kernel binaries.
  */
-- 
2.11.2


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

Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with memory encryption

[Borislav Petkov]

On Wed, Jun 21, 2017 at 05:37:22PM +0200, Joerg Roedel wrote:
> > Do you mean this is like the last exception case in that document above:
> > 
> > "
> >   - Pointers to data structures in coherent memory which might be modified
> > by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
> > used by a network adapter, where that adapter changes pointers to
> > indicate which descriptors have been processed, is an example of this
> > type of situation."
> > 
> > ?
> 
> So currently (without this patch) the build_completion_wait function
> does not take a volatile parameter, only wait_on_sem() does.
> 
> Wait_on_sem() needs it because its purpose is to poll a memory location
> which is changed by the iommu-hardware when its done with command
> processing.

Right, the reason above - memory modifiable by an IO device. You could
add a comment there explaining the need for the volatile.

> But the 'volatile' in build_completion_wait() looks unnecessary, because
> the function does not poll the memory location. It only uses the
> pointer, converts it to a physical address and writes it to the command
> to be queued.

Ok.

Thanks.

-- 
Regards/Gruss,
Boris.

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

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

Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing

[Thomas Gleixner]

On Wed, 21 Jun 2017, Tom Lendacky wrote:
> On 6/21/2017 2:16 AM, Thomas Gleixner wrote:
> > Why is this an unconditional function? Isn't the mask simply 0 when the MEM
> > ENCRYPT support is disabled?
> 
> I made it unconditional because of the call from head_64.S. I can't make
> use of the C level static inline function and since the mask is not a
> variable if CONFIG_AMD_MEM_ENCRYPT is not configured (#defined to 0) I
> can't reference the variable directly.
> 
> I could create a #define in head_64.S that changes this to load rax with
> the variable if CONFIG_AMD_MEM_ENCRYPT is configured or a zero if it's
> not or add a #ifdef at that point in the code directly. Thoughts on
> that?

See below.

> > That does not make any sense. Neither the call to sme_encrypt_kernel() nor
> > the following call to sme_get_me_mask().
> > 
> > __startup_64() is already C code, so why can't you simply call that from
> > __startup_64() in C and return the mask from there?
> 
> I was trying to keep it explicit as to what was happening, but I can
> move those calls into __startup_64().

That's much preferred. And the return value wants to be documented in both
C and ASM code.

> I'll still need the call to sme_get_me_mask() in the secondary_startup_64
> path, though (depending on your thoughts to the above response).

call verify_cpu

movq$(init_top_pgt - __START_KERNEL_map), %rax

So if you make that:

/*
 * Sanitize CPU configuration and retrieve the modifier
 * for the initial pgdir entry which will be programmed
 * into CR3. Depends on enabled SME encryption, normally 0.
 */
call __startup_secondary_64

addq$(init_top_pgt - __START_KERNEL_map), %rax

You can hide that stuff in C-code nicely without adding any cruft to the
ASM code.

Thanks,

tglx

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

Re: [PATCH v7 25/36] swiotlb: Add warnings for use of bounce buffers with SME

[Tom Lendacky]

On 6/21/2017 5:50 AM, Borislav Petkov wrote:

On Fri, Jun 16, 2017 at 01:54:36PM -0500, Tom Lendacky wrote:

Add warnings to let the user know when bounce buffers are being used for
DMA when SME is active.  Since the bounce buffers are not in encrypted
memory, these notifications are to allow the user to determine some
appropriate action - if necessary.  Actions can range from utilizing an
IOMMU, replacing the device with another device that can support 64-bit
DMA, ignoring the message if the device isn't used much, etc.

Signed-off-by: Tom Lendacky 
---
  include/linux/dma-mapping.h |   11 +++
  include/linux/mem_encrypt.h |8 
  lib/swiotlb.c   |3 +++
  3 files changed, 22 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4f3eece..ee2307e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -10,6 +10,7 @@
  #include 
  #include 
  #include 
+#include 
  
  /**

   * List of possible attributes associated with a DMA mapping. The semantics
@@ -577,6 +578,11 @@ static inline int dma_set_mask(struct device *dev, u64 
mask)
  
  	if (!dev->dma_mask || !dma_supported(dev, mask))

return -EIO;
+
+   /* Since mask is unsigned, this can only be true if SME is active */
+   if (mask < sme_dma_mask())
+   dev_warn(dev, "SME is active, device will require DMA bounce 
buffers\n");
+
*dev->dma_mask = mask;
return 0;
  }
@@ -596,6 +602,11 @@ static inline int dma_set_coherent_mask(struct device 
*dev, u64 mask)
  {
if (!dma_supported(dev, mask))
return -EIO;
+
+   /* Since mask is unsigned, this can only be true if SME is active */
+   if (mask < sme_dma_mask())
+   dev_warn(dev, "SME is active, device will require DMA bounce 
buffers\n");


Looks to me like those two checks above need to be a:

void sme_check_mask(struct device *dev, u64 mask)
{
 if (!sme_me_mask)
 return;

 /* Since mask is unsigned, this can only be true if SME is active */
 if (mask < (((u64)sme_me_mask << 1) - 1))
 dev_warn(dev, "SME is active, device will require DMA bounce 
buffers\n");
}

which gets called and sme_dma_mask() is not really needed.


Makes a lot of sense, I'll update the patch.

Thanks,
Tom





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

Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing

[Tom Lendacky]

On 6/21/2017 2:16 AM, Thomas Gleixner wrote:

On Fri, 16 Jun 2017, Tom Lendacky wrote:

diff --git a/arch/x86/include/asm/mem_encrypt.h 
b/arch/x86/include/asm/mem_encrypt.h
index a105796..988b336 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -15,16 +15,24 @@
  
  #ifndef __ASSEMBLY__
  
+#include 

+
  #ifdef CONFIG_AMD_MEM_ENCRYPT
  
  extern unsigned long sme_me_mask;
  
+void __init sme_enable(void);

+
  #else /* !CONFIG_AMD_MEM_ENCRYPT */
  
  #define sme_me_mask	0UL
  
+static inline void __init sme_enable(void) { }

+
  #endif/* CONFIG_AMD_MEM_ENCRYPT */
  
+unsigned long sme_get_me_mask(void);


Why is this an unconditional function? Isn't the mask simply 0 when the MEM
ENCRYPT support is disabled?


I made it unconditional because of the call from head_64.S. I can't make
use of the C level static inline function and since the mask is not a
variable if CONFIG_AMD_MEM_ENCRYPT is not configured (#defined to 0) I
can't reference the variable directly.

I could create a #define in head_64.S that changes this to load rax with
the variable if CONFIG_AMD_MEM_ENCRYPT is configured or a zero if it's
not or add a #ifdef at that point in the code directly. Thoughts on
that?




diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 6225550..ef12729 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -78,7 +78,29 @@ startup_64:
call__startup_64
popq%rsi
  
-	movq	$(early_top_pgt - __START_KERNEL_map), %rax

+   /*
+* Encrypt the kernel if SME is active.
+* The real_mode_data address is in %rsi and that register can be
+* clobbered by the called function so be sure to save it.
+*/
+   push%rsi
+   callsme_encrypt_kernel
+   pop %rsi


That does not make any sense. Neither the call to sme_encrypt_kernel() nor
the following call to sme_get_me_mask().

__startup_64() is already C code, so why can't you simply call that from
__startup_64() in C and return the mask from there?


I was trying to keep it explicit as to what was happening, but I can
move those calls into __startup_64(). I'll still need the call to
sme_get_me_mask() in the secondary_startup_64 path, though (depending on
your thoughts to the above response).




@@ -98,7 +120,20 @@ ENTRY(secondary_startup_64)
/* Sanitize CPU configuration */
call verify_cpu
  
-	movq	$(init_top_pgt - __START_KERNEL_map), %rax

+   /*
+* Get the SME encryption mask.
+*  The encryption mask will be returned in %rax so we do an ADD
+*  below to be sure that the encryption mask is part of the
+*  value that will stored in %cr3.
+*
+* The real_mode_data address is in %rsi and that register can be
+* clobbered by the called function so be sure to save it.
+*/
+   push%rsi
+   callsme_get_me_mask
+   pop %rsi


Do we really need a call here? The mask is established at this point, so
it's either 0 when the encryption stuff is not compiled in or it can be
retrieved from a variable which is accessible at this point.



Same as above, this can be updated based on the decided approach.

Thanks,
Tom


+
+   addq$(init_top_pgt - __START_KERNEL_map), %rax
  1:
  
  	/* Enable PAE mode, PGE and LA57 */


Thanks,

tglx



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

Re: [PATCH v7 07/36] x86/mm: Don't use phys_to_virt in ioremap() if SME is active

[Tom Lendacky]

On 6/21/2017 2:37 AM, Thomas Gleixner wrote:

On Fri, 16 Jun 2017, Tom Lendacky wrote:

Currently there is a check if the address being mapped is in the ISA
range (is_ISA_range()), and if it is then phys_to_virt() is used to
perform the mapping.  When SME is active, however, this will result
in the mapping having the encryption bit set when it is expected that
an ioremap() should not have the encryption bit set. So only use the
phys_to_virt() function if SME is not active

Reviewed-by: Borislav Petkov 
Signed-off-by: Tom Lendacky 
---
  arch/x86/mm/ioremap.c |7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 4c1b5fd..a382ba9 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -106,9 +107,11 @@ static void __iomem *__ioremap_caller(resource_size_t 
phys_addr,
}
  
  	/*

-* Don't remap the low PCI/ISA area, it's always mapped..
+* Don't remap the low PCI/ISA area, it's always mapped.
+*   But if SME is active, skip this so that the encryption bit
+*   doesn't get set.
 */
-   if (is_ISA_range(phys_addr, last_addr))
+   if (is_ISA_range(phys_addr, last_addr) && !sme_active())
return (__force void __iomem *)phys_to_virt(phys_addr);


More thoughts about that.

Making this conditional on !sme_active() is not the best idea. I'd rather
remove that whole thing and make it unconditional so the code pathes get
always exercised and any subtle wreckage is detected on a broader base and
not only on that hard to access and debug SME capable machine owned by Joe
User.


Ok, that sounds good.  I'll remove the check and usage of phys_to_virt()
and update the changelog with additional detail about that.

Thanks,
Tom



Thanks,

tglx



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

Re: [PATCH v7 07/36] x86/mm: Don't use phys_to_virt in ioremap() if SME is active

[Tom Lendacky]

On 6/20/2017 3:55 PM, Thomas Gleixner wrote:

On Fri, 16 Jun 2017, Tom Lendacky wrote:


Currently there is a check if the address being mapped is in the ISA
range (is_ISA_range()), and if it is then phys_to_virt() is used to
perform the mapping.  When SME is active, however, this will result
in the mapping having the encryption bit set when it is expected that
an ioremap() should not have the encryption bit set. So only use the
phys_to_virt() function if SME is not active


This does not make sense to me. What the heck has phys_to_virt() to do with
the encryption bit. Especially why would the encryption bit be set on that
mapping in the first place?


The default is that all entries that get added to the pagetables have
the encryption bit set unless specifically overridden.  Any __va() or
phys_to_virt() calls will result in a pagetable mapping that has the
encryption bit set.  For ioremap, the PAGE_KERNEL_IO protection is used
which will not/does not have the encryption bit set.



I'm probably missing something, but this want's some coherent explanation
understandable by mere mortals both in the changelog and the code comment.


I'll add some additional info to the changelog and code.

Thanks,
Tom



Thanks,

tglx



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

Re: [PATCH v7 06/36] x86/mm: Add Secure Memory Encryption (SME) support

[Tom Lendacky]

On 6/20/2017 3:49 PM, Thomas Gleixner wrote:

On Fri, 16 Jun 2017, Tom Lendacky wrote:
  
+config ARCH_HAS_MEM_ENCRYPT

+   def_bool y
+   depends on X86


That one is silly. The config switch is in the x86 KConfig file, so X86 is
on. If you intended to move this to some generic place outside of
x86/Kconfig then this should be

config ARCH_HAS_MEM_ENCRYPT
bool

and x86/Kconfig should have

select ARCH_HAS_MEM_ENCRYPT

and that should be selected by AMD_MEM_ENCRYPT


This is used for deciding whether to include the asm/mem_encrypt.h file
so it needs to be on whether AMD_MEM_ENCRYPT is configured or not. I'll
leave it in the x86/Kconfig file and remove the depends on line.

Thanks,
Tom




+config AMD_MEM_ENCRYPT
+   bool "AMD Secure Memory Encryption (SME) support"
+   depends on X86_64 && CPU_SUP_AMD
+   ---help---
+ Say yes to enable support for the encryption of system memory.
+ This requires an AMD processor that supports Secure Memory
+ Encryption (SME).


Thanks,

tglx



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

Re: [PATCH v7 23/36] x86, realmode: Decrypt trampoline area if memory encryption is active

[Borislav Petkov]

On Fri, Jun 16, 2017 at 01:54:12PM -0500, Tom Lendacky wrote:
> When Secure Memory Encryption is enabled, the trampoline area must not
> be encrypted. A CPU running in real mode will not be able to decrypt
> memory that has been encrypted because it will not be able to use addresses
> with the memory encryption mask.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/realmode/init.c |8 
>  1 file changed, 8 insertions(+)

Subject: x86/realmode: ...

other than that:

Reviewed-by: Borislav Petkov 

-- 
Regards/Gruss,
Boris.

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

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

Re: [PATCH v7 24/36] x86, swiotlb: Add memory encryption support

[Borislav Petkov]

On Fri, Jun 16, 2017 at 01:54:24PM -0500, Tom Lendacky wrote:
> Since DMA addresses will effectively look like 48-bit addresses when the
> memory encryption mask is set, SWIOTLB is needed if the DMA mask of the
> device performing the DMA does not support 48-bits. SWIOTLB will be
> initialized to create decrypted bounce buffers for use by these devices.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/dma-mapping.h |5 ++-
>  arch/x86/include/asm/mem_encrypt.h |5 +++
>  arch/x86/kernel/pci-dma.c  |   11 +--
>  arch/x86/kernel/pci-nommu.c|2 +
>  arch/x86/kernel/pci-swiotlb.c  |   15 +-
>  arch/x86/mm/mem_encrypt.c  |   22 +++
>  include/linux/swiotlb.h|1 +
>  init/main.c|   10 +++
>  lib/swiotlb.c  |   54 
> +++-
>  9 files changed, 108 insertions(+), 17 deletions(-)

Reviewed-by: Borislav Petkov 

-- 
Regards/Gruss,
Boris.

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

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

Re: [PATCH] s390/crash: Fix KEXEC_NOTE_BYTES definition

[Michael Holzheu]

Hi Xunlei,

Sorry for the late reply - I was on vacation up to now.
Give us some time to look into this issue.

Michael

Am Fri,  9 Jun 2017 10:17:05 +0800
schrieb Xunlei Pang :

> S390 KEXEC_NOTE_BYTES is not used by note_buf_t as before, which
> is now defined as follows:
> typedef u32 note_buf_t[CRASH_CORE_NOTE_BYTES/4];
> It was changed by the CONFIG_CRASH_CORE feature.
> 
> This patch gets rid of all the old KEXEC_NOTE_BYTES stuff, and
> renames KEXEC_NOTE_BYTES to CRASH_CORE_NOTE_BYTES for S390.
> 
> Fixes: 692f66f26a4c ("crash: move crashkernel parsing and vmcore related code 
> under CONFIG_CRASH_CORE")
> Cc: Dave Young 
> Cc: Dave Anderson 
> Cc: Hari Bathini 
> Cc: Gustavo Luiz Duarte 
> Signed-off-by: Xunlei Pang 
> ---
>  arch/s390/include/asm/kexec.h |  2 +-
>  include/linux/crash_core.h|  7 +++
>  include/linux/kexec.h | 11 +--
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
> index 2f924bc..352deb8 100644
> --- a/arch/s390/include/asm/kexec.h
> +++ b/arch/s390/include/asm/kexec.h
> @@ -47,7 +47,7 @@
>   * Seven notes plus zero note at the end: prstatus, fpregset, timer,
>   * tod_cmp, tod_reg, control regs, and prefix
>   */
> -#define KEXEC_NOTE_BYTES \
> +#define CRASH_CORE_NOTE_BYTES \
>   (ALIGN(sizeof(struct elf_note), 4) * 8 + \
>ALIGN(sizeof("CORE"), 4) * 7 + \
>ALIGN(sizeof(struct elf_prstatus), 4) + \
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index e9de6b4..dbc6e5c 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -10,9 +10,16 @@
>  #define CRASH_CORE_NOTE_NAME_BYTES ALIGN(sizeof(CRASH_CORE_NOTE_NAME), 4)
>  #define CRASH_CORE_NOTE_DESC_BYTES ALIGN(sizeof(struct elf_prstatus), 4)
> 
> +/*
> + * The per-cpu notes area is a list of notes terminated by a "NULL"
> + * note header.  For kdump, the code in vmcore.c runs in the context
> + * of the second kernel to combine them into one note.
> + */
> +#ifndef CRASH_CORE_NOTE_BYTES
>  #define CRASH_CORE_NOTE_BYTES   ((CRASH_CORE_NOTE_HEAD_BYTES * 2) +  
> \
>CRASH_CORE_NOTE_NAME_BYTES +   \
>CRASH_CORE_NOTE_DESC_BYTES)
> +#endif
> 
>  #define VMCOREINFO_BYTESPAGE_SIZE
>  #define VMCOREINFO_NOTE_NAME"VMCOREINFO"
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 3ea8275..133df03 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -14,7 +14,6 @@
> 
>  #if !defined(__ASSEMBLY__)
> 
> -#include 
>  #include 
> 
>  #include 
> @@ -25,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  /* Verify architecture specific macros are defined */
> 
> @@ -63,15 +63,6 @@
>  #define KEXEC_CORE_NOTE_NAME CRASH_CORE_NOTE_NAME
> 
>  /*
> - * The per-cpu notes area is a list of notes terminated by a "NULL"
> - * note header.  For kdump, the code in vmcore.c runs in the context
> - * of the second kernel to combine them into one note.
> - */
> -#ifndef KEXEC_NOTE_BYTES
> -#define KEXEC_NOTE_BYTES CRASH_CORE_NOTE_BYTES
> -#endif
> -
> -/*
>   * This structure is used to hold the arguments that are used when loading
>   * kernel binaries.
>   */


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

Re: [PATCH v7 20/36] x86, mpparse: Use memremap to map the mpf and mpc data

[Borislav Petkov]

On Fri, Jun 16, 2017 at 01:53:38PM -0500, Tom Lendacky wrote:
> The SMP MP-table is built by UEFI and placed in memory in a decrypted
> state. These tables are accessed using a mix of early_memremap(),
> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
> to use early_memremap()/early_memunmap(). This allows for proper setting
> of the encryption mask so that the data can be successfully accessed when
> SME is active.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/kernel/mpparse.c |   98 
> -
>  1 file changed, 70 insertions(+), 28 deletions(-)

Reviewed-by: Borislav Petkov 

Please put the conversion to pr_fmt() on the TODO list for later.

Thanks.

-- 
Regards/Gruss,
Boris.

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

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

Re: [PATCH v7 10/36] x86/mm: Provide general kernel support for memory encryption

[Borislav Petkov]

On Wed, Jun 21, 2017 at 09:18:59AM +0200, Thomas Gleixner wrote:
> That looks wrong. It's not decrypted it's rather unencrypted, right?

Yeah, it previous versions of the patchset, "decrypted" and
"unencrypted" were both present so we settled on "decrypted" for the
nomenclature.

-- 
Regards/Gruss,
Boris.

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

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

Re: [PATCH v7 10/36] x86/mm: Provide general kernel support for memory encryption

[Thomas Gleixner]

On Fri, 16 Jun 2017, Tom Lendacky wrote:
>  
> +#ifndef pgprot_encrypted
> +#define pgprot_encrypted(prot)   (prot)
> +#endif
> +
> +#ifndef pgprot_decrypted

That looks wrong. It's not decrypted it's rather unencrypted, right?

Thanks,

tglx

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

Re: [PATCH v7 08/36] x86/mm: Add support to enable SME in early boot processing

[Thomas Gleixner]

On Fri, 16 Jun 2017, Tom Lendacky wrote:
> diff --git a/arch/x86/include/asm/mem_encrypt.h 
> b/arch/x86/include/asm/mem_encrypt.h
> index a105796..988b336 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -15,16 +15,24 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include 
> +
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  
>  extern unsigned long sme_me_mask;
>  
> +void __init sme_enable(void);
> +
>  #else/* !CONFIG_AMD_MEM_ENCRYPT */
>  
>  #define sme_me_mask  0UL
>  
> +static inline void __init sme_enable(void) { }
> +
>  #endif   /* CONFIG_AMD_MEM_ENCRYPT */
>  
> +unsigned long sme_get_me_mask(void);

Why is this an unconditional function? Isn't the mask simply 0 when the MEM
ENCRYPT support is disabled?

> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 6225550..ef12729 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -78,7 +78,29 @@ startup_64:
>   call__startup_64
>   popq%rsi
>  
> - movq$(early_top_pgt - __START_KERNEL_map), %rax
> + /*
> +  * Encrypt the kernel if SME is active.
> +  * The real_mode_data address is in %rsi and that register can be
> +  * clobbered by the called function so be sure to save it.
> +  */
> + push%rsi
> + callsme_encrypt_kernel
> + pop %rsi

That does not make any sense. Neither the call to sme_encrypt_kernel() nor
the following call to sme_get_me_mask().

__startup_64() is already C code, so why can't you simply call that from
__startup_64() in C and return the mask from there?

> @@ -98,7 +120,20 @@ ENTRY(secondary_startup_64)
>   /* Sanitize CPU configuration */
>   call verify_cpu
>  
> - movq$(init_top_pgt - __START_KERNEL_map), %rax
> + /*
> +  * Get the SME encryption mask.
> +  *  The encryption mask will be returned in %rax so we do an ADD
> +  *  below to be sure that the encryption mask is part of the
> +  *  value that will stored in %cr3.
> +  *
> +  * The real_mode_data address is in %rsi and that register can be
> +  * clobbered by the called function so be sure to save it.
> +  */
> + push%rsi
> + callsme_get_me_mask
> + pop %rsi

Do we really need a call here? The mask is established at this point, so
it's either 0 when the encryption stuff is not compiled in or it can be
retrieved from a variable which is accessible at this point.

> +
> + addq$(init_top_pgt - __START_KERNEL_map), %rax
>  1:
>  
>   /* Enable PAE mode, PGE and LA57 */

Thanks,

tglx

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