Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/08/2016 08:55 AM, Borislav Petkov wrote: > On Thu, Sep 08, 2016 at 08:26:27AM -0500, Tom Lendacky wrote: >> When does this value get initialized? Since _PAGE_ENC is #defined to >> sme_me_mask, which is not set until the boot process begins, I'm afraid >> we'd end up using the initial value of sme_me_mask, which is zero. Do >> I have that right? > > Hmm, but then that would hold true for all the other defines where you > OR-in _PAGE_ENC, no? As long as the #define is not a global variable like this one it's ok. > > In any case, the preprocessed source looks like this: > > pmdval_t early_pmd_flags = (((pteval_t)(1)) << 0) | (((pteval_t)(1)) << > 1) | (((pteval_t)(1)) << 6) | (((pteval_t)(1)) << 5) | (((pteval_t)(1)) << > 8)) | (((pteval_t)(1)) << 63)) | (((pteval_t)(1)) << 7)) | sme_me_mask) & > ~pteval_t)(1)) << 8) | (((pteval_t)(1)) << 63)); > > but the problem is later, when building: > > arch/x86/kernel/head64.c:39:28: error: initializer element is not constant > pmdval_t early_pmd_flags = (__PAGE_KERNEL_LARGE | _PAGE_ENC) & > ~(_PAGE_GLOBAL | _PAGE_NX); > ^ > scripts/Makefile.build:153: recipe for target 'arch/x86/kernel/head64.s' > failed > > so I guess not. :-\ > > Ok, then at least please put the early_pmd_flags init after > sme_early_init() along with a small comment explaning what happens. Let me verify that we won't possibly take any kind of page fault during sme_early_init() and cause a page to be not be marked encrypted. Or... I can add a comment indicating I need to set this as early as possible to cover any page faults that might occur. Thanks, Tom > > Thanks. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On Thu, Sep 08, 2016 at 08:26:27AM -0500, Tom Lendacky wrote: > When does this value get initialized? Since _PAGE_ENC is #defined to > sme_me_mask, which is not set until the boot process begins, I'm afraid > we'd end up using the initial value of sme_me_mask, which is zero. Do > I have that right? Hmm, but then that would hold true for all the other defines where you OR-in _PAGE_ENC, no? In any case, the preprocessed source looks like this: pmdval_t early_pmd_flags = (((pteval_t)(1)) << 0) | (((pteval_t)(1)) << 1) | (((pteval_t)(1)) << 6) | (((pteval_t)(1)) << 5) | (((pteval_t)(1)) << 8)) | (((pteval_t)(1)) << 63)) | (((pteval_t)(1)) << 7)) | sme_me_mask) & ~pteval_t)(1)) << 8) | (((pteval_t)(1)) << 63)); but the problem is later, when building: arch/x86/kernel/head64.c:39:28: error: initializer element is not constant pmdval_t early_pmd_flags = (__PAGE_KERNEL_LARGE | _PAGE_ENC) & ~(_PAGE_GLOBAL | _PAGE_NX); ^ scripts/Makefile.build:153: recipe for target 'arch/x86/kernel/head64.s' failed so I guess not. :-\ Ok, then at least please put the early_pmd_flags init after sme_early_init() along with a small comment explaning what happens. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/07/2016 10:55 AM, Borislav Petkov wrote: > On Wed, Sep 07, 2016 at 09:30:54AM -0500, Tom Lendacky wrote: >> _PAGE_ENC is #defined as sme_me_mask and sme_me_mask has already been >> set (or not set) at this point - so it will be the mask if SME is >> active or 0 if SME is not active. > > Yeah, I remember :-) > >> sme_early_init() is merely propagating the mask to other structures. >> Since early_pmd_flags is mainly used in this file (one line in >> head_64.S is the other place) I felt it best to modify it here. But it >> can always be moved if you feel that is best. > > Hmm, so would it work then if you stick it in early_pmd_flags' > definition like you do with the other masks? I.e., > > pmdval_t early_pmd_flags = __PAGE_KERNEL_LARGE | _PAGE_ENC & ~(_PAGE_GLOBAL | > _PAGE_NX); When does this value get initialized? Since _PAGE_ENC is #defined to sme_me_mask, which is not set until the boot process begins, I'm afraid we'd end up using the initial value of sme_me_mask, which is zero. Do I have that right? Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On Wed, Sep 07, 2016 at 09:30:54AM -0500, Tom Lendacky wrote: > _PAGE_ENC is #defined as sme_me_mask and sme_me_mask has already been > set (or not set) at this point - so it will be the mask if SME is > active or 0 if SME is not active. Yeah, I remember :-) > sme_early_init() is merely propagating the mask to other structures. > Since early_pmd_flags is mainly used in this file (one line in > head_64.S is the other place) I felt it best to modify it here. But it > can always be moved if you feel that is best. Hmm, so would it work then if you stick it in early_pmd_flags' definition like you do with the other masks? I.e., pmdval_t early_pmd_flags = __PAGE_KERNEL_LARGE | _PAGE_ENC & ~(_PAGE_GLOBAL | _PAGE_NX); -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/06/2016 04:31 AM, Borislav Petkov wrote: > On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: >> Adding general kernel support for memory encryption includes: >> - Modify and create some page table macros to include the Secure Memory >> Encryption (SME) memory encryption mask >> - Update kernel boot support to call an SME routine that checks for and >> sets the SME capability (the SME routine will grow later and for now >> is just a stub routine) >> - Update kernel boot support to call an SME routine that encrypts the >> kernel (the SME routine will grow later and for now is just a stub >> routine) >> - Provide an SME initialization routine to update the protection map with >> the memory encryption mask so that it is used by default >> >> Signed-off-by: Tom Lendacky> > ... > >> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c >> index 54a2372..88c7bae 100644 >> --- a/arch/x86/kernel/head64.c >> +++ b/arch/x86/kernel/head64.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> >> /* >> * Manage page tables very early on. >> @@ -42,7 +43,7 @@ static void __init reset_early_page_tables(void) >> { >> memset(early_level4_pgt, 0, sizeof(pgd_t)*(PTRS_PER_PGD-1)); >> next_early_pgt = 0; >> -write_cr3(__pa_nodebug(early_level4_pgt)); >> +write_cr3(__sme_pa_nodebug(early_level4_pgt)); >> } >> >> /* Create a new PMD entry */ >> @@ -54,7 +55,7 @@ int __init early_make_pgtable(unsigned long address) >> pmdval_t pmd, *pmd_p; >> >> /* Invalid address or early pgt is done ? */ >> -if (physaddr >= MAXMEM || read_cr3() != __pa_nodebug(early_level4_pgt)) >> +if (physaddr >= MAXMEM || read_cr3() != >> __sme_pa_nodebug(early_level4_pgt)) >> return -1; >> >> again: >> @@ -157,6 +158,11 @@ asmlinkage __visible void __init >> x86_64_start_kernel(char * real_mode_data) >> >> clear_page(init_level4_pgt); >> >> +/* Update the early_pmd_flags with the memory encryption mask */ >> +early_pmd_flags |= _PAGE_ENC; >> + >> +sme_early_init(); >> + > > So maybe this comes later but you're setting _PAGE_ENC unconditionally > *before* sme_early_init(). > > I think you should set it in sme_early_init() and iff SME is enabled. _PAGE_ENC is #defined as sme_me_mask and sme_me_mask has already been set (or not set) at this point - so it will be the mask if SME is active or 0 if SME is not active. sme_early_init() is merely propagating the mask to other structures. Since early_pmd_flags is mainly used in this file (one line in head_64.S is the other place) I felt it best to modify it here. But it can always be moved if you feel that is best. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/05/2016 10:22 AM, Borislav Petkov wrote: > On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: >> Adding general kernel support for memory encryption includes: >> - Modify and create some page table macros to include the Secure Memory >> Encryption (SME) memory encryption mask >> - Update kernel boot support to call an SME routine that checks for and >> sets the SME capability (the SME routine will grow later and for now >> is just a stub routine) >> - Update kernel boot support to call an SME routine that encrypts the >> kernel (the SME routine will grow later and for now is just a stub >> routine) >> - Provide an SME initialization routine to update the protection map with >> the memory encryption mask so that it is used by default >> >> Signed-off-by: Tom Lendacky> > ... > >> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S >> index c98a559..30f7715 100644 >> --- a/arch/x86/kernel/head_64.S >> +++ b/arch/x86/kernel/head_64.S >> @@ -95,6 +95,13 @@ startup_64: >> jnz bad_address >> >> /* >> + * Enable memory encryption (if available). Add the memory encryption >> + * mask to %rbp to include it in the the page table fixup. >> + */ >> +callsme_enable >> +addqsme_me_mask(%rip), %rbp >> + >> +/* >> * Fixup the physical addresses in the page table >> */ >> addq%rbp, early_level4_pgt + (L4_START_KERNEL*8)(%rip) >> @@ -116,7 +123,8 @@ startup_64: >> movq%rdi, %rax >> shrq$PGDIR_SHIFT, %rax >> >> -leaq(4096 + _KERNPG_TABLE)(%rbx), %rdx >> +leaq(4096 + __KERNPG_TABLE)(%rbx), %rdx >> +addqsme_me_mask(%rip), %rdx /* Apply mem encryption mask */ > > Please add comments over the line and not at the side... Ok, will do. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/05/2016 03:48 AM, Borislav Petkov wrote: > On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: >> Adding general kernel support for memory encryption includes: >> - Modify and create some page table macros to include the Secure Memory >> Encryption (SME) memory encryption mask >> - Update kernel boot support to call an SME routine that checks for and >> sets the SME capability (the SME routine will grow later and for now >> is just a stub routine) >> - Update kernel boot support to call an SME routine that encrypts the >> kernel (the SME routine will grow later and for now is just a stub >> routine) >> - Provide an SME initialization routine to update the protection map with >> the memory encryption mask so that it is used by default >> >> Signed-off-by: Tom Lendacky>> --- > > ... > >> diff --git a/arch/x86/include/asm/pgtable_types.h >> b/arch/x86/include/asm/pgtable_types.h >> index f1218f5..a01f0e1 100644 >> --- a/arch/x86/include/asm/pgtable_types.h >> +++ b/arch/x86/include/asm/pgtable_types.h >> @@ -3,6 +3,7 @@ >> >> #include >> #include >> +#include >> >> #define FIRST_USER_ADDRESS 0UL >> >> @@ -121,9 +122,9 @@ >> >> #define _PAGE_PROTNONE (_AT(pteval_t, 1) << _PAGE_BIT_PROTNONE) >> >> -#define _PAGE_TABLE (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |\ >> +#define __PAGE_TABLE(_PAGE_PRESENT | _PAGE_RW | _PAGE_USER | >> \ >> _PAGE_ACCESSED | _PAGE_DIRTY) > > Hmm, so this naming looks confusing and error-prone: the only difference > is a single "_". > > How about this instead: > > #define _PAGE_TABLE_NO_ENC(_PAGE_PRESENT | _PAGE_RW | _PAGE_USER | > \ >_PAGE_ACCESSED | _PAGE_DIRTY) > > #define _PAGE_TABLE (_PAGE_TABLE_NO_ENC | _PAGE_ENC) > > Or call it _PAGE_TABLE_BASE or whatever. > > Ditto for __KERNPG_TABLE. > > This way you can differentiate between the two and use the _NO_ENC one > to define _PAGE_TABLE. And it will be absolutely clear when you use the > _NO_ENC one, what you mean and that you don't want to have the enc mask > in the PTE. > > Should be less confusing IMO too. Yup, makes sense. I'll rework/rename. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On 09/02/2016 01:14 PM, Borislav Petkov wrote: > On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: >> Adding general kernel support for memory encryption includes: >> - Modify and create some page table macros to include the Secure Memory >> Encryption (SME) memory encryption mask >> - Update kernel boot support to call an SME routine that checks for and >> sets the SME capability (the SME routine will grow later and for now >> is just a stub routine) >> - Update kernel boot support to call an SME routine that encrypts the >> kernel (the SME routine will grow later and for now is just a stub >> routine) >> - Provide an SME initialization routine to update the protection map with >> the memory encryption mask so that it is used by default >> >> Signed-off-by: Tom Lendacky>> --- > > ... > >> diff --git a/arch/x86/include/asm/mem_encrypt.h >> b/arch/x86/include/asm/mem_encrypt.h >> index 747fc52..9f3e762 100644 >> --- a/arch/x86/include/asm/mem_encrypt.h >> +++ b/arch/x86/include/asm/mem_encrypt.h >> @@ -15,12 +15,21 @@ >> >> #ifndef __ASSEMBLY__ >> >> +#include >> + >> #ifdef CONFIG_AMD_MEM_ENCRYPT >> >> extern unsigned long sme_me_mask; >> >> u8 sme_get_me_loss(void); >> >> +void __init sme_early_init(void); >> + >> +#define __sme_pa(x) (__pa((x)) | sme_me_mask) >> +#define __sme_pa_nodebug(x) (__pa_nodebug((x)) | sme_me_mask) >> + >> +#define __sme_va(x) (__va((x) & ~sme_me_mask)) > > So I'm wondering: why not push the masking off of the SME mask into the > __va() macro instead of defining a specific __sme_va() one? > > I mean, do you even see cases where __va() would need to have to > sme_mask left in the virtual address? > > Because if not, you could mask it out in __va() so that all __va() users > get the "clean" va, without the enc bits. That's a good point, yes, it could go in __va(). I'll make that change. > > Hmmm. > > Btw, this patch is huuuge. It would be nice if you could split it, if > possible... Ok, I'll look at how to make this a bit more manageable. Thanks, Tom > > Thanks. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: > Adding general kernel support for memory encryption includes: > - Modify and create some page table macros to include the Secure Memory > Encryption (SME) memory encryption mask > - Update kernel boot support to call an SME routine that checks for and > sets the SME capability (the SME routine will grow later and for now > is just a stub routine) > - Update kernel boot support to call an SME routine that encrypts the > kernel (the SME routine will grow later and for now is just a stub > routine) > - Provide an SME initialization routine to update the protection map with > the memory encryption mask so that it is used by default > > Signed-off-by: Tom Lendacky... > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index c98a559..30f7715 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -95,6 +95,13 @@ startup_64: > jnz bad_address > > /* > + * Enable memory encryption (if available). Add the memory encryption > + * mask to %rbp to include it in the the page table fixup. > + */ > + callsme_enable > + addqsme_me_mask(%rip), %rbp > + > + /* >* Fixup the physical addresses in the page table >*/ > addq%rbp, early_level4_pgt + (L4_START_KERNEL*8)(%rip) > @@ -116,7 +123,8 @@ startup_64: > movq%rdi, %rax > shrq$PGDIR_SHIFT, %rax > > - leaq(4096 + _KERNPG_TABLE)(%rbx), %rdx > + leaq(4096 + __KERNPG_TABLE)(%rbx), %rdx > + addqsme_me_mask(%rip), %rdx /* Apply mem encryption mask */ Please add comments over the line and not at the side... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: > Adding general kernel support for memory encryption includes: > - Modify and create some page table macros to include the Secure Memory > Encryption (SME) memory encryption mask > - Update kernel boot support to call an SME routine that checks for and > sets the SME capability (the SME routine will grow later and for now > is just a stub routine) > - Update kernel boot support to call an SME routine that encrypts the > kernel (the SME routine will grow later and for now is just a stub > routine) > - Provide an SME initialization routine to update the protection map with > the memory encryption mask so that it is used by default > > Signed-off-by: Tom Lendacky> --- ... > diff --git a/arch/x86/include/asm/pgtable_types.h > b/arch/x86/include/asm/pgtable_types.h > index f1218f5..a01f0e1 100644 > --- a/arch/x86/include/asm/pgtable_types.h > +++ b/arch/x86/include/asm/pgtable_types.h > @@ -3,6 +3,7 @@ > > #include > #include > +#include > > #define FIRST_USER_ADDRESS 0UL > > @@ -121,9 +122,9 @@ > > #define _PAGE_PROTNONE (_AT(pteval_t, 1) << _PAGE_BIT_PROTNONE) > > -#define _PAGE_TABLE (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |\ > +#define __PAGE_TABLE (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |\ >_PAGE_ACCESSED | _PAGE_DIRTY) Hmm, so this naming looks confusing and error-prone: the only difference is a single "_". How about this instead: #define _PAGE_TABLE_NO_ENC (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER | \ _PAGE_ACCESSED | _PAGE_DIRTY) #define _PAGE_TABLE (_PAGE_TABLE_NO_ENC | _PAGE_ENC) Or call it _PAGE_TABLE_BASE or whatever. Ditto for __KERNPG_TABLE. This way you can differentiate between the two and use the _NO_ENC one to define _PAGE_TABLE. And it will be absolutely clear when you use the _NO_ENC one, what you mean and that you don't want to have the enc mask in the PTE. Should be less confusing IMO too. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 07/20] x86: Provide general kernel support for memory encryption
On Mon, Aug 22, 2016 at 05:36:46PM -0500, Tom Lendacky wrote: > Adding general kernel support for memory encryption includes: > - Modify and create some page table macros to include the Secure Memory > Encryption (SME) memory encryption mask > - Update kernel boot support to call an SME routine that checks for and > sets the SME capability (the SME routine will grow later and for now > is just a stub routine) > - Update kernel boot support to call an SME routine that encrypts the > kernel (the SME routine will grow later and for now is just a stub > routine) > - Provide an SME initialization routine to update the protection map with > the memory encryption mask so that it is used by default > > Signed-off-by: Tom Lendacky> --- ... > diff --git a/arch/x86/include/asm/mem_encrypt.h > b/arch/x86/include/asm/mem_encrypt.h > index 747fc52..9f3e762 100644 > --- a/arch/x86/include/asm/mem_encrypt.h > +++ b/arch/x86/include/asm/mem_encrypt.h > @@ -15,12 +15,21 @@ > > #ifndef __ASSEMBLY__ > > +#include > + > #ifdef CONFIG_AMD_MEM_ENCRYPT > > extern unsigned long sme_me_mask; > > u8 sme_get_me_loss(void); > > +void __init sme_early_init(void); > + > +#define __sme_pa(x) (__pa((x)) | sme_me_mask) > +#define __sme_pa_nodebug(x) (__pa_nodebug((x)) | sme_me_mask) > + > +#define __sme_va(x) (__va((x) & ~sme_me_mask)) So I'm wondering: why not push the masking off of the SME mask into the __va() macro instead of defining a specific __sme_va() one? I mean, do you even see cases where __va() would need to have to sme_mask left in the virtual address? Because if not, you could mask it out in __va() so that all __va() users get the "clean" va, without the enc bits. Hmmm. Btw, this patch is huuuge. It would be nice if you could split it, if possible... Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu