| Message ID | 20220128213150.1333552-2-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DAX poison recovery | expand |
> +static inline int set_mce_nospec(unsigned long pfn) > { > unsigned long decoy_addr; > int rc; > @@ -117,10 +113,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) > */ > decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63)); > > - if (unmap) > - rc = set_memory_np(decoy_addr, 1); > - else > - rc = set_memory_uc(decoy_addr, 1); > + rc = set_memory_np(decoy_addr, 1); > if (rc) > pr_warn("Could not invalidate pfn=0x%lx from 1:1 map\n", pfn); > return rc; > @@ -130,7 +123,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) > /* Restore full speculative operation to the pfn. */ > static inline int clear_mce_nospec(unsigned long pfn) > { > - return set_memory_wb((unsigned long) pfn_to_kaddr(pfn), 1); > + return _set_memory_present((unsigned long) pfn_to_kaddr(pfn), 1); > } Wouldn't it make more sense to move these helpers out of line rather than exporting _set_memory_present? > /* > - * _set_memory_prot is an internal helper for callers that have been passed > + * __set_memory_prot is an internal helper for callers that have been passed This looks unrelated to the patch.
On 2/2/2022 5:21 AM, Christoph Hellwig wrote: >> +static inline int set_mce_nospec(unsigned long pfn) >> { >> unsigned long decoy_addr; >> int rc; >> @@ -117,10 +113,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) >> */ >> decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63)); >> >> - if (unmap) >> - rc = set_memory_np(decoy_addr, 1); >> - else >> - rc = set_memory_uc(decoy_addr, 1); >> + rc = set_memory_np(decoy_addr, 1); >> if (rc) >> pr_warn("Could not invalidate pfn=0x%lx from 1:1 map\n", pfn); >> return rc; >> @@ -130,7 +123,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) >> /* Restore full speculative operation to the pfn. */ >> static inline int clear_mce_nospec(unsigned long pfn) >> { >> - return set_memory_wb((unsigned long) pfn_to_kaddr(pfn), 1); >> + return _set_memory_present((unsigned long) pfn_to_kaddr(pfn), 1); >> } > > Wouldn't it make more sense to move these helpers out of line rather > than exporting _set_memory_present? Do you mean to move return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); into clear_mce_nospec() for the x86 arch and get rid of _set_memory_present? If so, sure I'll do that. > >> /* >> - * _set_memory_prot is an internal helper for callers that have been passed >> + * __set_memory_prot is an internal helper for callers that have been passed > > This looks unrelated to the patch. My bad, will remove the remnant. thanks! -jane
On 2/2/2022 1:20 PM, Jane Chu wrote: > On 2/2/2022 5:21 AM, Christoph Hellwig wrote: >>> +static inline int set_mce_nospec(unsigned long pfn) >>> { >>> unsigned long decoy_addr; >>> int rc; >>> @@ -117,10 +113,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) >>> */ >>> decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63)); >>> >>> - if (unmap) >>> - rc = set_memory_np(decoy_addr, 1); >>> - else >>> - rc = set_memory_uc(decoy_addr, 1); >>> + rc = set_memory_np(decoy_addr, 1); >>> if (rc) >>> pr_warn("Could not invalidate pfn=0x%lx from 1:1 map\n", pfn); >>> return rc; >>> @@ -130,7 +123,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) >>> /* Restore full speculative operation to the pfn. */ >>> static inline int clear_mce_nospec(unsigned long pfn) >>> { >>> - return set_memory_wb((unsigned long) pfn_to_kaddr(pfn), 1); >>> + return _set_memory_present((unsigned long) pfn_to_kaddr(pfn), 1); >>> } >> >> Wouldn't it make more sense to move these helpers out of line rather >> than exporting _set_memory_present? > > Do you mean to move > return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); > into clear_mce_nospec() for the x86 arch and get rid of _set_memory_present? > If so, sure I'll do that. Looks like I can't do that. It's either exporting _set_memory_present(), or exporting change_page_attr_set(). Perhaps the former is more conventional? -jane
On Wed, Feb 02, 2022 at 11:07:37PM +0000, Jane Chu wrote: > On 2/2/2022 1:20 PM, Jane Chu wrote: > >> Wouldn't it make more sense to move these helpers out of line rather > >> than exporting _set_memory_present? > > > > Do you mean to move > > return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); > > into clear_mce_nospec() for the x86 arch and get rid of _set_memory_present? > > If so, sure I'll do that. > > Looks like I can't do that. It's either exporting > _set_memory_present(), or exporting change_page_attr_set(). Perhaps the > former is more conventional? These helpers above means set_mce_nospec and clear_mce_nospec. If they are moved to normal functions instead of inlines, there is no need to export the internals at all.
On Thu, Feb 3, 2022 at 5:42 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Feb 02, 2022 at 11:07:37PM +0000, Jane Chu wrote: > > On 2/2/2022 1:20 PM, Jane Chu wrote: > > >> Wouldn't it make more sense to move these helpers out of line rather > > >> than exporting _set_memory_present? > > > > > > Do you mean to move > > > return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); > > > into clear_mce_nospec() for the x86 arch and get rid of _set_memory_present? > > > If so, sure I'll do that. > > > > Looks like I can't do that. It's either exporting > > _set_memory_present(), or exporting change_page_attr_set(). Perhaps the > > former is more conventional? > > These helpers above means set_mce_nospec and clear_mce_nospec. If they > are moved to normal functions instead of inlines, there is no need to > export the internals at all. Agree, {set,clear}_mce_nospec() can just move to arch/x86/mm/pat/set_memory.c.
On 2/3/2022 9:23 PM, Dan Williams wrote: > On Thu, Feb 3, 2022 at 5:42 AM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Wed, Feb 02, 2022 at 11:07:37PM +0000, Jane Chu wrote: >>> On 2/2/2022 1:20 PM, Jane Chu wrote: >>>>> Wouldn't it make more sense to move these helpers out of line rather >>>>> than exporting _set_memory_present? >>>> >>>> Do you mean to move >>>> return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); >>>> into clear_mce_nospec() for the x86 arch and get rid of _set_memory_present? >>>> If so, sure I'll do that. >>> >>> Looks like I can't do that. It's either exporting >>> _set_memory_present(), or exporting change_page_attr_set(). Perhaps the >>> former is more conventional? >> >> These helpers above means set_mce_nospec and clear_mce_nospec. If they >> are moved to normal functions instead of inlines, there is no need to >> export the internals at all. > > Agree, {set,clear}_mce_nospec() can just move to arch/x86/mm/pat/set_memory.c. Got it, will do. thanks! -jane
diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h index ff0f2d90338a..aef6677da291 100644 --- a/arch/x86/include/asm/set_memory.h +++ b/arch/x86/include/asm/set_memory.h @@ -50,6 +50,7 @@ int set_memory_decrypted(unsigned long addr, int numpages); int set_memory_np_noalias(unsigned long addr, int numpages); int set_memory_nonglobal(unsigned long addr, int numpages); int set_memory_global(unsigned long addr, int numpages); +int _set_memory_present(unsigned long addr, int numpages); int set_pages_array_uc(struct page **pages, int addrinarray); int set_pages_array_wc(struct page **pages, int addrinarray); @@ -89,13 +90,8 @@ void notify_range_enc_status_changed(unsigned long vaddr, int npages, bool enc); extern int kernel_set_to_readonly; #ifdef CONFIG_X86_64 -/* - * Prevent speculative access to the page by either unmapping - * it (if we do not require access to any part of the page) or - * marking it uncacheable (if we want to try to retrieve data - * from non-poisoned lines in the page). - */ -static inline int set_mce_nospec(unsigned long pfn, bool unmap) +/* Prevent speculative access to a page by marking it not-present */ +static inline int set_mce_nospec(unsigned long pfn) { unsigned long decoy_addr; int rc; @@ -117,10 +113,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) */ decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63)); - if (unmap) - rc = set_memory_np(decoy_addr, 1); - else - rc = set_memory_uc(decoy_addr, 1); + rc = set_memory_np(decoy_addr, 1); if (rc) pr_warn("Could not invalidate pfn=0x%lx from 1:1 map\n", pfn); return rc; @@ -130,7 +123,7 @@ static inline int set_mce_nospec(unsigned long pfn, bool unmap) /* Restore full speculative operation to the pfn. */ static inline int clear_mce_nospec(unsigned long pfn) { - return set_memory_wb((unsigned long) pfn_to_kaddr(pfn), 1); + return _set_memory_present((unsigned long) pfn_to_kaddr(pfn), 1); } #define clear_mce_nospec clear_mce_nospec #else diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 5818b837fd4d..8d12739f283d 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -613,7 +613,7 @@ static int uc_decode_notifier(struct notifier_block *nb, unsigned long val, pfn = mce->addr >> PAGE_SHIFT; if (!memory_failure(pfn, 0)) { - set_mce_nospec(pfn, whole_page(mce)); + set_mce_nospec(pfn); mce->kflags |= MCE_HANDLED_UC; } @@ -1297,7 +1297,7 @@ static void kill_me_maybe(struct callback_head *cb) ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags); if (!ret) { - set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page); + set_mce_nospec(p->mce_addr >> PAGE_SHIFT); sync_core(); return; } @@ -1321,7 +1321,7 @@ static void kill_me_never(struct callback_head *cb) p->mce_count = 0; pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr); if (!memory_failure(p->mce_addr >> PAGE_SHIFT, 0)) - set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page); + set_mce_nospec(p->mce_addr >> PAGE_SHIFT); } static void queue_task_work(struct mce *m, char *msg, void (*func)(struct callback_head *)) diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index b4072115c8ef..68d84c8bd977 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1816,7 +1816,7 @@ static inline int cpa_clear_pages_array(struct page **pages, int numpages, } /* - * _set_memory_prot is an internal helper for callers that have been passed + * __set_memory_prot is an internal helper for callers that have been passed * a pgprot_t value from upper layers and a reservation has already been taken. * If you want to set the pgprot to a specific page protocol, use the * set_memory_xx() functions. @@ -1983,6 +1983,12 @@ int set_memory_global(unsigned long addr, int numpages) __pgprot(_PAGE_GLOBAL), 0); } +int _set_memory_present(unsigned long addr, int numpages) +{ + return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT), 0); +} +EXPORT_SYMBOL_GPL(_set_memory_present); + /* * __set_memory_enc_pgtable() is used for the hypervisors that get * informed about "encryption" status via page tables. diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h index f36be5166c19..9ad898d40e7e 100644 --- a/include/linux/set_memory.h +++ b/include/linux/set_memory.h @@ -43,7 +43,7 @@ static inline bool can_set_direct_map(void) #endif /* CONFIG_ARCH_HAS_SET_DIRECT_MAP */ #ifndef set_mce_nospec -static inline int set_mce_nospec(unsigned long pfn, bool unmap) +static inline int set_mce_nospec(unsigned long pfn) { return 0; }
Mark poisoned page as not present, and to reverse the 'np' effect, restate the _PAGE_PRESENT bit. Please refer to discussions here for reason behind the decision. https://lore.kernel.org/all/CAPcyv4hrXPb1tASBZUg-GgdVs0OOFKXMXLiHmktg_kFi7YBMyQ@mail.gmail.com/ Fixes: 284ce4011ba6 ("x86/memory_failure: Introduce {set, clear}_mce_nospec()") Signed-off-by: Jane Chu <jane.chu@oracle.com> --- arch/x86/include/asm/set_memory.h | 17 +++++------------ arch/x86/kernel/cpu/mce/core.c | 6 +++--- arch/x86/mm/pat/set_memory.c | 8 +++++++- include/linux/set_memory.h | 2 +- 4 files changed, 16 insertions(+), 17 deletions(-)