[PATCH v1 0/2] Add NOTIFY_SEI notification type support

[PATCH v1 0/2] Add NOTIFY_SEI notification type support

[Dongjiu Geng]

This series patch is separated from https://www.spinics.net/lists/kvm/msg168917.html

1. CPI 6.1 adds support for NOTIFY_SEI as a GHES notification mechanism, so this patch supports this
   notification in software

Dongjiu Geng (2):
  ACPI / APEI: Add SEI notification type support for ARMv8
  arm64: handle NOTIFY_SEI notification by the APEI driver

 arch/arm64/kernel/traps.c | 15 ++++++++++++++
 drivers/acpi/apei/Kconfig | 15 ++++++++++++++
 drivers/acpi/apei/ghes.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/ghes.h       |  1 +
 4 files changed, 84 insertions(+)

-- 
1.9.1

[PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8

[Dongjiu Geng]

ACPI 6.x adds support for NOTIFY_SEI as a GHES notification
mechanism, so add new GHES notification handling functions.
Expose API ghes_notify_sei() to arch code, arch code will call
this API when it gets this NOTIFY_SEI.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
Note:
Firmware will follow the SError mask rule, if the SError is masked,
the firmware will not deliver NOTIFY_SEI notification.
---
 drivers/acpi/apei/Kconfig | 15 ++++++++++++++
 drivers/acpi/apei/ghes.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/ghes.h       |  1 +
 3 files changed, 69 insertions(+)

diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index 52ae543..ff4afc3 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -55,6 +55,21 @@ config ACPI_APEI_SEA
 	  option allows the OS to look for such hardware error record, and
 	  take appropriate action.
 
+config ACPI_APEI_SEI
+	bool "APEI SError(System Error) Interrupt logging/recovering support"
+	depends on ARM64 && ACPI_APEI_GHES
+	default y
+	help
+	  This option should be enabled if the system supports
+	  firmware first handling of SEI (SError interrupt).
+
+	  SEI happens with asynchronous external abort for errors on device
+	  memory reads on ARMv8 systems. If a system supports firmware first
+	  handling of SEI, the platform analyzes and handles hardware error
+	  notifications from SEI, and it may then form a hardware error record for
+	  the OS to parse and handle. This option allows the OS to look for
+	  such hardware error record, and take appropriate action.
+
 config ACPI_APEI_MEMORY_FAILURE
 	bool "APEI memory error recovering support"
 	depends on ACPI_APEI && MEMORY_FAILURE
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 1efefe9..33f77ae 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -827,6 +827,46 @@ static inline void ghes_sea_add(struct ghes *ghes) { }
 static inline void ghes_sea_remove(struct ghes *ghes) { }
 #endif /* CONFIG_ACPI_APEI_SEA */
 
+#ifdef CONFIG_ACPI_APEI_SEI
+static LIST_HEAD(ghes_sei);
+
+/*
+ * Return 0 only if one of the SEI error sources successfully reported an error
+ * record sent from the firmware.
+ */
+int ghes_notify_sei(void)
+{
+	struct ghes *ghes;
+	int ret = -ENOENT;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ghes, &ghes_sei, list) {
+		if (!ghes_proc(ghes))
+			ret = 0;
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
+static void ghes_sei_add(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	list_add_rcu(&ghes->list, &ghes_sei);
+	mutex_unlock(&ghes_list_mutex);
+}
+
+static void ghes_sei_remove(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	list_del_rcu(&ghes->list);
+	mutex_unlock(&ghes_list_mutex);
+	synchronize_rcu();
+}
+#else /* CONFIG_ACPI_APEI_SEI */
+static inline void ghes_sei_add(struct ghes *ghes) { }
+static inline void ghes_sei_remove(struct ghes *ghes) { }
+#endif /* CONFIG_ACPI_APEI_SEI */
+
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 /*
  * printk is not safe in NMI context.  So in NMI handler, we allocate
@@ -1055,6 +1095,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
 			goto err;
 		}
 		break;
+	case ACPI_HEST_NOTIFY_SEI:
+		if (!IS_ENABLED(CONFIG_ACPI_APEI_SEI)) {
+			pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEI is not supported!\n",
+				generic->header.source_id);
+		goto err;
+	}
+	break;
 	case ACPI_HEST_NOTIFY_NMI:
 		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
 			pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n",
@@ -1126,6 +1173,9 @@ static int ghes_probe(struct platform_device *ghes_dev)
 	case ACPI_HEST_NOTIFY_SEA:
 		ghes_sea_add(ghes);
 		break;
+	case ACPI_HEST_NOTIFY_SEI:
+		ghes_sei_add(ghes);
+		break;
 	case ACPI_HEST_NOTIFY_NMI:
 		ghes_nmi_add(ghes);
 		break;
@@ -1179,6 +1229,9 @@ static int ghes_remove(struct platform_device *ghes_dev)
 	case ACPI_HEST_NOTIFY_SEA:
 		ghes_sea_remove(ghes);
 		break;
+	case ACPI_HEST_NOTIFY_SEI:
+		ghes_sei_remove(ghes);
+		break;
 	case ACPI_HEST_NOTIFY_NMI:
 		ghes_nmi_remove(ghes);
 		break;
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 8feb0c8..9ba59e2 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -120,5 +120,6 @@ static inline void *acpi_hest_get_next(struct acpi_hest_generic_data *gdata)
 	     section = acpi_hest_get_next(section))
 
 int ghes_notify_sea(void);
+int ghes_notify_sei(void);
 
 #endif /* GHES_H */
-- 
1.9.1

Re: [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8

[Mark Rutland]

On Thu, May 31, 2018 at 08:41:45PM +0800, Dongjiu Geng wrote:
> +#ifdef CONFIG_ACPI_APEI_SEI
> +static LIST_HEAD(ghes_sei);
> +
> +/*
> + * Return 0 only if one of the SEI error sources successfully reported an error
> + * record sent from the firmware.
> + */
> +int ghes_notify_sei(void)
> +{
> +	struct ghes *ghes;
> +	int ret = -ENOENT;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ghes, &ghes_sei, list) {
> +		if (!ghes_proc(ghes))
> +			ret = 0;
> +	}
> +	rcu_read_unlock();
> +	return ret;
> +}
> +
> +static void ghes_sei_add(struct ghes *ghes)
> +{
> +	mutex_lock(&ghes_list_mutex);
> +	list_add_rcu(&ghes->list, &ghes_sei);
> +	mutex_unlock(&ghes_list_mutex);
> +}
> +
> +static void ghes_sei_remove(struct ghes *ghes)
> +{
> +	mutex_lock(&ghes_list_mutex);
> +	list_del_rcu(&ghes->list);
> +	mutex_unlock(&ghes_list_mutex);
> +	synchronize_rcu();
> +}
> +#else /* CONFIG_ACPI_APEI_SEI */
> +static inline void ghes_sei_add(struct ghes *ghes) { }
> +static inline void ghes_sei_remove(struct ghes *ghes) { }
> +#endif /* CONFIG_ACPI_APEI_SEI */
> +
>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>  /*

> index 8feb0c8..9ba59e2 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -120,5 +120,6 @@ static inline void *acpi_hest_get_next(struct acpi_hest_generic_data *gdata)
>  	     section = acpi_hest_get_next(section))
>  
>  int ghes_notify_sea(void);
> +int ghes_notify_sei(void);

It would be nice to have a stub definition when !CONFIG_ACPI_APEI_SEI,
e.g.

#ifdef CONFIG_ACPI_APEI_SEI
int ghes_notify_sei(void);
#else
static in int ghes_notify_sei(void) { return -ENOENT; }
#endif

... as callers could simply call this without additional checks.

As a cleanup, similar would be nice for ghes_notify_sea() with
CONFIG_ACPI_APEI_SEA.

Thanks,
Mark.

Re: [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8

[gengdongjiu]

Hi Mark,
   Thanks for the comments.

On 2018/5/31 18:52, Mark Rutland wrote:
> On Thu, May 31, 2018 at 08:41:45PM +0800, Dongjiu Geng wrote:
>> +#ifdef CONFIG_ACPI_APEI_SEI
>> +static LIST_HEAD(ghes_sei);
>> +
>> +/*
>> + * Return 0 only if one of the SEI error sources successfully reported an error
>> + * record sent from the firmware.
>> + */
>> +int ghes_notify_sei(void)
>> +{
>> +	struct ghes *ghes;
>> +	int ret = -ENOENT;
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(ghes, &ghes_sei, list) {
>> +		if (!ghes_proc(ghes))
>> +			ret = 0;
>> +	}
>> +	rcu_read_unlock();
>> +	return ret;
>> +}
>> +
>> +static void ghes_sei_add(struct ghes *ghes)
>> +{
>> +	mutex_lock(&ghes_list_mutex);
>> +	list_add_rcu(&ghes->list, &ghes_sei);
>> +	mutex_unlock(&ghes_list_mutex);
>> +}
>> +
>> +static void ghes_sei_remove(struct ghes *ghes)
>> +{
>> +	mutex_lock(&ghes_list_mutex);
>> +	list_del_rcu(&ghes->list);
>> +	mutex_unlock(&ghes_list_mutex);
>> +	synchronize_rcu();
>> +}
>> +#else /* CONFIG_ACPI_APEI_SEI */
>> +static inline void ghes_sei_add(struct ghes *ghes) { }
>> +static inline void ghes_sei_remove(struct ghes *ghes) { }
>> +#endif /* CONFIG_ACPI_APEI_SEI */
>> +
>>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>>  /*
> 
>> index 8feb0c8..9ba59e2 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -120,5 +120,6 @@ static inline void *acpi_hest_get_next(struct acpi_hest_generic_data *gdata)
>>  	     section = acpi_hest_get_next(section))
>>  
>>  int ghes_notify_sea(void);
>> +int ghes_notify_sei(void);
> 
> It would be nice to have a stub definition when !CONFIG_ACPI_APEI_SEI,
> e.g.
> 
> #ifdef CONFIG_ACPI_APEI_SEI
> int ghes_notify_sei(void);
> #else
> static in int ghes_notify_sei(void) { return -ENOENT; }
> #endif
> 
> ... as callers could simply call this without additional checks.
> 
> As a cleanup, similar would be nice for ghes_notify_sea() with
> CONFIG_ACPI_APEI_SEA.

I think your suggestion is better, I will do the cleanup include the ghes_notify_sea().
thanks again.

> 
> Thanks,
> Mark.
> 
> .
> 

[PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver

[Dongjiu Geng]

When kernel or KVM gets the NOTIFY_SEI notification, it firstly
calls the APEI driver to handle this notification.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 arch/arm64/kernel/traps.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
---
change since https://www.spinics.net/lists/kvm/msg168919.html

1. Remove the handle_guest_sei() helper


diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8bbdc17..676e40c 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -50,6 +50,7 @@
 #include <asm/exception.h>
 #include <asm/system_misc.h>
 #include <asm/sysreg.h>
+#include <acpi/ghes.h>
 
 static const char *handler[]= {
 	"Synchronous Abort",
@@ -701,6 +702,20 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
 bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
 {
 	u32 aet = arm64_ras_serror_get_severity(esr);
+	int ret = -ENOENT;
+
+	if (IS_ENABLED(CONFIG_ACPI_APEI_SEI)) {
+		if (interrupts_enabled(regs))
+			nmi_enter();
+
+		ret = ghes_notify_sei();
+
+		if (interrupts_enabled(regs))
+			nmi_exit();
+
+		if (!ret)
+			return false;
+	}
 
 	switch (aet) {
 	case ESR_ELx_AET_CE:	/* corrected error */
-- 
1.9.1

Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver

[Mark Rutland]

On Thu, May 31, 2018 at 08:41:46PM +0800, Dongjiu Geng wrote:
> When kernel or KVM gets the NOTIFY_SEI notification, it firstly
> calls the APEI driver to handle this notification.
> 
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> ---
>  arch/arm64/kernel/traps.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> ---
> change since https://www.spinics.net/lists/kvm/msg168919.html
> 
> 1. Remove the handle_guest_sei() helper
> 
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 8bbdc17..676e40c 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -50,6 +50,7 @@
>  #include <asm/exception.h>
>  #include <asm/system_misc.h>
>  #include <asm/sysreg.h>
> +#include <acpi/ghes.h>

Nit: please place newline before the new include, since it comes from a
different directory (and we do so in fault.c).

>  static const char *handler[]= {
>  	"Synchronous Abort",
> @@ -701,6 +702,20 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
>  bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
>  {
>  	u32 aet = arm64_ras_serror_get_severity(esr);
> +	int ret = -ENOENT;
> +
> +	if (IS_ENABLED(CONFIG_ACPI_APEI_SEI)) {
> +		if (interrupts_enabled(regs))
> +			nmi_enter();
> +
> +		ret = ghes_notify_sei();
> +
> +		if (interrupts_enabled(regs))
> +			nmi_exit();
> +
> +		if (!ret)
> +			return false;
> +	}

In do_serror() we already handle nmi_{enter,exit}(), so there's no need
for that here.

TBH, I don't understand why do_sea() does that conditionally today.
Unless there's some constraint I'm missing, I think it would make more
sense to do that regardless of whether the interrupted context had
interrupts enabled. James -- does that make sense to you?

If you update the prior patch with a stub for !CONFIG_ACPI_APEI_SEI, you
can simplify all of the above additions down to:

	if (!ghes_notify_sei())
		return;

... which would look a lot nicer.

Thanks,
Mark.

Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver

[James Morse]

Hi Mark, Dongjiu Geng,

On 31/05/18 12:01, Mark Rutland wrote:
> In do_serror() we already handle nmi_{enter,exit}(), so there's no need
> for that here.

Even better: nmi_enter() has a BUG_ON(in_nmi()).


> TBH, I don't understand why do_sea() does that conditionally today.
> Unless there's some constraint I'm missing, 

APEI uses a different fixmap entry and locks when in_nmi(). This was because we
may interrupt the irq-masked region in APEI that was using the regular memory.
(e.g. the 'polled' notification, or something backed by an interrupt.) But,
Borislav has spotted other things in here that are broken[0]. I'm working on
rolling all that into 'v5' of the in_nmi() rework stuff.

We currently get away with this on arm because 'SEA' is the only NMI-like thing,
and it occurs synchronously. The problem cases are all also cases where the
kernel text is corrupt, which we can't possibly hope to handle.

For NOTIFY_SDEI and NOTIFY_SEI this is the wrong pattern as these are
asynchronous. do_serror() has already done most of the work for NOTIFY_SEI, but
we need to use the estatus queue in APEI, which is currently x86 only.


> I think it would make more
> sense to do that regardless of whether the interrupted context had
> interrupts enabled. James -- does that make sense to you?
> 
> If you update the prior patch with a stub for !CONFIG_ACPI_APEI_SEI, you
> can simplify all of the above additions down to:
> 
> 	if (!ghes_notify_sei())
> 		return;
> 
> ... which would look a lot nicer.

The code that calls ghes_notify_sei() may need calling by KVM too, but its
default action to an 'unclaimed' SError will be different.
Because of the race between memory_failure() and return-to-userspace, we may
need to kick the irq work queue (if we can), as we return from do_serror(). [1]
and [2] provide an example for NOTIFY_SEA. SDEI does this by returning to the
kernel through the IRQ handler, (which handles the KVM case too).


I think this series is unsafe until we can use the estatus queue in APEI. Its
also missing the handling for an SError interrupting a KVM guest.


Thanks,

James

[0] https://www.spinics.net/lists/arm-kernel/msg653332.html
[1] https://www.spinics.net/lists/arm-kernel/msg649237.html
[2] https://www.spinics.net/lists/arm-kernel/msg649239.html

Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver

[gengdongjiu]

Hi Mark, James,
   Thanks the review.

On 2018/6/1 0:51, James Morse wrote:
> Hi Mark, Dongjiu Geng,
> 
> On 31/05/18 12:01, Mark Rutland wrote:
>> In do_serror() we already handle nmi_{enter,exit}(), so there's no need
>> for that here.
> 
> Even better: nmi_enter() has a BUG_ON(in_nmi()).

There are two places call the arm64_is_fatal_ras_serror():
1. do_serror()
2. kvm_handle_guest_serror()

Yes, the do_serror() already handle nmi_{enter,exit}(), so the arm64_is_fatal_ras_serror() does not need to handle it again.
For the kvm_handle_guest_serror(), it does not handle the nmi_{enter,exit}(), so should we add nmi_{enter,exit}() in  kvm_handle_guest_serror() before calling arm64_is_fatal_ras_serror()?

For the NOTIFY_SEA, I do not know why handle_guest_sea() does not handle the nmi_{enter,exit}(). James, does we miss it in handle_guest_sea()?

> 
> 
>> TBH, I don't understand why do_sea() does that conditionally today.
>> Unless there's some constraint I'm missing, 
> 
> APEI uses a different fixmap entry and locks when in_nmi(). This was because we
> may interrupt the irq-masked region in APEI that was using the regular memory.
> (e.g. the 'polled' notification, or something backed by an interrupt.) But,
> Borislav has spotted other things in here that are broken[0]. I'm working on
> rolling all that into 'v5' of the in_nmi() rework stuff.
> 
> We currently get away with this on arm because 'SEA' is the only NMI-like thing,
> and it occurs synchronously. The problem cases are all also cases where the
> kernel text is corrupt, which we can't possibly hope to handle.
> 
> For NOTIFY_SDEI and NOTIFY_SEI this is the wrong pattern as these are
> asynchronous. do_serror() has already done most of the work for NOTIFY_SEI, but
> we need to use the estatus queue in APEI, which is currently x86 only.
I think this patch can based on you 'v5' of the in_nmi() rework stuff

> 
> 
>> I think it would make more
>> sense to do that regardless of whether the interrupted context had
>> interrupts enabled. James -- does that make sense to you?
>>
>> If you update the prior patch with a stub for !CONFIG_ACPI_APEI_SEI, you
>> can simplify all of the above additions down to:
>>
>> 	if (!ghes_notify_sei())
>> 		return;
>>
>> ... which would look a lot nicer.
> 
> The code that calls ghes_notify_sei() may need calling by KVM too, but its
> default action to an 'unclaimed' SError will be different.
> Because of the race between memory_failure() and return-to-userspace, we may
> need to kick the irq work queue (if we can), as we return from do_serror(). [1]
> and [2] provide an example for NOTIFY_SEA. SDEI does this by returning to the
> kernel through the IRQ handler, (which handles the KVM case too).
I can kick the IRQ work queue as you do for the NOTIFY_SEA and NOTIFY_SDEI.

> 
> 
> I think this series is unsafe until we can use the estatus queue in APEI. Its
> also missing the handling for an SError interrupting a KVM guest.

how about this series is based on your patches that uses the estatus queue in APEI to make it safe?

when an SError interrupting a KVM guest, it will trap to hypervisor, hypervisor will call
below software stack to handle it:
kvm_handle_guest_serror()->arm64_is_fatal_ras_serror()->ghes_notify_sei()

so it already handles the case that an SError interrupting a KVM guest.

> 
> 
> Thanks,
> 
> James
> 
> [0] https://www.spinics.net/lists/arm-kernel/msg653332.html
> [1] https://www.spinics.net/lists/arm-kernel/msg649237.html
> [2] https://www.spinics.net/lists/arm-kernel/msg649239.html
> 
> .
> 

Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver

[James Morse]

Hi gengdongjiu,

On 01/06/18 08:21, gengdongjiu wrote:
> On 2018/6/1 0:51, James Morse wrote:
>> On 31/05/18 12:01, Mark Rutland wrote:
>>> In do_serror() we already handle nmi_{enter,exit}(), so there's no need
>>> for that here.
>>
>> Even better: nmi_enter() has a BUG_ON(in_nmi()).
> 
> There are two places call the arm64_is_fatal_ras_serror():
> 1. do_serror()
> 2. kvm_handle_guest_serror()
> 
> Yes, the do_serror() already handle nmi_{enter,exit}(), so the arm64_is_fatal_ras_serror() does not need to handle it again.

> For the kvm_handle_guest_serror(), it does not handle the nmi_{enter,exit}(),
> so should we add nmi_{enter,exit}() in  kvm_handle_guest_serror() before calling
> arm64_is_fatal_ras_serror()?

kvm_handle_guest_serror() never interrupts anything, so its not an NMI, this is
why the KVM code doesn't do this today. We can take another SError from there
without issue.

But once we call APEI, it needs to prevent re-entrance, so SError has to be
masked. If we're using the estatus-queue (which we should), then we should
always call the notification helper in the same context. If do_serror() is
in_nmi(), then we should fake it up in the appropriate helper.

Given the latest review comments from Borislav, the in_nmi() weirdness in APEI
may all disappear, in which case all that matters is we call APEI with SError
masked.


> > For the NOTIFY_SEA, I do not know why handle_guest_sea() does not handle
> the nmi_{enter,exit}(). James, does we miss it in handle_guest_sea()?

The exception went to the hypervisor, it only interrupted the guest. KVM returns
to process context, and KVM then goes on to call handle_guest_sea().

nmi_enter() is to tell things like printk() that we may have interrupted its
IRQ-masked regions, and it can't take any locks to do the work. We know this is
never true for KVM at this point.


>>> TBH, I don't understand why do_sea() does that conditionally today.
>>> Unless there's some constraint I'm missing, 
>>
>> APEI uses a different fixmap entry and locks when in_nmi(). This was because we
>> may interrupt the irq-masked region in APEI that was using the regular memory.
>> (e.g. the 'polled' notification, or something backed by an interrupt.) But,
>> Borislav has spotted other things in here that are broken[0]. I'm working on
>> rolling all that into 'v5' of the in_nmi() rework stuff.
>>
>> We currently get away with this on arm because 'SEA' is the only NMI-like thing,
>> and it occurs synchronously. The problem cases are all also cases where the
>> kernel text is corrupt, which we can't possibly hope to handle.
>>
>> For NOTIFY_SDEI and NOTIFY_SEI this is the wrong pattern as these are
>> asynchronous. do_serror() has already done most of the work for NOTIFY_SEI, but
>> we need to use the estatus queue in APEI, which is currently x86 only.

> I think this patch can based on you 'v5' of the in_nmi() rework stuff

I think NOTIFY_SEI needs the estatus-queue, so yes. But the KVM changes mean
this will need doing as a separate series.


>>> I think it would make more
>>> sense to do that regardless of whether the interrupted context had
>>> interrupts enabled. James -- does that make sense to you?
>>>
>>> If you update the prior patch with a stub for !CONFIG_ACPI_APEI_SEI, you
>>> can simplify all of the above additions down to:
>>>
>>> 	if (!ghes_notify_sei())
>>> 		return;
>>>
>>> ... which would look a lot nicer.
>>
>> The code that calls ghes_notify_sei() may need calling by KVM too, but its
>> default action to an 'unclaimed' SError will be different.
>> Because of the race between memory_failure() and return-to-userspace, we may
>> need to kick the irq work queue (if we can), as we return from do_serror(). [1]
>> and [2] provide an example for NOTIFY_SEA. SDEI does this by returning to the
>> kernel through the IRQ handler, (which handles the KVM case too).

> I can kick the IRQ work queue as you do for the NOTIFY_SEA and NOTIFY_SDEI.

(NOTIFY_SDEI has its own tricks: it returns to the kernel through the IRQ VBAR
entry)

I think this needs something like the apei_claim_sea() in that series, probably
with some __ version called from do_serror() to avoid the BUG_ON(in_nmi()).


>> I think this series is unsafe until we can use the estatus queue in APEI. Its
>> also missing the handling for an SError interrupting a KVM guest.
> 
> how about this series is based on your patches that uses the estatus queue in APEI to make it safe?
> 
> when an SError interrupting a KVM guest, it will trap to hypervisor, hypervisor will call
> below software stack to handle it:
> kvm_handle_guest_serror()->arm64_is_fatal_ras_serror()->ghes_notify_sei()
> 
> so it already handles the case that an SError interrupting a KVM guest.

It already calls it at some point... this was called out in the cover-letter
(and maybe commit messages) of the series that added that code: We will need to
do more once this is used as a notification from firmware.

That series was about unmasking SError as much as possible, and enabling IESB.
We then had to handle any case where we may now take an SError, but without
kernel-first support (which requires some topology information) all we could do
is ignore corrected/restartable errors, and panic() otherwise. This isn't
actually trying to handle the error.

KVM calls this code quite late. Now that we are trying to handle the error, the
'when' matters. SError is unmasked when we return to the run-loop, we may take
another, less severe, SError before checking the value we had from guest-exit.
We unmask interrupts, we may take an interrupt and run some other code, while
holding an SError ESR that says the system can't be trusted anymore.

(incidentally ... firmware should avoid making two NOTIFY_SEI pending for the
system at the same time. The SError doesn't tell us which GHES or set of CPER
records correspond with this error, so we have to walk the list and process
whatever we find. Two CPUs doing this at the same time will do the wrong thing
if the CPU-context on one of them matters.).


Thanks,

James