[PATCH -next v4 0/3] minor improvements for x86 mce processing

[PATCH -next v4 0/3] minor improvements for x86 mce processing

[Tong Tiangen]

In this patchset, we remove the unused macro EX_TYPE_COPY and centralize
the processing of memory-failure to do_machine_check() to avoid calling
memory_failure_queue() separately for different MC-Safe scenarios. In
addition, MCE_IN_KERNEL_COPYIN is renamed MCE_IN_KERNEL_COPY_MC to expand
its usage scope.

[1]https://lore.kernel.org/linux-mm/20230526063242.133656-1-wangkefeng.wang@huawei.com/

since v3:
  1. Rebased on linux-next tag next-20240111.
  2. Delete duplicate commit references on patch 3.

since v2:
  1. remove redundant fixup type EX_TYPE_COPY.
  2. rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC.
  3. update patch3's commit message and the processing logic of
     EX_TYPE_DEFAULT_MCE_SAFE based on the discussion of [1].

Kefeng Wang (1):
  x86/mce: set MCE_IN_KERNEL_COPY_MC for DEFAULT_MCE_SAFE exception

Tong Tiangen (2):
  x86/mce: remove redundant fixup type EX_TYPE_COPY
  x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

 arch/x86/include/asm/asm.h                 |  3 ---
 arch/x86/include/asm/extable_fixup_types.h | 23 +++++++++++-----------
 arch/x86/include/asm/mce.h                 |  8 ++++----
 arch/x86/kernel/cpu/mce/core.c             |  2 +-
 arch/x86/kernel/cpu/mce/severity.c         |  7 +++----
 arch/x86/mm/extable.c                      |  9 ---------
 mm/ksm.c                                   |  1 -
 mm/memory.c                                | 13 ++++--------
 8 files changed, 23 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH -next v4 1/3] x86/mce: remove redundant fixup type EX_TYPE_COPY

[Tong Tiangen]

In commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache' function"),
rewrited __copy_user_nocache() uses EX_TYPE_UACCESS instead of
EX_TYPE_COPY，this change does not broken the MC safe copy for
__copy_user_nocache(), but as a result, there's no place for EX_TYPE_COPY
to use. Therefore, we remove the definition of EX_TYPE_COPY.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/include/asm/asm.h                 |  3 ---
 arch/x86/include/asm/extable_fixup_types.h | 23 +++++++++++-----------
 arch/x86/kernel/cpu/mce/severity.c         |  1 -
 arch/x86/mm/extable.c                      |  9 ---------
 4 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index fbcfec4dc4cc..692409ea0c37 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -215,9 +215,6 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
 #define _ASM_EXTABLE_UA(from, to)				\
 	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_UACCESS)
 
-#define _ASM_EXTABLE_CPY(from, to)				\
-	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_COPY)
-
 #define _ASM_EXTABLE_FAULT(from, to)				\
 	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_FAULT)
 
diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index fe6312045042..787916ec1e12 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -36,18 +36,17 @@
 #define	EX_TYPE_DEFAULT			 1
 #define	EX_TYPE_FAULT			 2
 #define	EX_TYPE_UACCESS			 3
