* [PATCH v1 0/2] Add NOTIFY_SEI notification type support @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 0/2] Add NOTIFY_SEI notification type support @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 0/2] Add NOTIFY_SEI notification type support @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 2018-05-31 12:41 ` Dongjiu Geng (?) @ 2018-05-31 12:41 ` Dongjiu Geng -1 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 2018-05-31 12:41 ` Dongjiu Geng @ 2018-05-31 10:52 ` Mark Rutland -1 siblings, 0 replies; 23+ messages in thread From: Mark Rutland @ 2018-05-31 10:52 UTC (permalink / raw) To: Dongjiu Geng Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, james.morse, linux-arm-kernel, linux-kernel, linux-acpi 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 @ 2018-05-31 10:52 ` Mark Rutland 0 siblings, 0 replies; 23+ messages in thread From: Mark Rutland @ 2018-05-31 10:52 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 2018-05-31 10:52 ` Mark Rutland (?) @ 2018-06-01 10:07 ` gengdongjiu -1 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 10:07 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, james.morse, linux-arm-kernel, linux-kernel, linux-acpi 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. > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 @ 2018-06-01 10:07 ` gengdongjiu 0 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 10:07 UTC (permalink / raw) To: linux-arm-kernel 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. > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 @ 2018-06-01 10:07 ` gengdongjiu 0 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 10:07 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, james.morse, linux-arm-kernel, linux-kernel, linux-acpi 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. > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver 2018-05-31 12:41 ` Dongjiu Geng (?) @ 2018-05-31 12:41 ` Dongjiu Geng -1 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-05-31 12:41 ` Dongjiu Geng 0 siblings, 0 replies; 23+ messages in thread From: Dongjiu Geng @ 2018-05-31 12:41 UTC (permalink / raw) To: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, mark.rutland, dave.martin, ard.biesheuvel, james.morse, gengdongjiu, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver 2018-05-31 12:41 ` Dongjiu Geng @ 2018-05-31 11:01 ` Mark Rutland -1 siblings, 0 replies; 23+ messages in thread From: Mark Rutland @ 2018-05-31 11:01 UTC (permalink / raw) To: Dongjiu Geng, james.morse Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, linux-arm-kernel, linux-kernel, linux-acpi 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-05-31 11:01 ` Mark Rutland 0 siblings, 0 replies; 23+ messages in thread From: Mark Rutland @ 2018-05-31 11:01 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver 2018-05-31 11:01 ` Mark Rutland @ 2018-05-31 16:51 ` James Morse -1 siblings, 0 replies; 23+ messages in thread From: James Morse @ 2018-05-31 16:51 UTC (permalink / raw) To: Mark Rutland, Dongjiu Geng Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-05-31 16:51 ` James Morse 0 siblings, 0 replies; 23+ messages in thread From: James Morse @ 2018-05-31 16:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver 2018-05-31 16:51 ` James Morse (?) @ 2018-06-01 7:21 ` gengdongjiu -1 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 7:21 UTC (permalink / raw) To: James Morse, Mark Rutland Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, linux-arm-kernel, linux-kernel, linux-acpi 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 > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-06-01 7:21 ` gengdongjiu 0 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 7:21 UTC (permalink / raw) To: linux-arm-kernel 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 > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-06-01 7:21 ` gengdongjiu 0 siblings, 0 replies; 23+ messages in thread From: gengdongjiu @ 2018-06-01 7:21 UTC (permalink / raw) To: James Morse, Mark Rutland Cc: catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, linux-arm-kernel, linux-kernel, linux-acpi 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 > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver 2018-06-01 7:21 ` gengdongjiu @ 2018-06-15 16:49 ` James Morse -1 siblings, 0 replies; 23+ messages in thread From: James Morse @ 2018-06-15 16:49 UTC (permalink / raw) To: gengdongjiu Cc: Mark Rutland, catalin.marinas, will.deacon, rjw, lenb, tony.luck, bp, robert.moore, erik.schmauss, dave.martin, ard.biesheuvel, linux-arm-kernel, linux-kernel, linux-acpi 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver @ 2018-06-15 16:49 ` James Morse 0 siblings, 0 replies; 23+ messages in thread From: James Morse @ 2018-06-15 16:49 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-06-15 16:49 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-31 12:41 [PATCH v1 0/2] Add NOTIFY_SEI notification type support Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 12:41 ` [PATCH v1 1/2] ACPI / APEI: Add SEI notification type support for ARMv8 Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 10:52 ` Mark Rutland 2018-05-31 10:52 ` Mark Rutland 2018-06-01 10:07 ` gengdongjiu 2018-06-01 10:07 ` gengdongjiu 2018-06-01 10:07 ` gengdongjiu 2018-05-31 12:41 ` [PATCH v1 2/2] arm64: handle NOTIFY_SEI notification by the APEI driver Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 12:41 ` Dongjiu Geng 2018-05-31 11:01 ` Mark Rutland 2018-05-31 11:01 ` Mark Rutland 2018-05-31 16:51 ` James Morse 2018-05-31 16:51 ` James Morse 2018-06-01 7:21 ` gengdongjiu 2018-06-01 7:21 ` gengdongjiu 2018-06-01 7:21 ` gengdongjiu 2018-06-15 16:49 ` James Morse 2018-06-15 16:49 ` James Morse
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.