Hi, I have a question about this definition in
arch/x86/mm/mem_encrypt_identity.c:
#define PMD_FLAGS_LARGE (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
#define PMD_FLAGS_DEC PMD_FLAGS_LARGE
#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
(_PAGE_PAT | _PAGE_PWT))
_PAGE_CACHE_MASK and _PAGE_PAT are for 4k pages, not 2M pages. The
definition of PMD_FLAGS_DEC_WP clears the PSE bit by masking out
_PAGE_CACHE_MASK, and sets it again by setting _PAGE_PAT, resulting in
PMD_FLAGS_DEC_WP actually being write-through, not write-protected,
using PAT index 1.
Shouldn't the definition be
#define PMD_FLAGS_DEC_WP (PMD_FLAGS_DEC | _PAGE_PAT_LARGE | _PAGE_PWT)
for write-protected using PAT index 5?
I guess the difference doesn't actually matter for encrypt-in-place? But
mem_encrypt_boot.S takes pains to initialize PA5 to be write-protected,
and it looks like it won't actually be used.
Thanks.
On 11/8/20 10:37 AM, Arvind Sankar wrote:
> Hi, I have a question about this definition in
> arch/x86/mm/mem_encrypt_identity.c:
>
> #define PMD_FLAGS_LARGE (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>
> #define PMD_FLAGS_DEC PMD_FLAGS_LARGE
> #define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
> (_PAGE_PAT | _PAGE_PWT))
>
> _PAGE_CACHE_MASK and _PAGE_PAT are for 4k pages, not 2M pages. The
> definition of PMD_FLAGS_DEC_WP clears the PSE bit by masking out
> _PAGE_CACHE_MASK, and sets it again by setting _PAGE_PAT, resulting in
> PMD_FLAGS_DEC_WP actually being write-through, not write-protected,
> using PAT index 1.
>
> Shouldn't the definition be
>
> #define PMD_FLAGS_DEC_WP (PMD_FLAGS_DEC | _PAGE_PAT_LARGE | _PAGE_PWT)
>
> for write-protected using PAT index 5?
Yes it should. There should probably be a _PAGE_CACHE_MASK_LARGE
definition so that the end result is:
#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK_LARGE) | \
(_PAGE_PAT_LARGE | _PAGE_PWT)
>
> I guess the difference doesn't actually matter for encrypt-in-place? But
> mem_encrypt_boot.S takes pains to initialize PA5 to be write-protected,
> and it looks like it won't actually be used.
Given how early in the boot everything occurs and the cache flushing that
is performed related to the operations, it works. But this should be
fixed.
Are you planning on sending a patch?
Thanks,
Tom
>
> Thanks.
>
The PAT bit is in different locations for 4k and 2M/1G page table
entries.
Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
index for write-protected pages.
Remove a duplication definition of _PAGE_PAT_LARGE.
Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/include/asm/pgtable_types.h | 3 +--
arch/x86/mm/mem_encrypt_identity.c | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 816b31c68550..f24d7ef8fffa 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -155,6 +155,7 @@ enum page_cache_mode {
#define _PAGE_ENC (_AT(pteval_t, sme_me_mask))
#define _PAGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)
+#define _PAGE_LARGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT_LARGE)
#define _PAGE_NOCACHE (cachemode2protval(_PAGE_CACHE_MODE_UC))
#define _PAGE_CACHE_WP (cachemode2protval(_PAGE_CACHE_MODE_WP))
@@ -176,8 +177,6 @@ enum page_cache_mode {
#define __pgprot(x) ((pgprot_t) { (x) } )
#define __pg(x) __pgprot(x)
-#define _PAGE_PAT_LARGE (_AT(pteval_t, 1) << _PAGE_BIT_PAT_LARGE)
-
#define PAGE_NONE __pg( 0| 0| 0|___A| 0| 0| 0|___G)
#define PAGE_SHARED __pg(__PP|__RW|_USR|___A|__NX| 0| 0| 0)
#define PAGE_SHARED_EXEC __pg(__PP|__RW|_USR|___A| 0| 0| 0| 0)
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 733b983f3a89..6c5eb6f3f14f 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -45,8 +45,8 @@
#define PMD_FLAGS_LARGE (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
#define PMD_FLAGS_DEC PMD_FLAGS_LARGE
-#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
- (_PAGE_PAT | _PAGE_PWT))
+#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_LARGE_CACHE_MASK) | \
+ (_PAGE_PAT_LARGE | _PAGE_PWT))
#define PMD_FLAGS_ENC (PMD_FLAGS_LARGE | _PAGE_ENC)
--
2.26.2
On 11/9/20 11:35 AM, Arvind Sankar wrote:
> The PAT bit is in different locations for 4k and 2M/1G page table
> entries.
>
> Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
> caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
> and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
> index for write-protected pages.
>
> Remove a duplication definition of _PAGE_PAT_LARGE.
>
> Signed-off-by: Arvind Sankar <[email protected]>
Fixes: tag?
Tested-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/include/asm/pgtable_types.h | 3 +--
> arch/x86/mm/mem_encrypt_identity.c | 4 ++--
> 2 files changed, 3 insertions(+), 4 deletions(-)
>
On Mon, Nov 09, 2020 at 02:41:48PM -0600, Tom Lendacky wrote:
> On 11/9/20 11:35 AM, Arvind Sankar wrote:
> > The PAT bit is in different locations for 4k and 2M/1G page table
> > entries.
> >
> > Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
> > caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
> > and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
> > index for write-protected pages.
> >
> > Remove a duplication definition of _PAGE_PAT_LARGE.
> >
> > Signed-off-by: Arvind Sankar <[email protected]>
>
> Fixes: tag?
It's been broken since it was added in
6ebcb060713f ("x86/mm: Add support to encrypt the kernel in-place")
but the code has been restructured since then. I think it should be
backportable to 4.19.x if you want, except for that "duplication
definition"[sic] I removed, which was only added in v5.6. Do I need to
split that out into a separate patch?
>
> Tested-by: Tom Lendacky <[email protected]>
>
> > ---
> > arch/x86/include/asm/pgtable_types.h | 3 +--
> > arch/x86/mm/mem_encrypt_identity.c | 4 ++--
> > 2 files changed, 3 insertions(+), 4 deletions(-)
> >
On 11/9/20 3:42 PM, Arvind Sankar wrote:
> On Mon, Nov 09, 2020 at 02:41:48PM -0600, Tom Lendacky wrote:
>> On 11/9/20 11:35 AM, Arvind Sankar wrote:
>>> The PAT bit is in different locations for 4k and 2M/1G page table
>>> entries.
>>>
>>> Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
>>> caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
>>> and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
>>> index for write-protected pages.
>>>
>>> Remove a duplication definition of _PAGE_PAT_LARGE.
>>>
>>> Signed-off-by: Arvind Sankar <[email protected]>
>>
>> Fixes: tag?
>
> It's been broken since it was added in
>
> 6ebcb060713f ("x86/mm: Add support to encrypt the kernel in-place")
>
> but the code has been restructured since then. I think it should be
> backportable to 4.19.x if you want, except for that "duplication
> definition"[sic] I removed, which was only added in v5.6. Do I need to
> split that out into a separate patch?
For backporting it would probably be best if the patches were split. For
the PMD flags, I think you can target the original release and the stable
maintainers are pretty good about finding the right file. If not, they'll
ping you.
Thanks,
Tom
>
>>
>> Tested-by: Tom Lendacky <[email protected]>
>>
>>> ---
>>> arch/x86/include/asm/pgtable_types.h | 3 +--
>>> arch/x86/mm/mem_encrypt_identity.c | 4 ++--
>>> 2 files changed, 3 insertions(+), 4 deletions(-)
>>>
The PAT bit is in different locations for 4k and 2M/1G page table
entries.
Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
index for write-protected pages.
Signed-off-by: Arvind Sankar <[email protected]>
Fixes: 6ebcb060713f ("x86/mm: Add support to encrypt the kernel in-place")
Tested-by: Tom Lendacky <[email protected]>
Cc: [email protected]
---
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/mm/mem_encrypt_identity.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 816b31c68550..394757ee030a 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -155,6 +155,7 @@ enum page_cache_mode {
#define _PAGE_ENC (_AT(pteval_t, sme_me_mask))
#define _PAGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)
+#define _PAGE_LARGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT_LARGE)
#define _PAGE_NOCACHE (cachemode2protval(_PAGE_CACHE_MODE_UC))
#define _PAGE_CACHE_WP (cachemode2protval(_PAGE_CACHE_MODE_WP))
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 733b983f3a89..6c5eb6f3f14f 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -45,8 +45,8 @@
#define PMD_FLAGS_LARGE (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
#define PMD_FLAGS_DEC PMD_FLAGS_LARGE
-#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
- (_PAGE_PAT | _PAGE_PWT))
+#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_LARGE_CACHE_MASK) | \
+ (_PAGE_PAT_LARGE | _PAGE_PWT))
#define PMD_FLAGS_ENC (PMD_FLAGS_LARGE | _PAGE_ENC)
--
2.26.2
_PAGE_PAT_LARGE is already defined next to _PAGE_PAT. Remove the
duplicate.
Signed-off-by: Arvind Sankar <[email protected]>
Fixes: 4efb56649132 ("x86/mm: Tabulate the page table encoding definitions")
---
arch/x86/include/asm/pgtable_types.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 394757ee030a..f24d7ef8fffa 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -177,8 +177,6 @@ enum page_cache_mode {
#define __pgprot(x) ((pgprot_t) { (x) } )
#define __pg(x) __pgprot(x)
-#define _PAGE_PAT_LARGE (_AT(pteval_t, 1) << _PAGE_BIT_PAT_LARGE)
-
#define PAGE_NONE __pg( 0| 0| 0|___A| 0| 0| 0|___G)
#define PAGE_SHARED __pg(__PP|__RW|_USR|___A|__NX| 0| 0| 0)
#define PAGE_SHARED_EXEC __pg(__PP|__RW|_USR|___A| 0| 0| 0| 0)
--
2.26.2
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 29ac40cbed2bc06fa218ca25d7f5e280d3d08a25
Gitweb: https://git.kernel.org/tip/29ac40cbed2bc06fa218ca25d7f5e280d3d08a25
Author: Arvind Sankar <[email protected]>
AuthorDate: Wed, 11 Nov 2020 11:09:45 -05:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 10 Dec 2020 12:28:06 +01:00
x86/mm/mem_encrypt: Fix definition of PMD_FLAGS_DEC_WP
The PAT bit is in different locations for 4k and 2M/1G page table
entries.
Add a definition for _PAGE_LARGE_CACHE_MASK to represent the three
caching bits (PWT, PCD, PAT), similar to _PAGE_CACHE_MASK for 4k pages,
and use it in the definition of PMD_FLAGS_DEC_WP to get the correct PAT
index for write-protected pages.
Fixes: 6ebcb060713f ("x86/mm: Add support to encrypt the kernel in-place")
Signed-off-by: Arvind Sankar <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Tested-by: Tom Lendacky <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/mm/mem_encrypt_identity.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 816b31c..394757e 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -155,6 +155,7 @@ enum page_cache_mode {
#define _PAGE_ENC (_AT(pteval_t, sme_me_mask))
#define _PAGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)
+#define _PAGE_LARGE_CACHE_MASK (_PAGE_PWT | _PAGE_PCD | _PAGE_PAT_LARGE)
#define _PAGE_NOCACHE (cachemode2protval(_PAGE_CACHE_MODE_UC))
#define _PAGE_CACHE_WP (cachemode2protval(_PAGE_CACHE_MODE_WP))
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 733b983..6c5eb6f 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -45,8 +45,8 @@
#define PMD_FLAGS_LARGE (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
#define PMD_FLAGS_DEC PMD_FLAGS_LARGE
-#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
- (_PAGE_PAT | _PAGE_PWT))
+#define PMD_FLAGS_DEC_WP ((PMD_FLAGS_DEC & ~_PAGE_LARGE_CACHE_MASK) | \
+ (_PAGE_PAT_LARGE | _PAGE_PWT))
#define PMD_FLAGS_ENC (PMD_FLAGS_LARGE | _PAGE_ENC)
On Wed, Nov 11, 2020 at 11:09:46AM -0500, Arvind Sankar wrote:
> _PAGE_PAT_LARGE is already defined next to _PAGE_PAT. Remove the
> duplicate.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> Fixes: 4efb56649132 ("x86/mm: Tabulate the page table encoding definitions")
> ---
> arch/x86/include/asm/pgtable_types.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index 394757ee030a..f24d7ef8fffa 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -177,8 +177,6 @@ enum page_cache_mode {
> #define __pgprot(x) ((pgprot_t) { (x) } )
> #define __pg(x) __pgprot(x)
>
> -#define _PAGE_PAT_LARGE (_AT(pteval_t, 1) << _PAGE_BIT_PAT_LARGE)
> -
> #define PAGE_NONE __pg( 0| 0| 0|___A| 0| 0| 0|___G)
> #define PAGE_SHARED __pg(__PP|__RW|_USR|___A|__NX| 0| 0| 0)
> #define PAGE_SHARED_EXEC __pg(__PP|__RW|_USR|___A| 0| 0| 0| 0)
> --
> 2.26.2
>
Ping
On 1/5/21 1:28 PM, Arvind Sankar wrote:
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index 394757ee030a..f24d7ef8fffa 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -177,8 +177,6 @@ enum page_cache_mode {
> #define __pgprot(x) ((pgprot_t) { (x) } )
> #define __pg(x) __pgprot(x)
>
> -#define _PAGE_PAT_LARGE (_AT(pteval_t, 1) << _PAGE_BIT_PAT_LARGE)
Huh, I didn't realize that duplicate #defines were allowed. Guess you
learn something new every day. This looks fine to me, it removes the
exact dup that Ingo appears to have added.
Acked-by: Dave Hansen <[email protected]>
The following commit has been merged into the x86/cleanups branch of tip:
Commit-ID: 4af0e6e39b7ed77796a41537db91d717fedd0ac3
Gitweb: https://git.kernel.org/tip/4af0e6e39b7ed77796a41537db91d717fedd0ac3
Author: Arvind Sankar <[email protected]>
AuthorDate: Wed, 11 Nov 2020 11:09:46 -05:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 08 Jan 2021 22:04:51 +01:00
x86/mm: Remove duplicate definition of _PAGE_PAT_LARGE
_PAGE_PAT_LARGE is already defined next to _PAGE_PAT. Remove the
duplicate.
Fixes: 4efb56649132 ("x86/mm: Tabulate the page table encoding definitions")
Signed-off-by: Arvind Sankar <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Dave Hansen <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/pgtable_types.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 394757e..f24d7ef 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -177,8 +177,6 @@ enum page_cache_mode {
#define __pgprot(x) ((pgprot_t) { (x) } )
#define __pg(x) __pgprot(x)
-#define _PAGE_PAT_LARGE (_AT(pteval_t, 1) << _PAGE_BIT_PAT_LARGE)
-
#define PAGE_NONE __pg( 0| 0| 0|___A| 0| 0| 0|___G)
#define PAGE_SHARED __pg(__PP|__RW|_USR|___A|__NX| 0| 0| 0)
#define PAGE_SHARED_EXEC __pg(__PP|__RW|_USR|___A| 0| 0| 0| 0)