-#define	EX_TYPE_COPY			 4
-#define	EX_TYPE_CLEAR_FS		 5
-#define	EX_TYPE_FPU_RESTORE		 6
-#define	EX_TYPE_BPF			 7
-#define	EX_TYPE_WRMSR			 8
-#define	EX_TYPE_RDMSR			 9
-#define	EX_TYPE_WRMSR_SAFE		10 /* reg := -EIO */
-#define	EX_TYPE_RDMSR_SAFE		11 /* reg := -EIO */
-#define	EX_TYPE_WRMSR_IN_MCE		12
-#define	EX_TYPE_RDMSR_IN_MCE		13
-#define	EX_TYPE_DEFAULT_MCE_SAFE	14
-#define	EX_TYPE_FAULT_MCE_SAFE		15
+#define	EX_TYPE_CLEAR_FS		 4
+#define	EX_TYPE_FPU_RESTORE		 5
+#define	EX_TYPE_BPF			 6
+#define	EX_TYPE_WRMSR			 7
+#define	EX_TYPE_RDMSR			 8
+#define	EX_TYPE_WRMSR_SAFE		 9 /* reg := -EIO */
+#define	EX_TYPE_RDMSR_SAFE		10 /* reg := -EIO */
+#define	EX_TYPE_WRMSR_IN_MCE		11
+#define	EX_TYPE_RDMSR_IN_MCE		12
+#define	EX_TYPE_DEFAULT_MCE_SAFE	13
+#define	EX_TYPE_FAULT_MCE_SAFE		14
 
 #define	EX_TYPE_POP_REG			16 /* sp += sizeof(long) */
 #define EX_TYPE_POP_ZERO		(EX_TYPE_POP_REG | EX_DATA_IMM(0))
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index c4477162c07d..bca780fa5e57 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -290,7 +290,6 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 
 	switch (fixup_type) {
 	case EX_TYPE_UACCESS:
-	case EX_TYPE_COPY:
 		if (!copy_user)
 			return IN_KERNEL;
 		m->kflags |= MCE_IN_KERNEL_COPYIN;
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 271dcb2deabc..2354c0156e51 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -163,13 +163,6 @@ static bool ex_handler_uaccess(const struct exception_table_entry *fixup,
 	return ex_handler_default(fixup, regs);
 }
 
-static bool ex_handler_copy(const struct exception_table_entry *fixup,
-			    struct pt_regs *regs, int trapnr)
-{
-	WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?");
-	return ex_handler_fault(fixup, regs, trapnr);
-}
-
 static bool ex_handler_msr(const struct exception_table_entry *fixup,
 			   struct pt_regs *regs, bool wrmsr, bool safe, int reg)
 {
@@ -267,8 +260,6 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 		return ex_handler_fault(e, regs, trapnr);
 	case EX_TYPE_UACCESS:
 		return ex_handler_uaccess(e, regs, trapnr, fault_addr);
-	case EX_TYPE_COPY:
-		return ex_handler_copy(e, regs, trapnr);
 	case EX_TYPE_CLEAR_FS:
 		return ex_handler_clear_fs(e, regs);
 	case EX_TYPE_FPU_RESTORE:
-- 
2.25.1

[PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

In the x86 mce processing, macro MCE_IN_KERNEL_COPYIN is used to identify
copied from user. do_machine_check() uses this flag to isolate posion
page in memory_failure(). there's nothing wrong but we can expand the use
of this macro.

Currently, there are some kernel memory copy scenarios is also mc safe
which use copy_mc_to_kernel() or copy_mc_user_highpage(). In these
scenarios, posion pages need to be isolated too. Therefore, a macro similar
to MCE_IN_KERNEL_COPYIN is required. For this reason, we can rename
MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC, the new macro can be applied
to both user-to-kernel mc safe copy and kernel-to-kernel mc safe copy.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/include/asm/mce.h         | 8 ++++----
 arch/x86/kernel/cpu/mce/core.c     | 2 +-
 arch/x86/kernel/cpu/mce/severity.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index de3118305838..cb628ab2f32f 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -151,11 +151,11 @@
 
 /*
  * Indicates an MCE that happened in kernel space while copying data
- * from user. In this case fixup_exception() gets the kernel to the
- * error exit for the copy function. Machine check handler can then
- * treat it like a fault taken in user mode.
+ * from user or kernel. In this case fixup_exception() gets the kernel
+ * to the error exit for the copy function. Machine check handler can
+ * then treat it like a fault taken in user or kernel mode.
  */
-#define MCE_IN_KERNEL_COPYIN	BIT_ULL(7)
+#define MCE_IN_KERNEL_COPY_MC	BIT_ULL(7)
 
 /*
  * This structure contains all data related to the MCE log.  Also
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2bfd0e3c62e4..bd1a31ed148b 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1607,7 +1607,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 				mce_panic("Failed kernel mode recovery", &m, msg);
 		}
 
-		if (m.kflags & MCE_IN_KERNEL_COPYIN)
+		if (m.kflags & MCE_IN_KERNEL_COPY_MC)
 			queue_task_work(&m, msg, kill_me_never);
 	}
 
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index bca780fa5e57..df67a7a13034 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -292,7 +292,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	case EX_TYPE_UACCESS:
 		if (!copy_user)
 			return IN_KERNEL;
-		m->kflags |= MCE_IN_KERNEL_COPYIN;
+		m->kflags |= MCE_IN_KERNEL_COPY_MC;
 		fallthrough;
 
 	case EX_TYPE_FAULT_MCE_SAFE:
-- 
2.25.1

[PATCH -next v4 3/3] x86/mce: set MCE_IN_KERNEL_COPY_MC for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

From: Kefeng Wang <wangkefeng.wang@huawei.com>

If an MCE has happened in kernel space and the kernel can recover,
mce.kflags MCE_IN_KERNEL_RECOV will set in error_context().

With the setting of MCE_IN_KERNEL_RECOV, the MCE is handled in
do_machine_check(). But due to lack of MCE_IN_KERNEL_COPY_MC, although the
kernel won't panic, the corrupted page don't be isolated, new one maybe
consume it again, which is not what we expected.

In order to avoid above issue, some hwpoison recover process[1][2][3],
memory_failure_queue() is called to cope with such unhandled corrupted
pages, also there are some other already existed MC-safe copy scenarios,
eg, nvdimm, dm-writecache, dax, which don't isolate corrupted pages.

The best way to fix them is set MCE_IN_KERNEL_COPY_MC for MC-Safe Copy,
then let the core do_machine_check() to isolate corrupted page instead
of doing it one-by-one.

EX_TYPE_FAULT_MCE_SAFE is used for the FPU. Here, we do not touch the logic
of FPU. We only modify the logic of EX_TYPE_DEFAULT_MCE_SAFE which is used
in the scenarios described above.

[1] commit d302c2398ba2 ("mm, hwpoison: when copy-on-write hits poison, take page offline")
[2] commit 1cb9dc4b475c ("mm: hwpoison: support recovery from HugePage copy-on-write faults")
[3] commit 6b970599e807 ("mm: hwpoison: support recovery from ksm_might_need_to_copy()")

Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/kernel/cpu/mce/severity.c |  4 ++--
 mm/ksm.c                           |  1 -
 mm/memory.c                        | 13 ++++---------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index df67a7a13034..b4b1d028cbb3 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	case EX_TYPE_UACCESS:
 		if (!copy_user)
 			return IN_KERNEL;
+		fallthrough;
+	case EX_TYPE_DEFAULT_MCE_SAFE:
 		m->kflags |= MCE_IN_KERNEL_COPY_MC;
 		fallthrough;
-
 	case EX_TYPE_FAULT_MCE_SAFE:
-	case EX_TYPE_DEFAULT_MCE_SAFE:
 		m->kflags |= MCE_IN_KERNEL_RECOV;
 		return IN_KERNEL_RECOV;
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 8c001819cf10..ba9d324ea1c6 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -3084,7 +3084,6 @@ struct folio *ksm_might_need_to_copy(struct folio *folio,
 		if (copy_mc_user_highpage(folio_page(new_folio, 0), page,
 								addr, vma)) {
 			folio_put(new_folio);
-			memory_failure_queue(folio_pfn(folio), 0);
 			return ERR_PTR(-EHWPOISON);
 		}
 		folio_set_dirty(new_folio);
diff --git a/mm/memory.c b/mm/memory.c
index c66af4520958..33d8903ab2af 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2843,10 +2843,8 @@ static inline int __wp_page_copy_user(struct page *dst, struct page *src,
 	unsigned long addr = vmf->address;
 
 	if (likely(src)) {
-		if (copy_mc_user_highpage(dst, src, addr, vma)) {
-			memory_failure_queue(page_to_pfn(src), 0);
+		if (copy_mc_user_highpage(dst, src, addr, vma))
 			return -EHWPOISON;
-		}
 		return 0;
 	}
 
@@ -6176,10 +6174,8 @@ static int copy_user_gigantic_page(struct folio *dst, struct folio *src,
 
 		cond_resched();
 		if (copy_mc_user_highpage(dst_page, src_page,
-					  addr + i*PAGE_SIZE, vma)) {
-			memory_failure_queue(page_to_pfn(src_page), 0);
+					  addr + i*PAGE_SIZE, vma))
 			return -EHWPOISON;
-		}
 	}
 	return 0;
 }
@@ -6196,10 +6192,9 @@ static int copy_subpage(unsigned long addr, int idx, void *arg)
 	struct page *dst = nth_page(copy_arg->dst, idx);
 	struct page *src = nth_page(copy_arg->src, idx);
 
-	if (copy_mc_user_highpage(dst, src, addr, copy_arg->vma)) {
-		memory_failure_queue(page_to_pfn(src), 0);
+	if (copy_mc_user_highpage(dst, src, addr, copy_arg->vma))
 		return -EHWPOISON;
-	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH -next v4 0/3] minor improvements for x86 mce processing

[Kefeng Wang]

Hi Borislav and Tony，

On 2024/1/11 21:55, Tong Tiangen wrote:
> In this patchset, we remove the unused macro EX_TYPE_COPY and centralize
> the processing of memory-failure to do_machine_check() to avoid calling
> memory_failure_queue() separately for different MC-Safe scenarios. In
> addition, MCE_IN_KERNEL_COPYIN is renamed MCE_IN_KERNEL_COPY_MC to expand
> its usage scope.

The patchset is a followup[1], as Borislav suggested[2], we firstly
cleanup unused EX_TYPE_COPY, then rename MCE_IN_KERNEL_COPYIN to
reduce confusion, could you give us some comments about it,
many thanks.

> 
> [1]https://lore.kernel.org/linux-mm/20230526063242.133656-1-wangkefeng.wang@huawei.com/
> 
[2] 
https://lore.kernel.org/linux-edac/20230602160138.GDZHoSYsWoPAinMszk@fat_crate.local/
> since v3:
>    1. Rebased on linux-next tag next-20240111.
>    2. Delete duplicate commit references on patch 3.
> 
> since v2:
>    1. remove redundant fixup type EX_TYPE_COPY.
>    2. rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC.
>    3. update patch3's commit message and the processing logic of
>       EX_TYPE_DEFAULT_MCE_SAFE based on the discussion of [1].
> 
> Kefeng Wang (1):
>    x86/mce: set MCE_IN_KERNEL_COPY_MC for DEFAULT_MCE_SAFE exception
> 
> Tong Tiangen (2):
>    x86/mce: remove redundant fixup type EX_TYPE_COPY
>    x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC
> 
>   arch/x86/include/asm/asm.h                 |  3 ---
>   arch/x86/include/asm/extable_fixup_types.h | 23 +++++++++++-----------
>   arch/x86/include/asm/mce.h                 |  8 ++++----
>   arch/x86/kernel/cpu/mce/core.c             |  2 +-
>   arch/x86/kernel/cpu/mce/severity.c         |  7 +++----
>   arch/x86/mm/extable.c                      |  9 ---------
>   mm/ksm.c                                   |  1 -
>   mm/memory.c                                | 13 ++++--------
>   8 files changed, 23 insertions(+), 43 deletions(-)
> 

Re: [PATCH -next v4 0/3] minor improvements for x86 mce processing

[Borislav Petkov]

On Mon, Jan 15, 2024 at 09:25:57PM +0800, Kefeng Wang wrote:
> could you give us some comments about it, many thanks.

Since we have a (suspended¹) merge window currently:

From: Documentation/process/maintainer-tip.rst

Merge window
^^^^^^^^^^^^

Please do not expect large patch series to be handled during the merge
window or even during the week before.  Such patches should be submitted in
mergeable state *at* *least* a week before the merge window opens.
Exceptions are made for bug fixes and *sometimes* for small standalone
drivers for new hardware or minimally invasive patches for hardware
enablement.

During the merge window, the maintainers instead focus on following the
upstream changes, fixing merge window fallout, collecting bug fixes, and
allowing themselves a breath. Please respect that.

The release candidate -rc1 is the starting point for new patches to be
applied which are targeted for the next merge window.

¹ https://lore.kernel.org/r/CAHk-=wjMWpmXtKeiN__vnNO4TcttZR-8dVvd_oBq%2BhjeSsWUwg@mail.gmail.com

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 0/3] minor improvements for x86 mce processing

[Kefeng Wang]

On 2024/1/15 21:33, Borislav Petkov wrote:
> On Mon, Jan 15, 2024 at 09:25:57PM +0800, Kefeng Wang wrote:
>> could you give us some comments about it, many thanks.
> 
> Since we have a (suspended¹) merge window currently:
> 
> From: Documentation/process/maintainer-tip.rst
> 
> Merge window
> ^^^^^^^^^^^^
> 
> Please do not expect large patch series to be handled during the merge
> window or even during the week before.  Such patches should be submitted in
> mergeable state *at* *least* a week before the merge window opens.
> Exceptions are made for bug fixes and *sometimes* for small standalone
> drivers for new hardware or minimally invasive patches for hardware
> enablement.
> 
> During the merge window, the maintainers instead focus on following the
> upstream changes, fixing merge window fallout, collecting bug fixes, and
> allowing themselves a breath. Please respect that.
> 
> The release candidate -rc1 is the starting point for new patches to be
> applied which are targeted for the next merge window.

Oh, sure, we could resend after -rc1, thanks.
> 
> ¹ https://lore.kernel.org/r/CAHk-=wjMWpmXtKeiN__vnNO4TcttZR-8dVvd_oBq%2BhjeSsWUwg@mail.gmail.com
> 

Hope everything is OK!

Re: [PATCH -next v4 0/3] minor improvements for x86 mce processing

[Borislav Petkov]

On Tue, Jan 16, 2024 at 09:14:56AM +0800, Kefeng Wang wrote:
> Oh, sure, we could resend after -rc1, thanks.

No, no need to resend after -rc1.

What you should do, instead, is take the time to read the text I pasted
more carefully and get acquainted with the development process:

https://kernel.org/doc/html/latest/process/development-process.html

and especially this:

https://kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient

In those pages is a wealth of useful information.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 1/3] x86/mce: remove redundant fixup type EX_TYPE_COPY

[Borislav Petkov]

On Thu, Jan 11, 2024 at 09:55:46PM +0800, Tong Tiangen wrote:
> In commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache' function"),
> rewrited __copy_user_nocache() uses EX_TYPE_UACCESS instead of
> EX_TYPE_COPY，this change does not broken the MC safe copy for
> __copy_user_nocache(), but as a result, there's no place for EX_TYPE_COPY
> to use. Therefore, we remove the definition of EX_TYPE_COPY.

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Also, see section "Changelog" in
Documentation/process/maintainer-tip.rst

> diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
> index fe6312045042..787916ec1e12 100644
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -36,18 +36,17 @@
>  #define	EX_TYPE_DEFAULT			 1
>  #define	EX_TYPE_FAULT			 2
>  #define	EX_TYPE_UACCESS			 3
> -#define	EX_TYPE_COPY			 4
> -#define	EX_TYPE_CLEAR_FS		 5
> -#define	EX_TYPE_FPU_RESTORE		 6
> -#define	EX_TYPE_BPF			 7
> -#define	EX_TYPE_WRMSR			 8
> -#define	EX_TYPE_RDMSR			 9
> -#define	EX_TYPE_WRMSR_SAFE		10 /* reg := -EIO */
> -#define	EX_TYPE_RDMSR_SAFE		11 /* reg := -EIO */
> -#define	EX_TYPE_WRMSR_IN_MCE		12
> -#define	EX_TYPE_RDMSR_IN_MCE		13
> -#define	EX_TYPE_DEFAULT_MCE_SAFE	14
> -#define	EX_TYPE_FAULT_MCE_SAFE		15
> +#define	EX_TYPE_CLEAR_FS		 4
> +#define	EX_TYPE_FPU_RESTORE		 5
> +#define	EX_TYPE_BPF			 6
> +#define	EX_TYPE_WRMSR			 7
> +#define	EX_TYPE_RDMSR			 8
> +#define	EX_TYPE_WRMSR_SAFE		 9 /* reg := -EIO */
> +#define	EX_TYPE_RDMSR_SAFE		10 /* reg := -EIO */
> +#define	EX_TYPE_WRMSR_IN_MCE		11
> +#define	EX_TYPE_RDMSR_IN_MCE		12
> +#define	EX_TYPE_DEFAULT_MCE_SAFE	13
> +#define	EX_TYPE_FAULT_MCE_SAFE		14

You don't need to renumber them - all you need to do is to comment it
out:

/* unused, was: #define EX_TYPE_COPY 		4 */

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Thu, Jan 11, 2024 at 09:55:47PM +0800, Tong Tiangen wrote:
> Currently, there are some kernel memory copy scenarios is also mc safe
> which use copy_mc_to_kernel() or copy_mc_user_highpage().

Both of those end up in copy_mc_enhanced_fast_string() which does
EX_TYPE_DEFAULT_MCE_SAFE.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 1/3] x86/mce: remove redundant fixup type EX_TYPE_COPY

[Tong Tiangen]

在 2024/1/31 5:09, Borislav Petkov 写道:
> On Thu, Jan 11, 2024 at 09:55:46PM +0800, Tong Tiangen wrote:
>> In commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache' function"),
>> rewrited __copy_user_nocache() uses EX_TYPE_UACCESS instead of
>> EX_TYPE_COPY，this change does not broken the MC safe copy for
>> __copy_user_nocache(), but as a result, there's no place for EX_TYPE_COPY
>> to use. Therefore, we remove the definition of EX_TYPE_COPY.
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
> Also, see section "Changelog" in
> Documentation/process/maintainer-tip.rst

OK, thanks, Improved in the next release.

> 
>> diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
>> index fe6312045042..787916ec1e12 100644
>> --- a/arch/x86/include/asm/extable_fixup_types.h
>> +++ b/arch/x86/include/asm/extable_fixup_types.h
>> @@ -36,18 +36,17 @@
>>   #define	EX_TYPE_DEFAULT			 1
>>   #define	EX_TYPE_FAULT			 2
>>   #define	EX_TYPE_UACCESS			 3
>> -#define	EX_TYPE_COPY			 4
>> -#define	EX_TYPE_CLEAR_FS		 5
>> -#define	EX_TYPE_FPU_RESTORE		 6
>> -#define	EX_TYPE_BPF			 7
>> -#define	EX_TYPE_WRMSR			 8
>> -#define	EX_TYPE_RDMSR			 9
>> -#define	EX_TYPE_WRMSR_SAFE		10 /* reg := -EIO */
>> -#define	EX_TYPE_RDMSR_SAFE		11 /* reg := -EIO */
>> -#define	EX_TYPE_WRMSR_IN_MCE		12
>> -#define	EX_TYPE_RDMSR_IN_MCE		13
>> -#define	EX_TYPE_DEFAULT_MCE_SAFE	14
>> -#define	EX_TYPE_FAULT_MCE_SAFE		15
>> +#define	EX_TYPE_CLEAR_FS		 4
>> +#define	EX_TYPE_FPU_RESTORE		 5
>> +#define	EX_TYPE_BPF			 6
>> +#define	EX_TYPE_WRMSR			 7
>> +#define	EX_TYPE_RDMSR			 8
>> +#define	EX_TYPE_WRMSR_SAFE		 9 /* reg := -EIO */
>> +#define	EX_TYPE_RDMSR_SAFE		10 /* reg := -EIO */
>> +#define	EX_TYPE_WRMSR_IN_MCE		11
>> +#define	EX_TYPE_RDMSR_IN_MCE		12
>> +#define	EX_TYPE_DEFAULT_MCE_SAFE	13
>> +#define	EX_TYPE_FAULT_MCE_SAFE		14
> 
> You don't need to renumber them - all you need to do is to comment it
> out:
> 
> /* unused, was: #define EX_TYPE_COPY 		4 */

OK.

Thanks.
Tong.

> 

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

在 2024/1/31 15:02, Borislav Petkov 写道:
> On Thu, Jan 11, 2024 at 09:55:47PM +0800, Tong Tiangen wrote:
>> Currently, there are some kernel memory copy scenarios is also mc safe
>> which use copy_mc_to_kernel() or copy_mc_user_highpage().
> 
> Both of those end up in copy_mc_enhanced_fast_string() which does
> EX_TYPE_DEFAULT_MCE_SAFE.

OK, how about this commit msg change? :)

Currently, there are some kernel memory copy scenarios is also mc safe
which use copy_mc_to_kernel() or copy_mc_user_highpage(), **both of 
those end up in copy_mc_enhanced_fast_string() or copy_mc_fragile() 
which does EX_TYPE_DEFAULT_MCE_SAFE.**

In these scenarios, posion pages need to be isolated too. Therefore, a
macro similar to MCE_IN_KERNEL_COPYIN is required. For this reason, we
can rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC, the new macro
can be applied to both user-to-kernel mc safe copy and kernel-to-kernel
mc safe copy.

Thanks.
Tong.

> 

Re: [PATCH -next v4 1/3] x86/mce: remove redundant fixup type EX_TYPE_COPY

[Tong Tiangen]

在 2024/1/31 5:09, Borislav Petkov 写道:
> On Thu, Jan 11, 2024 at 09:55:46PM +0800, Tong Tiangen wrote:
>> In commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache' function"),
>> rewrited __copy_user_nocache() uses EX_TYPE_UACCESS instead of
>> EX_TYPE_COPY，this change does not broken the MC safe copy for
>> __copy_user_nocache(), but as a result, there's no place for EX_TYPE_COPY
>> to use. Therefore, we remove the definition of EX_TYPE_COPY.
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
> 
> Also, see section "Changelog" in
> Documentation/process/maintainer-tip.rst

Hi Borislav:

how about this commit msg change?

Since commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache'
function") rewrited __copy_user_nocache() uses EX_TYPE_UACCESS
instead of EX_TYPE_COPY，there is no user for EX_TYPE_COPY，so
remove it.

Thanks.
Tong.

> 
>> diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
>> index fe6312045042..787916ec1e12 100644
>> --- a/arch/x86/include/asm/extable_fixup_types.h
>> +++ b/arch/x86/include/asm/extable_fixup_types.h
>> @@ -36,18 +36,17 @@
>>   #define	EX_TYPE_DEFAULT			 1
>>   #define	EX_TYPE_FAULT			 2
>>   #define	EX_TYPE_UACCESS			 3
>> -#define	EX_TYPE_COPY			 4
>> -#define	EX_TYPE_CLEAR_FS		 5
>> -#define	EX_TYPE_FPU_RESTORE		 6
>> -#define	EX_TYPE_BPF			 7
>> -#define	EX_TYPE_WRMSR			 8
>> -#define	EX_TYPE_RDMSR			 9
>> -#define	EX_TYPE_WRMSR_SAFE		10 /* reg := -EIO */
>> -#define	EX_TYPE_RDMSR_SAFE		11 /* reg := -EIO */
>> -#define	EX_TYPE_WRMSR_IN_MCE		12
>> -#define	EX_TYPE_RDMSR_IN_MCE		13
>> -#define	EX_TYPE_DEFAULT_MCE_SAFE	14
>> -#define	EX_TYPE_FAULT_MCE_SAFE		15
>> +#define	EX_TYPE_CLEAR_FS		 4
>> +#define	EX_TYPE_FPU_RESTORE		 5
>> +#define	EX_TYPE_BPF			 6
>> +#define	EX_TYPE_WRMSR			 7
>> +#define	EX_TYPE_RDMSR			 8
>> +#define	EX_TYPE_WRMSR_SAFE		 9 /* reg := -EIO */
>> +#define	EX_TYPE_RDMSR_SAFE		10 /* reg := -EIO */
>> +#define	EX_TYPE_WRMSR_IN_MCE		11
>> +#define	EX_TYPE_RDMSR_IN_MCE		12
>> +#define	EX_TYPE_DEFAULT_MCE_SAFE	13
>> +#define	EX_TYPE_FAULT_MCE_SAFE		14
> 
> You don't need to renumber them - all you need to do is to comment it
> out:
> 
> /* unused, was: #define EX_TYPE_COPY 		4 */
> 

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Thu, Feb 01, 2024 at 07:37:25PM +0800, Tong Tiangen wrote:
> 在 2024/1/31 15:02, Borislav Petkov 写道:
> > On Thu, Jan 11, 2024 at 09:55:47PM +0800, Tong Tiangen wrote:
> > > Currently, there are some kernel memory copy scenarios is also mc safe
> > > which use copy_mc_to_kernel() or copy_mc_user_highpage().
> > 
> > Both of those end up in copy_mc_enhanced_fast_string() which does
> > EX_TYPE_DEFAULT_MCE_SAFE.
> 
> OK, how about this commit msg change? :)
> 
> Currently, there are some kernel memory copy scenarios is also mc safe
> which use copy_mc_to_kernel() or copy_mc_user_highpage(), **both of those
> end up in copy_mc_enhanced_fast_string() or copy_mc_fragile() which does
> EX_TYPE_DEFAULT_MCE_SAFE.**
> 
> In these scenarios, posion pages need to be isolated too. Therefore, a
> macro similar to MCE_IN_KERNEL_COPYIN is required. For this reason, we
> can rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC, the new macro
> can be applied to both user-to-kernel mc safe copy and kernel-to-kernel
> mc safe copy.

