| Message ID | 20180717112029.42378-11-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > Store KeyID in bits 31:16 of extended page flags. These bits are unused. I'd love a two sentence remind of what page_ext is and why you chose to use it. Yes, you need this. No, not everybody that you want to review this patch set knows what it is or why you chose it. > page_keyid() returns zero until page_ext is ready. Is there any implication of this? Or does it not matter because we don't run userspace until after page_ext initialization is done? > page_ext initializer enables static branch to indicate that "enables a static branch" > page_keyid() can use page_ext. The same static branch will gate MKTME > readiness in general. Can you elaborate on this a bit? It would also be a nice place to hint to the folks working hard on the APIs to ensure she checks this. > We don't yet set KeyID for the page. It will come in the following > patch that implements prep_encrypted_page(). All pages have KeyID-0 for > now. It also wouldn't hurt to mention why you don't use an X86_FEATURE_* for this rather than an explicit static branch. I'm sure the x86 maintainers will be curious.
On Wed, Jul 18, 2018 at 04:38:02PM -0700, Dave Hansen wrote: > On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > > Store KeyID in bits 31:16 of extended page flags. These bits are unused. > > I'd love a two sentence remind of what page_ext is and why you chose to > use it. Yes, you need this. No, not everybody that you want to review > this patch set knows what it is or why you chose it. Okay. > > page_keyid() returns zero until page_ext is ready. > > Is there any implication of this? Or does it not matter because we > don't run userspace until after page_ext initialization is done? It matters in sense that we shouldn't reference page_ext before it's initialized otherwise we will get garbage and crash. > > page_ext initializer enables static branch to indicate that > > "enables a static branch" > > > page_keyid() can use page_ext. The same static branch will gate MKTME > > readiness in general. > > Can you elaborate on this a bit? It would also be a nice place to hint > to the folks working hard on the APIs to ensure she checks this. Okay. > > We don't yet set KeyID for the page. It will come in the following > > patch that implements prep_encrypted_page(). All pages have KeyID-0 for > > now. > > It also wouldn't hurt to mention why you don't use an X86_FEATURE_* for > this rather than an explicit static branch. I'm sure the x86 > maintainers will be curious. Sure.
On Mon, Jul 23, 2018 at 12:45:17PM +0300, Kirill A. Shutemov wrote: > On Wed, Jul 18, 2018 at 04:38:02PM -0700, Dave Hansen wrote: > > On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > > > Store KeyID in bits 31:16 of extended page flags. These bits are unused. > > > > I'd love a two sentence remind of what page_ext is and why you chose to > > use it. Yes, you need this. No, not everybody that you want to review > > this patch set knows what it is or why you chose it. > > Okay. > > > > page_keyid() returns zero until page_ext is ready. > > > > Is there any implication of this? Or does it not matter because we > > don't run userspace until after page_ext initialization is done? > > It matters in sense that we shouldn't reference page_ext before it's > initialized otherwise we will get garbage and crash. > > > > page_ext initializer enables static branch to indicate that > > > > "enables a static branch" > > > > > page_keyid() can use page_ext. The same static branch will gate MKTME > > > readiness in general. > > > > Can you elaborate on this a bit? It would also be a nice place to hint > > to the folks working hard on the APIs to ensure she checks this. > > Okay. At API init time we can check if (MKTME_ENABLED && mktme_nr_keyids > 0) Sounds like this is another dependency we need to check and 'wait' on? It happens after MKTME_ENABLED is set? Let me know. > > > > We don't yet set KeyID for the page. It will come in the following > > > patch that implements prep_encrypted_page(). All pages have KeyID-0 for > > > now. > > > > It also wouldn't hurt to mention why you don't use an X86_FEATURE_* for > > this rather than an explicit static branch. I'm sure the x86 > > maintainers will be curious. > > Sure. > > -- > Kirill A. Shutemov
diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h index df31876ec48c..7266494b4f0a 100644 --- a/arch/x86/include/asm/mktme.h +++ b/arch/x86/include/asm/mktme.h @@ -2,11 +2,18 @@ #define _ASM_X86_MKTME_H #include <linux/types.h> +#include <linux/page_ext.h> #ifdef CONFIG_X86_INTEL_MKTME extern phys_addr_t mktme_keyid_mask; extern int mktme_nr_keyids; extern int mktme_keyid_shift; + +extern struct page_ext_operations page_mktme_ops; + +#define page_keyid page_keyid +int page_keyid(const struct page *page); + #else #define mktme_keyid_mask ((phys_addr_t)0) #define mktme_nr_keyids 0 diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h index 7555b48803a8..39af59487d5f 100644 --- a/arch/x86/include/asm/page.h +++ b/arch/x86/include/asm/page.h @@ -19,6 +19,7 @@ struct page; #include <linux/range.h> +#include <asm/mktme.h> extern struct range pfn_mapped[]; extern int nr_pfn_mapped; diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c index 467f1b26c737..09cbff678b9f 100644 --- a/arch/x86/mm/mktme.c +++ b/arch/x86/mm/mktme.c @@ -3,3 +3,37 @@ phys_addr_t mktme_keyid_mask; int mktme_nr_keyids; int mktme_keyid_shift; + +static DEFINE_STATIC_KEY_FALSE(mktme_enabled_key); + +static inline bool mktme_enabled(void) +{ + return static_branch_unlikely(&mktme_enabled_key); +} + +int page_keyid(const struct page *page) +{ + if (!mktme_enabled()) + return 0; + + return lookup_page_ext(page)->keyid; +} +EXPORT_SYMBOL(page_keyid); + +static bool need_page_mktme(void) +{ + /* Make sure keyid doesn't collide with extended page flags */ + BUILD_BUG_ON(__NR_PAGE_EXT_FLAGS > 16); + + return !!mktme_nr_keyids; +} + +static void init_page_mktme(void) +{ + static_branch_enable(&mktme_enabled_key); +} + +struct page_ext_operations page_mktme_ops = { + .need = need_page_mktme, + .init = init_page_mktme, +}; diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h index f84f167ec04c..d9c5aae9523f 100644 --- a/include/linux/page_ext.h +++ b/include/linux/page_ext.h @@ -23,6 +23,7 @@ enum page_ext_flags { PAGE_EXT_YOUNG, PAGE_EXT_IDLE, #endif + __NR_PAGE_EXT_FLAGS }; /* @@ -33,7 +34,15 @@ enum page_ext_flags { * then the page_ext for pfn always exists. */ struct page_ext { - unsigned long flags; + union { + unsigned long flags; +#ifdef CONFIG_X86_INTEL_MKTME + struct { + unsigned short __pad; + unsigned short keyid; + }; +#endif + }; }; extern void pgdat_page_ext_init(struct pglist_data *pgdat); diff --git a/mm/page_ext.c b/mm/page_ext.c index a9826da84ccb..036658229842 100644 --- a/mm/page_ext.c +++ b/mm/page_ext.c @@ -68,6 +68,9 @@ static struct page_ext_operations *page_ext_ops[] = { #if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) &page_idle_ops, #endif +#ifdef CONFIG_X86_INTEL_MKTME + &page_mktme_ops, +#endif }; static unsigned long total_usage;
Store KeyID in bits 31:16 of extended page flags. These bits are unused. page_keyid() returns zero until page_ext is ready. page_ext initializer enables static branch to indicate that page_keyid() can use page_ext. The same static branch will gate MKTME readiness in general. We don't yet set KeyID for the page. It will come in the following patch that implements prep_encrypted_page(). All pages have KeyID-0 for now. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- arch/x86/include/asm/mktme.h | 7 +++++++ arch/x86/include/asm/page.h | 1 + arch/x86/mm/mktme.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/page_ext.h | 11 ++++++++++- mm/page_ext.c | 3 +++ 5 files changed, 55 insertions(+), 1 deletion(-)