* stop messing with set_fs in arm_sdei @ 2020-04-14 14:23 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-kernel, linux-arm-kernel Hi James, can you take a look at this series? I've been trying to figure out what the set_fs in arm_sdei is good for, and could not find any good reason. But I don't have any hardware implementing this interface, so the changes are entirely untested. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* stop messing with set_fs in arm_sdei @ 2020-04-14 14:23 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel Hi James, can you take a look at this series? I've been trying to figure out what the set_fs in arm_sdei is good for, and could not find any good reason. But I don't have any hardware implementing this interface, so the changes are entirely untested. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] firmware: arm_sdei: remove unused interfaces 2020-04-14 14:23 ` Christoph Hellwig @ 2020-04-14 14:23 ` Christoph Hellwig -1 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-kernel, linux-arm-kernel The export symbols to register/unregister and enable/disable events aren't ever used outside of arm_sdei.c, so mark them static. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/firmware/arm_sdei.c | 13 +++++-------- include/linux/arm_sdei.h | 15 --------------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index 334c8be0c11f..bdd6461647d7 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -400,7 +400,7 @@ static void _local_event_enable(void *data) sdei_cross_call_return(arg, err); } -int sdei_event_enable(u32 event_num) +static int sdei_event_enable(u32 event_num) { int err = -EINVAL; struct sdei_event *event; @@ -429,7 +429,6 @@ int sdei_event_enable(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_enable); static int sdei_api_event_disable(u32 event_num) { @@ -447,7 +446,7 @@ static void _ipi_event_disable(void *data) sdei_cross_call_return(arg, err); } -int sdei_event_disable(u32 event_num) +static int sdei_event_disable(u32 event_num) { int err = -EINVAL; struct sdei_event *event; @@ -471,7 +470,6 @@ int sdei_event_disable(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_disable); static int sdei_api_event_unregister(u32 event_num) { @@ -502,7 +500,7 @@ static int _sdei_event_unregister(struct sdei_event *event) return sdei_do_cross_call(_local_event_unregister, event); } -int sdei_event_unregister(u32 event_num) +static int sdei_event_unregister(u32 event_num) { int err; struct sdei_event *event; @@ -533,7 +531,6 @@ int sdei_event_unregister(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_unregister); /* * unregister events, but don't destroy them as they are re-registered by @@ -603,7 +600,8 @@ static int _sdei_event_register(struct sdei_event *event) return err; } -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) +static int sdei_event_register(u32 event_num, sdei_event_callback *cb, + void *arg) { int err; struct sdei_event *event; @@ -643,7 +641,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) return err; } -EXPORT_SYMBOL(sdei_event_register); static int sdei_reregister_event_llocked(struct sdei_event *event) { diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h index 0a241c5c911d..5f9fb1d95d51 100644 --- a/include/linux/arm_sdei.h +++ b/include/linux/arm_sdei.h @@ -22,21 +22,6 @@ */ typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg); -/* - * Register your callback to claim an event. The event must be described - * by firmware. - */ -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg); - -/* - * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling - * it until it succeeds. - */ -int sdei_event_unregister(u32 event_num); - -int sdei_event_enable(u32 event_num); -int sdei_event_disable(u32 event_num); - /* GHES register/unregister helpers */ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb, sdei_event_callback *critical_cb); -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/2] firmware: arm_sdei: remove unused interfaces @ 2020-04-14 14:23 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel The export symbols to register/unregister and enable/disable events aren't ever used outside of arm_sdei.c, so mark them static. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/firmware/arm_sdei.c | 13 +++++-------- include/linux/arm_sdei.h | 15 --------------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index 334c8be0c11f..bdd6461647d7 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -400,7 +400,7 @@ static void _local_event_enable(void *data) sdei_cross_call_return(arg, err); } -int sdei_event_enable(u32 event_num) +static int sdei_event_enable(u32 event_num) { int err = -EINVAL; struct sdei_event *event; @@ -429,7 +429,6 @@ int sdei_event_enable(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_enable); static int sdei_api_event_disable(u32 event_num) { @@ -447,7 +446,7 @@ static void _ipi_event_disable(void *data) sdei_cross_call_return(arg, err); } -int sdei_event_disable(u32 event_num) +static int sdei_event_disable(u32 event_num) { int err = -EINVAL; struct sdei_event *event; @@ -471,7 +470,6 @@ int sdei_event_disable(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_disable); static int sdei_api_event_unregister(u32 event_num) { @@ -502,7 +500,7 @@ static int _sdei_event_unregister(struct sdei_event *event) return sdei_do_cross_call(_local_event_unregister, event); } -int sdei_event_unregister(u32 event_num) +static int sdei_event_unregister(u32 event_num) { int err; struct sdei_event *event; @@ -533,7 +531,6 @@ int sdei_event_unregister(u32 event_num) return err; } -EXPORT_SYMBOL(sdei_event_unregister); /* * unregister events, but don't destroy them as they are re-registered by @@ -603,7 +600,8 @@ static int _sdei_event_register(struct sdei_event *event) return err; } -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) +static int sdei_event_register(u32 event_num, sdei_event_callback *cb, + void *arg) { int err; struct sdei_event *event; @@ -643,7 +641,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) return err; } -EXPORT_SYMBOL(sdei_event_register); static int sdei_reregister_event_llocked(struct sdei_event *event) { diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h index 0a241c5c911d..5f9fb1d95d51 100644 --- a/include/linux/arm_sdei.h +++ b/include/linux/arm_sdei.h @@ -22,21 +22,6 @@ */ typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg); -/* - * Register your callback to claim an event. The event must be described - * by firmware. - */ -int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg); - -/* - * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling - * it until it succeeds. - */ -int sdei_event_unregister(u32 event_num); - -int sdei_event_enable(u32 event_num); -int sdei_event_disable(u32 event_num); - /* GHES register/unregister helpers */ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb, sdei_event_callback *critical_cb); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] arm_sdei: remove the set_fs calls in sdei_event_handler 2020-04-14 14:23 ` Christoph Hellwig @ 2020-04-14 14:23 ` Christoph Hellwig -1 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-kernel, linux-arm-kernel There are only two callbacks that can be called, which both eventually end up calling __ghes_sdei_callback. __ghes_sdei_callback calls irq_work_queue which is a normal kernel helper called from all kinds of contexts and ghes_in_nmi_queue_one_entry. ghes_in_nmi_queue_one_entry is also called from other code without messing with the address limit, so it better work without it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/firmware/arm_sdei.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index bdd6461647d7..1c51b378dfca 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -1137,19 +1137,12 @@ int sdei_event_handler(struct pt_regs *regs, struct sdei_registered_event *arg) { int err; - mm_segment_t orig_addr_limit; u32 event_num = arg->event_num; - orig_addr_limit = get_fs(); - set_fs(USER_DS); - err = arg->callback(event_num, regs, arg->callback_arg); if (err) pr_err_ratelimited("event %u on CPU %u failed with error: %d\n", event_num, smp_processor_id(), err); - - set_fs(orig_addr_limit); - return err; } NOKPROBE_SYMBOL(sdei_event_handler); -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] arm_sdei: remove the set_fs calls in sdei_event_handler @ 2020-04-14 14:23 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-14 14:23 UTC (permalink / raw) To: James Morse; +Cc: linux-arm-kernel, linux-kernel There are only two callbacks that can be called, which both eventually end up calling __ghes_sdei_callback. __ghes_sdei_callback calls irq_work_queue which is a normal kernel helper called from all kinds of contexts and ghes_in_nmi_queue_one_entry. ghes_in_nmi_queue_one_entry is also called from other code without messing with the address limit, so it better work without it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/firmware/arm_sdei.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index bdd6461647d7..1c51b378dfca 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -1137,19 +1137,12 @@ int sdei_event_handler(struct pt_regs *regs, struct sdei_registered_event *arg) { int err; - mm_segment_t orig_addr_limit; u32 event_num = arg->event_num; - orig_addr_limit = get_fs(); - set_fs(USER_DS); - err = arg->callback(event_num, regs, arg->callback_arg); if (err) pr_err_ratelimited("event %u on CPU %u failed with error: %d\n", event_num, smp_processor_id(), err); - - set_fs(orig_addr_limit); - return err; } NOKPROBE_SYMBOL(sdei_event_handler); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: stop messing with set_fs in arm_sdei 2020-04-14 14:23 ` Christoph Hellwig @ 2020-04-14 15:59 ` James Morse -1 siblings, 0 replies; 10+ messages in thread From: James Morse @ 2020-04-14 15:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, linux-arm-kernel Hi Christoph, On 14/04/2020 15:23, Christoph Hellwig wrote: > can you take a look at this series? I've been trying to figure out > what the set_fs in arm_sdei is good for, and could not find any > good reason. But I don't have any hardware implementing this interface, > so the changes are entirely untested. Its a firmware thing, think of it as a firmware assisted software NMI. The arch code save/restores set_fs() because the entry code does that when taking an exception from EL1. SDEI does the same because it doesn't come via the same entry code. It does it in C because that C is always run before the handler, something that isn't true for the regular assembly version. The regular entry code does this because any exception may have interrupted code that had addr_limit set to something else: https://bugs.chromium.org/p/project-zero/issues/detail?id=822 and the patch that fixed it: commit e19a6ee2460b "arm64: kernel: Save and restore UAO and addr_limit on exception entry" Thanks, James _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: stop messing with set_fs in arm_sdei @ 2020-04-14 15:59 ` James Morse 0 siblings, 0 replies; 10+ messages in thread From: James Morse @ 2020-04-14 15:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-arm-kernel, linux-kernel Hi Christoph, On 14/04/2020 15:23, Christoph Hellwig wrote: > can you take a look at this series? I've been trying to figure out > what the set_fs in arm_sdei is good for, and could not find any > good reason. But I don't have any hardware implementing this interface, > so the changes are entirely untested. Its a firmware thing, think of it as a firmware assisted software NMI. The arch code save/restores set_fs() because the entry code does that when taking an exception from EL1. SDEI does the same because it doesn't come via the same entry code. It does it in C because that C is always run before the handler, something that isn't true for the regular assembly version. The regular entry code does this because any exception may have interrupted code that had addr_limit set to something else: https://bugs.chromium.org/p/project-zero/issues/detail?id=822 and the patch that fixed it: commit e19a6ee2460b "arm64: kernel: Save and restore UAO and addr_limit on exception entry" Thanks, James ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: stop messing with set_fs in arm_sdei 2020-04-14 15:59 ` James Morse @ 2020-04-20 10:22 ` Christoph Hellwig -1 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-20 10:22 UTC (permalink / raw) To: James Morse; +Cc: Christoph Hellwig, linux-arm-kernel, linux-kernel On Tue, Apr 14, 2020 at 04:59:16PM +0100, James Morse wrote: > Hi Christoph, > > On 14/04/2020 15:23, Christoph Hellwig wrote: > > can you take a look at this series? I've been trying to figure out > > what the set_fs in arm_sdei is good for, and could not find any > > good reason. But I don't have any hardware implementing this interface, > > so the changes are entirely untested. > > Its a firmware thing, think of it as a firmware assisted software NMI. > > The arch code save/restores set_fs() because the entry code does that when taking an > exception from EL1. SDEI does the same because it doesn't come via the same entry code. It > does it in C because that C is always run before the handler, something that isn't true > for the regular assembly version. > > The regular entry code does this because any exception may have interrupted code that had > addr_limit set to something else: > https://bugs.chromium.org/p/project-zero/issues/detail?id=822 > > and the patch that fixed it: commit e19a6ee2460b "arm64: kernel: Save and restore UAO and > addr_limit on exception entry" Can you throw in a comment documenting this better? And pick up the first patch while we're at it - no need to expose such low-level mechanisms to modules. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: stop messing with set_fs in arm_sdei @ 2020-04-20 10:22 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-04-20 10:22 UTC (permalink / raw) To: James Morse; +Cc: Christoph Hellwig, linux-arm-kernel, linux-kernel On Tue, Apr 14, 2020 at 04:59:16PM +0100, James Morse wrote: > Hi Christoph, > > On 14/04/2020 15:23, Christoph Hellwig wrote: > > can you take a look at this series? I've been trying to figure out > > what the set_fs in arm_sdei is good for, and could not find any > > good reason. But I don't have any hardware implementing this interface, > > so the changes are entirely untested. > > Its a firmware thing, think of it as a firmware assisted software NMI. > > The arch code save/restores set_fs() because the entry code does that when taking an > exception from EL1. SDEI does the same because it doesn't come via the same entry code. It > does it in C because that C is always run before the handler, something that isn't true > for the regular assembly version. > > The regular entry code does this because any exception may have interrupted code that had > addr_limit set to something else: > https://bugs.chromium.org/p/project-zero/issues/detail?id=822 > > and the patch that fixed it: commit e19a6ee2460b "arm64: kernel: Save and restore UAO and > addr_limit on exception entry" Can you throw in a comment documenting this better? And pick up the first patch while we're at it - no need to expose such low-level mechanisms to modules. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-20 10:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-14 14:23 stop messing with set_fs in arm_sdei Christoph Hellwig 2020-04-14 14:23 ` Christoph Hellwig 2020-04-14 14:23 ` [PATCH 1/2] firmware: arm_sdei: remove unused interfaces Christoph Hellwig 2020-04-14 14:23 ` Christoph Hellwig 2020-04-14 14:23 ` [PATCH 2/2] arm_sdei: remove the set_fs calls in sdei_event_handler Christoph Hellwig 2020-04-14 14:23 ` Christoph Hellwig 2020-04-14 15:59 ` stop messing with set_fs in arm_sdei James Morse 2020-04-14 15:59 ` James Morse 2020-04-20 10:22 ` Christoph Hellwig 2020-04-20 10:22 ` Christoph Hellwig
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.