Maybe my question wasn't clear: why is that renaming churn needed at
all? What are you "fixing" here?

What is the problem that you're encountering which needs fixing?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

在 2024/2/1 22:20, Borislav Petkov 写道:
> On Thu, Feb 01, 2024 at 07:37:25PM +0800, Tong Tiangen wrote:
>> 在 2024/1/31 15:02, Borislav Petkov 写道:
>>> On Thu, Jan 11, 2024 at 09:55:47PM +0800, Tong Tiangen wrote:
>>>> Currently, there are some kernel memory copy scenarios is also mc safe
>>>> which use copy_mc_to_kernel() or copy_mc_user_highpage().
>>>
>>> Both of those end up in copy_mc_enhanced_fast_string() which does
>>> EX_TYPE_DEFAULT_MCE_SAFE.
>>
>> OK, how about this commit msg change? :)
>>
>> Currently, there are some kernel memory copy scenarios is also mc safe
>> which use copy_mc_to_kernel() or copy_mc_user_highpage(), **both of those
>> end up in copy_mc_enhanced_fast_string() or copy_mc_fragile() which does
>> EX_TYPE_DEFAULT_MCE_SAFE.**
>>
>> In these scenarios, posion pages need to be isolated too. Therefore, a
>> macro similar to MCE_IN_KERNEL_COPYIN is required. For this reason, we
>> can rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC, the new macro
>> can be applied to both user-to-kernel mc safe copy and kernel-to-kernel
>> mc safe copy.
> 
> Maybe my question wasn't clear: why is that renaming churn needed at
> all? What are you "fixing" here?
> 
> What is the problem that you're encountering which needs fixing?

This patch is a prepare patch and the next patch is a fix patch, the
complete logic of the two patches is as follows:

The problem i'm encountering:
-------------------------------
In the x86 mce processing, error_context() setting macro
MCE_IN_KERNEL_COPYIN to identify copy from user(user-to-kernel copy) for
fixup_type EX_TYPE_UACCESS.

Then do_machine_check() uses macro MCE_IN_KERNEL_COPYIN to isolate
posion page in memory_failure().

Currently, there are some kernel memory copy scenarios is also mc safe
which use copy_mc_to_kernel() or copy_mc_user_highpage(), these kernel-
to-kernel copy use fixup_type EX_TYPE_DEFAULT_MCE_SAFE. In these
scenarios, posion pages need to be isolated too and the current
implementation is to actively call memory_failure_queue() when the copy
fails.

Calling memory_failure_queue() separately is not a good implementation,
call it uniformly in do_machine_check() is more reasonable.

Solution:
----------
A macro similar to MCE_IN_KERNEL_COPYIN is required, so we can rename
MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC, the new macro can be
applied to both user-to-kernel mc safe copy and kernel-to-kernel mc safe
copy, in error_context()，we can set MCE_IN_KERNEL_COPY_MC for both
fixup_type EX_TYPE_UACCESS and EX_TYPE_DEFAULT_MCE_SAFE.



Do you think it's clear to say so and then we can merge the two patches
to make the complete logic clearer in commit msg ?

Many thanks.
Tong.

> 
> Thx.
> 

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Fri, Feb 02, 2024 at 03:51:12PM +0800, Tong Tiangen wrote:
> Currently, there are some kernel memory copy scenarios is also mc safe
> which use copy_mc_to_kernel() or copy_mc_user_highpage(), these kernel-
> to-kernel copy use fixup_type EX_TYPE_DEFAULT_MCE_SAFE. In these
> scenarios, posion pages need to be isolated too and the current

So you have, for example:

  unsigned long __must_check copy_mc_to_kernel(void *dst, const void *src, unsigned len)

Now imagine you get a MCE for *dst which is some kernel page which
cannot be poisoned: direct map, kernel text, and so on.

Attempting to poison such a page would not work, to put it mildly.

So, again, what *exactly* are you "fixing" here?

When I read "Currently, there are some kernel memory copy scenarios" and
there's nothing more explaining what those scenarios are, I'm tempted to
ignore this completely until you give a detailed and concrete example
what the problem is:

What exactly are you doing, what goes wrong, why does this need to be
fixed and so on...

If there isn't such a real-life use case you're encountering, then this
all is waste of time.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Luck, Tony]

> So you have, for example:
>
>  unsigned long __must_check copy_mc_to_kernel(void *dst, const void *src, unsigned len)
>
> Now imagine you get a MCE for *dst which is some kernel page which
> cannot be poisoned: direct map, kernel text, and so on.

At least on Intel you can only get a machine check for operation on poison data LOAD.
Not for a STORE. I believe that is generally true - other arches to confirm.

So there can't me a machine check on "*dst".

-Tony

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Fri, Feb 02, 2024 at 06:44:32PM +0000, Luck, Tony wrote:
> At least on Intel you can only get a machine check for operation on poison data LOAD.
> Not for a STORE. I believe that is generally true - other arches to confirm.

So what happens if you store to a poisoned cacheline on Intel? It'll
raise a poison consumption error when that cacheline is loaded in the
cache? Because you need to load that line into the cache for writing,
I'd presume...

What happens if you have bits flipped in the cacheline you want to write
to?

That's fine because you're overwriting them anyway?

I'd presume ECC check gets performed on cacheline load and then you'll
have to raise an #MC...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Luck, Tony]

> > At least on Intel you can only get a machine check for operation on poison data LOAD.
> > Not for a STORE. I believe that is generally true - other arches to confirm.
>
> So what happens if you store to a poisoned cacheline on Intel? It'll
> raise a poison consumption error when that cacheline is loaded in the
> cache? Because you need to load that line into the cache for writing,
> I'd presume...

There are two places in the pipeline where poison is significant.

1) When the memory controller gets a request to fetch some data. If the ECC
check on the bits returned from the DIMMs the memory controller will log
a "UCNA" signature error to a machine check bank for the memory channel
where the DIMMs live. If CMCI is enabled for that bank, then a CMCI is
sent to all logical CPUs that are in the scope of that bank (generally a
CPU socket). The data is marked with a POISON signature and passed
to the entity that requested it. Caches support this POISON signature
and preserve it as data is moved between caches, or written back to
memory. This may have been a prefetch or a speculative read. In these
cases there won't be a machine check. Linux uc_decode_notifier() will
try to offline pages when it sees UCNA signatures.

2) When a CPU core tries to retire an instruction that consumes poison
data, or needs to retire a poisoned instruction. These log an SRAR signature
into a core scoped bank (on most Xeons to date bank 0 for poisoned instructions,
bank 1 for poisoned data consumption). Then they signal a machine check.

> What happens if you have bits flipped in the cacheline you want to write
> to?
>
> That's fine because you're overwriting them anyway?
>
> I'd presume ECC check gets performed on cacheline load and then you'll
> have to raise an #MC...

Partial cacheline stores to data marked as POISON in the cache maintain
the poison status. Full cacheline writes (certainly with MOVDIR64B instruction,
possibly with some AVX512 instructions) can clear the POISON status (since
you have all new data). A sequence of partial cache line stores that overwrite
all data in a cache line will NOT clear the POISON status.

Nothing is logged or signaled when updating data in the cache.

-Tony

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Fri, Feb 02, 2024 at 09:36:27PM +0000, Luck, Tony wrote:
> There are two places in the pipeline where poison is significant.
> 
> 1) When the memory controller gets a request to fetch some data. If the ECC
> check on the bits returned from the DIMMs the memory controller will log
> a "UCNA" signature error to a machine check bank for the memory channel
> where the DIMMs live. If CMCI is enabled for that bank, then a CMCI is
> sent to all logical CPUs that are in the scope of that bank (generally a
> CPU socket). The data is marked with a POISON signature and passed
> to the entity that requested it. Caches support this POISON signature
> and preserve it as data is moved between caches, or written back to
> memory. This may have been a prefetch or a speculative read. In these
> cases there won't be a machine check. Linux uc_decode_notifier() will
> try to offline pages when it sees UCNA signatures.

Yap, deferred errors.

> 2) When a CPU core tries to retire an instruction that consumes poison
> data, or needs to retire a poisoned instruction. These log an SRAR signature
> into a core scoped bank (on most Xeons to date bank 0 for poisoned instructions,
> bank 1 for poisoned data consumption). Then they signal a machine check.

And that is the #MC on a poison data load thing. FWIW, the other vendor
does it very similarly.

> Partial cacheline stores to data marked as POISON in the cache maintain
> the poison status. Full cacheline writes (certainly with MOVDIR64B instruction,
> possibly with some AVX512 instructions) can clear the POISON status (since
> you have all new data). A sequence of partial cache line stores that overwrite
> all data in a cache line will NOT clear the POISON status.

That's interesting - partial stores don't clear poison data.

> Nothing is logged or signaled when updating data in the cache.

Ok, no #MC on writing to poisoned cachelines.

Ok, so long story short, #MC only on loads. Good.

Now, since you're explaining things today :) pls explain to me what this
patchset is all about? You having reviewed patch 3 and all?

Why is this pattern:

	if (copy_mc_user_highpage(dst, src, addr, vma)) {
		memory_failure_queue(page_to_pfn(src), 0);

not good anymore?

Or is the goal here to poison straight from the #MC handler and not
waste time and potentially get another #MC while memory_failure_queue()
on the source address is done?

Or something completely different?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Luck, Tony]

> Now, since you're explaining things today :) pls explain to me what this
> patchset is all about? You having reviewed patch 3 and all?
>
> Why is this pattern:
>
>       if (copy_mc_user_highpage(dst, src, addr, vma)) {
>               memory_failure_queue(page_to_pfn(src), 0);
>
> not good anymore?
>
> Or is the goal here to poison straight from the #MC handler and not
> waste time and potentially get another #MC while memory_failure_queue()
> on the source address is done?
>
> Or something completely different?

See the comment above memory_failure_queue()

* The function is primarily of use for corruptions that
 * happen outside the current execution context (e.g. when
 * detected by a background scrubber)

In the copy_mc_user_highpage() case the fault happens in
the current execution context. So scheduling someone else
to handle it at some future point is risky. Just deal with it
right away.

-Tony

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

在 2024/2/3 6:46, Luck, Tony 写道:
>> Now, since you're explaining things today :) pls explain to me what this
>> patchset is all about? You having reviewed patch 3 and all?
>>
>> Why is this pattern:
>>
>>        if (copy_mc_user_highpage(dst, src, addr, vma)) {
>>                memory_failure_queue(page_to_pfn(src), 0);
>>
>> not good anymore?
>>
>> Or is the goal here to poison straight from the #MC handler and not
>> waste time and potentially get another #MC while memory_failure_queue()
>> on the source address is done?
>>
>> Or something completely different?
> 
> See the comment above memory_failure_queue()
> 
> * The function is primarily of use for corruptions that
>   * happen outside the current execution context (e.g. when
>   * detected by a background scrubber)
> 
> In the copy_mc_user_highpage() case the fault happens in
> the current execution context. So scheduling someone else
> to handle it at some future point is risky. Just deal with it
> right away.
> 
> -Tony

The goal of this patch:
   When #MC is triggered by copy_mc_user_highpage(), #MC is directly
processed in the synchronously triggered do_machine_check() ->
kill_me_never() -> memory_failure().

And the current handling is to call memory_failure_queue() ->
schedule_work_on() in the execution context, I think that's what
"scheduling someone else to handle it at some future point is risky."

Thanks.
Tong.

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Borislav Petkov]

On Sat, Feb 03, 2024 at 03:56:04PM +0800, Tong Tiangen wrote:
> The goal of this patch:
>   When #MC is triggered by copy_mc_user_highpage(), #MC is directly
> processed in the synchronously triggered do_machine_check() ->
> kill_me_never() -> memory_failure().
> 
> And the current handling is to call memory_failure_queue() ->
> schedule_work_on() in the execution context, I think that's what
> "scheduling someone else to handle it at some future point is risky."

Ok, now take everything that was discussed on the thread and use it to
rewrite all your commit messages to explain *why* you're doing this, not
*what* you're doing - that is visible from the diff.

A possible way to structure them is:

1. Prepare the context for the explanation briefly.

2. Explain the problem at hand.

3. "It happens because of <...>"

4. "Fix it by doing X"

5. "(Potentially do Y)."

And some of those above are optional depending on the issue being
explained.

For more detailed info, see
Documentation/process/submitting-patches.rst,
Section "2) Describe your changes".

Also, to the tone, from Documentation/process/submitting-patches.rst:

 "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour."

Also, do not talk about what your patch does - that should (hopefully) be
visible from the diff itself. Rather, talk about *why* you're doing what
you're doing.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v4 2/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

在 2024/2/3 17:43, Borislav Petkov 写道:
> On Sat, Feb 03, 2024 at 03:56:04PM +0800, Tong Tiangen wrote:
>> The goal of this patch:
>>    When #MC is triggered by copy_mc_user_highpage(), #MC is directly
>> processed in the synchronously triggered do_machine_check() ->
>> kill_me_never() -> memory_failure().
>>
>> And the current handling is to call memory_failure_queue() ->
>> schedule_work_on() in the execution context, I think that's what
>> "scheduling someone else to handle it at some future point is risky."
> 
> Ok, now take everything that was discussed on the thread and use it to
> rewrite all your commit messages to explain *why* you're doing this, not
> *what* you're doing - that is visible from the diff.
> 
> A possible way to structure them is:
> 
> 1. Prepare the context for the explanation briefly.
> 
> 2. Explain the problem at hand.
> 
> 3. "It happens because of <...>"
> 
> 4. "Fix it by doing X"
> 
> 5. "(Potentially do Y)."
> 
> And some of those above are optional depending on the issue being
> explained.
> 
> For more detailed info, see
> Documentation/process/submitting-patches.rst,
> Section "2) Describe your changes".
> 
> Also, to the tone, from Documentation/process/submitting-patches.rst:
> 
>   "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>    to do frotz", as if you are giving orders to the codebase to change
>    its behaviour."
> 
> Also, do not talk about what your patch does - that should (hopefully) be
> visible from the diff itself. Rather, talk about *why* you're doing what
> you're doing.

OK, will improved next version.

Thank.
Tong.

> 
> Thx.
>