* [Patch v2 0/4] Protect against concurrent calls into UV BIOS
@ 2019-02-07 4:22 Hedi Berriche
2019-02-07 4:22 ` [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock Hedi Berriche
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Hedi Berriche @ 2019-02-07 4:22 UTC (permalink / raw)
To: linux-kernel, Ard Biesheuvel, Thomas Gleixner, Bhupesh Sharma
Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, x86,
Hedi Berriche, Russ Anderson, Mike Travis, Dimitri Sivanich,
Steve Wahl, stable
Changes since v1:
Addressed comments from Bhupesh Sharma, Thomas Gleixner, and Ard Biesheuvel:
* made __uv_bios_call() static
* moved the efi_enabled() cleanup to its own patchlet
* explained the reason for renaming the efi_runtime_lock semaphore
* dropped the reviewed-bys as they should be given on the mailing list
* Cc'ng stable@vger.kernel.org given the nature of the problem addressed by the patches
---
Calls into UV BIOS were not being serialised which is wrong as it violates EFI
runtime rules, and bad as it does result in all sorts of potentially hard to
track down hangs and panics when efi_scratch.prev_mm gets clobbered whenever
efi_switch_mm() gets called concurrently from two different CPUs.
Patch #1 makes the efi_runtime_lock semaphore visible to EFI runtime callers
defined outside drivers/firmware/efi/runtime-wrappers.c in preparation for
using it to serialise calls into UV BIOS; the lock is also renamed to
efi_runtime_sem so that it can coexist with the efi_runtime_lock spinlock
defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is enabled.
Patch #2 removes uv_bios_call_reentrant() because it's dead code.
Patch #3 is a cleanup that drops test_bit() in favour of the ad hoc efi_enabled().
Patch #4 makes uv_bios_call() variants use efi_runtime_sem to protect against
concurrency.
Cc: Russ Anderson <rja@hpe.com>
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Dimitri Sivanich <sivanich@hpe.com>
Cc: Steve Wahl <steve.wahl@hpe.com>
Cc: stable@vger.kernel.org
Hedi Berriche (4):
efi/x86: turn EFI runtime semaphore into a global lock
x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers
x86/platform/UV: use efi_enabled() instead of test_bit()
x86/platform/UV: use efi_runtime_sem to serialise BIOS calls
arch/x86/include/asm/uv/bios.h | 4 +-
arch/x86/platform/uv/bios_uv.c | 33 ++++++++------
drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++-------------
include/linux/efi.h | 3 ++
4 files changed, 55 insertions(+), 45 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-07 4:22 [Patch v2 0/4] Protect against concurrent calls into UV BIOS Hedi Berriche @ 2019-02-07 4:22 ` Hedi Berriche 2019-02-07 16:05 ` Ard Biesheuvel 2019-02-07 4:22 ` [Patch v2 2/4] x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers Hedi Berriche ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Hedi Berriche @ 2019-02-07 4:22 UTC (permalink / raw) To: linux-kernel, Ard Biesheuvel, Thomas Gleixner, Bhupesh Sharma Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, x86, Hedi Berriche, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable Make efi_runtime_lock semaphore global so that it can be used by EFI runtime callers that may be defined outside efi/runtime-wrappers.c. Also now that efi_runtime_lock semaphore is no longer static, rename it to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is enabled. The immediate motivation of this change is to serialise UV platform BIOS calls using the efi_runtime_sem semaphore. No functional changes. Cc: Russ Anderson <rja@hpe.com> Cc: Mike Travis <mike.travis@hpe.com> Cc: Dimitri Sivanich <sivanich@hpe.com> Cc: Steve Wahl <steve.wahl@hpe.com> Cc: stable@vger.kernel.org Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> --- drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- include/linux/efi.h | 3 ++ 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 8903b9ccfc2b..ec60d6227925 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; * @rts_arg<1-5>: efi_runtime_service() function arguments * * Accesses to efi_runtime_services() are serialized by a binary - * semaphore (efi_runtime_lock) and caller waits until the work is + * semaphore (efi_runtime_sem) and caller waits until the work is * finished, hence _only_ one work is queued at a time and the caller * thread waits for completion. */ @@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) * none of the remaining functions are actually ever called at runtime. * So let's just use a single lock to serialize all Runtime Services calls. */ -static DEFINE_SEMAPHORE(efi_runtime_lock); +DEFINE_SEMAPHORE(efi_runtime_sem); /* * Calls the appropriate efi_runtime_service() with the appropriate @@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, data); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, data); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, { efi_status_t status; - if (down_trylock(&efi_runtime_lock)) + if (down_trylock(&efi_runtime_sem)) return EFI_NOT_READY; status = efi_call_virt(set_variable, name, vendor, attr, data_size, data); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, remaining_space, max_variable_size, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - if (down_trylock(&efi_runtime_lock)) + if (down_trylock(&efi_runtime_sem)) return EFI_NOT_READY; status = efi_call_virt(query_variable_info, attr, storage_space, remaining_space, max_variable_size); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) { efi_status_t status; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, unsigned long data_size, efi_char16_t *data) { - if (down_interruptible(&efi_runtime_lock)) { + if (down_interruptible(&efi_runtime_sem)) { pr_warn("failed to invoke the reset_system() runtime service:\n" "could not get exclusive access to the firmware\n"); return; } efi_rts_work.efi_rts_id = RESET_SYSTEM; __efi_call_virt(reset_system, reset_type, status, data_size, data); - up(&efi_runtime_lock); + up(&efi_runtime_sem); } static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, @@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, NULL, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } @@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - if (down_interruptible(&efi_runtime_lock)) + if (down_interruptible(&efi_runtime_sem)) return EFI_ABORTED; status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, max_size, reset_type, NULL); - up(&efi_runtime_lock); + up(&efi_runtime_sem); return status; } diff --git a/include/linux/efi.h b/include/linux/efi.h index 45ff763fba76..930cd20842b8 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; /* Workqueue to queue EFI Runtime Services */ extern struct workqueue_struct *efi_rts_wq; +/* EFI runtime semaphore */ +extern struct semaphore efi_runtime_sem; + struct linux_efi_memreserve { int size; // allocated size of the array atomic_t count; // number of entries used -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-07 4:22 ` [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock Hedi Berriche @ 2019-02-07 16:05 ` Ard Biesheuvel 2019-02-07 17:38 ` Hedi Berriche 0 siblings, 1 reply; 11+ messages in thread From: Ard Biesheuvel @ 2019-02-07 16:05 UTC (permalink / raw) To: Hedi Berriche Cc: Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: > > Make efi_runtime_lock semaphore global so that it can be used by EFI > runtime callers that may be defined outside efi/runtime-wrappers.c. > > Also now that efi_runtime_lock semaphore is no longer static, rename it > to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock > defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is > enabled. > > The immediate motivation of this change is to serialise UV platform BIOS > calls using the efi_runtime_sem semaphore. > > No functional changes. > > Cc: Russ Anderson <rja@hpe.com> > Cc: Mike Travis <mike.travis@hpe.com> > Cc: Dimitri Sivanich <sivanich@hpe.com> > Cc: Steve Wahl <steve.wahl@hpe.com> > Cc: stable@vger.kernel.org > Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> Hello Hedi, Same feedback as on v1: please don't rename the lock. > --- > drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- > include/linux/efi.h | 3 ++ > 2 files changed, 33 insertions(+), 30 deletions(-) > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > index 8903b9ccfc2b..ec60d6227925 100644 > --- a/drivers/firmware/efi/runtime-wrappers.c > +++ b/drivers/firmware/efi/runtime-wrappers.c > @@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; > * @rts_arg<1-5>: efi_runtime_service() function arguments > * > * Accesses to efi_runtime_services() are serialized by a binary > - * semaphore (efi_runtime_lock) and caller waits until the work is > + * semaphore (efi_runtime_sem) and caller waits until the work is > * finished, hence _only_ one work is queued at a time and the caller > * thread waits for completion. > */ > @@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > * none of the remaining functions are actually ever called at runtime. > * So let's just use a single lock to serialize all Runtime Services calls. > */ > -static DEFINE_SEMAPHORE(efi_runtime_lock); > +DEFINE_SEMAPHORE(efi_runtime_sem); > > /* > * Calls the appropriate efi_runtime_service() with the appropriate > @@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, > NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, > NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, > data); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, > NULL, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, > data); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, > { > efi_status_t status; > > - if (down_trylock(&efi_runtime_lock)) > + if (down_trylock(&efi_runtime_sem)) > return EFI_NOT_READY; > > status = efi_call_virt(set_variable, name, vendor, attr, data_size, > data); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, > remaining_space, max_variable_size, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - if (down_trylock(&efi_runtime_lock)) > + if (down_trylock(&efi_runtime_sem)) > return EFI_NOT_READY; > > status = efi_call_virt(query_variable_info, attr, storage_space, > remaining_space, max_variable_size); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) > { > efi_status_t status; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, > NULL, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, > unsigned long data_size, > efi_char16_t *data) > { > - if (down_interruptible(&efi_runtime_lock)) { > + if (down_interruptible(&efi_runtime_sem)) { > pr_warn("failed to invoke the reset_system() runtime service:\n" > "could not get exclusive access to the firmware\n"); > return; > } > efi_rts_work.efi_rts_id = RESET_SYSTEM; > __efi_call_virt(reset_system, reset_type, status, data_size, data); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > } > > static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > @@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, > NULL, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > @@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - if (down_interruptible(&efi_runtime_lock)) > + if (down_interruptible(&efi_runtime_sem)) > return EFI_ABORTED; > status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, > max_size, reset_type, NULL); > - up(&efi_runtime_lock); > + up(&efi_runtime_sem); > return status; > } > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 45ff763fba76..930cd20842b8 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; > /* Workqueue to queue EFI Runtime Services */ > extern struct workqueue_struct *efi_rts_wq; > > +/* EFI runtime semaphore */ > +extern struct semaphore efi_runtime_sem; > + > struct linux_efi_memreserve { > int size; // allocated size of the array > atomic_t count; // number of entries used > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-07 16:05 ` Ard Biesheuvel @ 2019-02-07 17:38 ` Hedi Berriche 2019-02-07 19:38 ` Ard Biesheuvel 2019-02-12 17:25 ` Hedi Berriche 0 siblings, 2 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-07 17:38 UTC (permalink / raw) To: Ard Biesheuvel Cc: Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Thu, Feb 07, 2019 at 16:05 Ard Biesheuvel wrote: >On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: >> >> Make efi_runtime_lock semaphore global so that it can be used by EFI >> runtime callers that may be defined outside efi/runtime-wrappers.c. >> >> Also now that efi_runtime_lock semaphore is no longer static, rename it >> to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock >> defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is >> enabled. >> >> The immediate motivation of this change is to serialise UV platform BIOS >> calls using the efi_runtime_sem semaphore. >> >> No functional changes. >> >> Cc: Russ Anderson <rja@hpe.com> >> Cc: Mike Travis <mike.travis@hpe.com> >> Cc: Dimitri Sivanich <sivanich@hpe.com> >> Cc: Steve Wahl <steve.wahl@hpe.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> > >Hello Hedi, Hi Ard, >Same feedback as on v1: please don't rename the lock. With the efi_runtime_lock semaphore being no longer static, not renaming it will cause a compile failure as it will clash with the declaration of the efi_runtime_lock *spinlock* in arch/x86/platform/efi/efi_64.c in the CONFIG_EFI_MIXED case. >> --- >> drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- >> include/linux/efi.h | 3 ++ >> 2 files changed, 33 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >> index 8903b9ccfc2b..ec60d6227925 100644 >> --- a/drivers/firmware/efi/runtime-wrappers.c >> +++ b/drivers/firmware/efi/runtime-wrappers.c >> @@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; >> * @rts_arg<1-5>: efi_runtime_service() function arguments >> * >> * Accesses to efi_runtime_services() are serialized by a binary >> - * semaphore (efi_runtime_lock) and caller waits until the work is >> + * semaphore (efi_runtime_sem) and caller waits until the work is >> * finished, hence _only_ one work is queued at a time and the caller >> * thread waits for completion. >> */ >> @@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >> * none of the remaining functions are actually ever called at runtime. >> * So let's just use a single lock to serialize all Runtime Services calls. >> */ >> -static DEFINE_SEMAPHORE(efi_runtime_lock); >> +DEFINE_SEMAPHORE(efi_runtime_sem); >> >> /* >> * Calls the appropriate efi_runtime_service() with the appropriate >> @@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, >> NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, >> NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, >> data); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, >> NULL, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, >> data); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, >> { >> efi_status_t status; >> >> - if (down_trylock(&efi_runtime_lock)) >> + if (down_trylock(&efi_runtime_sem)) >> return EFI_NOT_READY; >> >> status = efi_call_virt(set_variable, name, vendor, attr, data_size, >> data); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, >> remaining_space, max_variable_size, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - if (down_trylock(&efi_runtime_lock)) >> + if (down_trylock(&efi_runtime_sem)) >> return EFI_NOT_READY; >> >> status = efi_call_virt(query_variable_info, attr, storage_space, >> remaining_space, max_variable_size); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) >> { >> efi_status_t status; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, >> NULL, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, >> unsigned long data_size, >> efi_char16_t *data) >> { >> - if (down_interruptible(&efi_runtime_lock)) { >> + if (down_interruptible(&efi_runtime_sem)) { >> pr_warn("failed to invoke the reset_system() runtime service:\n" >> "could not get exclusive access to the firmware\n"); >> return; >> } >> efi_rts_work.efi_rts_id = RESET_SYSTEM; >> __efi_call_virt(reset_system, reset_type, status, data_size, data); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> } >> >> static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >> @@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, >> NULL, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> @@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - if (down_interruptible(&efi_runtime_lock)) >> + if (down_interruptible(&efi_runtime_sem)) >> return EFI_ABORTED; >> status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, >> max_size, reset_type, NULL); >> - up(&efi_runtime_lock); >> + up(&efi_runtime_sem); >> return status; >> } >> >> diff --git a/include/linux/efi.h b/include/linux/efi.h >> index 45ff763fba76..930cd20842b8 100644 >> --- a/include/linux/efi.h >> +++ b/include/linux/efi.h >> @@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; >> /* Workqueue to queue EFI Runtime Services */ >> extern struct workqueue_struct *efi_rts_wq; >> >> +/* EFI runtime semaphore */ >> +extern struct semaphore efi_runtime_sem; >> + >> struct linux_efi_memreserve { >> int size; // allocated size of the array >> atomic_t count; // number of entries used >> -- >> 2.20.1 >> -- Be careful of reading health books, you might die of a misprint. -- Mark Twain ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-07 17:38 ` Hedi Berriche @ 2019-02-07 19:38 ` Ard Biesheuvel 2019-02-12 17:25 ` Hedi Berriche 1 sibling, 0 replies; 11+ messages in thread From: Ard Biesheuvel @ 2019-02-07 19:38 UTC (permalink / raw) To: Ard Biesheuvel, Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Thu, 7 Feb 2019 at 18:38, Hedi Berriche <hedi.berriche@hpe.com> wrote: > > On Thu, Feb 07, 2019 at 16:05 Ard Biesheuvel wrote: > >On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: > >> > >> Make efi_runtime_lock semaphore global so that it can be used by EFI > >> runtime callers that may be defined outside efi/runtime-wrappers.c. > >> > >> Also now that efi_runtime_lock semaphore is no longer static, rename it > >> to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock > >> defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is > >> enabled. > >> > >> The immediate motivation of this change is to serialise UV platform BIOS > >> calls using the efi_runtime_sem semaphore. > >> > >> No functional changes. > >> > >> Cc: Russ Anderson <rja@hpe.com> > >> Cc: Mike Travis <mike.travis@hpe.com> > >> Cc: Dimitri Sivanich <sivanich@hpe.com> > >> Cc: Steve Wahl <steve.wahl@hpe.com> > >> Cc: stable@vger.kernel.org > >> Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> > > > >Hello Hedi, > > Hi Ard, > > >Same feedback as on v1: please don't rename the lock. > > With the efi_runtime_lock semaphore being no longer static, not renaming it > will cause a compile failure as it will clash with the declaration of the > efi_runtime_lock *spinlock* in arch/x86/platform/efi/efi_64.c in the > CONFIG_EFI_MIXED case. > Ah right, it's in the commit log, as requested. Apologies for not spotting that. Given how UV is a bit of an outlier, I'd prefer it if we could make this functionality a bit more light weight, by only exposing the lock if needed, and not create a global symbol for an internal lock if we can avoid it. Could you please try something along the lines of --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -146,6 +146,10 @@ */ static DEFINE_SEMAPHORE(efi_runtime_lock); +#ifdef CONFIG_X86_UV +extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock); +#endif + /* * Calls the appropriate efi_runtime_service() with the appropriate * arguments. and add a declaration extern struct semaphore __efi_uv_runtime_lock; to bios_uv.c itself rather than to a header file? You can combine this with your changes in patch #4 (and drop the first patch entirely) Also, I noticed that there is some CONFIG_EFI guarded code in bios_uv.c while the whole file already depends on CONFIG_EFI via the Kconfig symbol. Perhaps you could include a patch to clean that up as well? Thanks. > >> --- > >> drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- > >> include/linux/efi.h | 3 ++ > >> 2 files changed, 33 insertions(+), 30 deletions(-) > >> > >> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > >> index 8903b9ccfc2b..ec60d6227925 100644 > >> --- a/drivers/firmware/efi/runtime-wrappers.c > >> +++ b/drivers/firmware/efi/runtime-wrappers.c > >> @@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; > >> * @rts_arg<1-5>: efi_runtime_service() function arguments > >> * > >> * Accesses to efi_runtime_services() are serialized by a binary > >> - * semaphore (efi_runtime_lock) and caller waits until the work is > >> + * semaphore (efi_runtime_sem) and caller waits until the work is > >> * finished, hence _only_ one work is queued at a time and the caller > >> * thread waits for completion. > >> */ > >> @@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > >> * none of the remaining functions are actually ever called at runtime. > >> * So let's just use a single lock to serialize all Runtime Services calls. > >> */ > >> -static DEFINE_SEMAPHORE(efi_runtime_lock); > >> +DEFINE_SEMAPHORE(efi_runtime_sem); > >> > >> /* > >> * Calls the appropriate efi_runtime_service() with the appropriate > >> @@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, > >> NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, > >> NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, > >> data); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, > >> NULL, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, > >> data); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, > >> { > >> efi_status_t status; > >> > >> - if (down_trylock(&efi_runtime_lock)) > >> + if (down_trylock(&efi_runtime_sem)) > >> return EFI_NOT_READY; > >> > >> status = efi_call_virt(set_variable, name, vendor, attr, data_size, > >> data); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, > >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >> return EFI_UNSUPPORTED; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, > >> remaining_space, max_variable_size, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, > >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >> return EFI_UNSUPPORTED; > >> > >> - if (down_trylock(&efi_runtime_lock)) > >> + if (down_trylock(&efi_runtime_sem)) > >> return EFI_NOT_READY; > >> > >> status = efi_call_virt(query_variable_info, attr, storage_space, > >> remaining_space, max_variable_size); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) > >> { > >> efi_status_t status; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, > >> NULL, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, > >> unsigned long data_size, > >> efi_char16_t *data) > >> { > >> - if (down_interruptible(&efi_runtime_lock)) { > >> + if (down_interruptible(&efi_runtime_sem)) { > >> pr_warn("failed to invoke the reset_system() runtime service:\n" > >> "could not get exclusive access to the firmware\n"); > >> return; > >> } > >> efi_rts_work.efi_rts_id = RESET_SYSTEM; > >> __efi_call_virt(reset_system, reset_type, status, data_size, data); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> } > >> > >> static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > >> @@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >> return EFI_UNSUPPORTED; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, > >> NULL, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> @@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >> return EFI_UNSUPPORTED; > >> > >> - if (down_interruptible(&efi_runtime_lock)) > >> + if (down_interruptible(&efi_runtime_sem)) > >> return EFI_ABORTED; > >> status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, > >> max_size, reset_type, NULL); > >> - up(&efi_runtime_lock); > >> + up(&efi_runtime_sem); > >> return status; > >> } > >> > >> diff --git a/include/linux/efi.h b/include/linux/efi.h > >> index 45ff763fba76..930cd20842b8 100644 > >> --- a/include/linux/efi.h > >> +++ b/include/linux/efi.h > >> @@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; > >> /* Workqueue to queue EFI Runtime Services */ > >> extern struct workqueue_struct *efi_rts_wq; > >> > >> +/* EFI runtime semaphore */ > >> +extern struct semaphore efi_runtime_sem; > >> + > >> struct linux_efi_memreserve { > >> int size; // allocated size of the array > >> atomic_t count; // number of entries used > >> -- > >> 2.20.1 > >> > > -- > Be careful of reading health books, you might die of a misprint. > -- Mark Twain ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-07 17:38 ` Hedi Berriche 2019-02-07 19:38 ` Ard Biesheuvel @ 2019-02-12 17:25 ` Hedi Berriche 2019-02-12 17:26 ` Ard Biesheuvel 2019-02-12 23:46 ` Hedi Berriche 1 sibling, 2 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-12 17:25 UTC (permalink / raw) To: Ard Biesheuvel Cc: Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Thu, Feb 07, 2019 at 17:38 Hedi Berriche wrote: >On Thu, Feb 07, 2019 at 16:05 Ard Biesheuvel wrote: >>On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: >>> >>>Make efi_runtime_lock semaphore global so that it can be used by EFI >>>runtime callers that may be defined outside efi/runtime-wrappers.c. >>> >>>Also now that efi_runtime_lock semaphore is no longer static, rename it >>>to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock >>>defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is >>>enabled. >>> >>>The immediate motivation of this change is to serialise UV platform BIOS >>>calls using the efi_runtime_sem semaphore. >>> >>>No functional changes. >>> >>>Cc: Russ Anderson <rja@hpe.com> >>>Cc: Mike Travis <mike.travis@hpe.com> >>>Cc: Dimitri Sivanich <sivanich@hpe.com> >>>Cc: Steve Wahl <steve.wahl@hpe.com> >>>Cc: stable@vger.kernel.org >>>Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> >> >>Hello Hedi, > >Hi Ard, > >>Same feedback as on v1: please don't rename the lock. > >With the efi_runtime_lock semaphore being no longer static, not renaming it >will cause a compile failure as it will clash with the declaration of the >efi_runtime_lock *spinlock* in arch/x86/platform/efi/efi_64.c in the >CONFIG_EFI_MIXED case. Ard, Any comments on whether resolving the conflict between {efi_runtime_lock, drivers/firmware/efi/runtime-wrappers.c} and {efi_runtime_lock, arch/x86/platform/efi/efi_64.c} provides the required justification for renaming the efi_runtime_lock semaphore? Cheers, Hedi. >>>--- >>> drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- >>> include/linux/efi.h | 3 ++ >>> 2 files changed, 33 insertions(+), 30 deletions(-) >>> >>>diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >>>index 8903b9ccfc2b..ec60d6227925 100644 >>>--- a/drivers/firmware/efi/runtime-wrappers.c >>>+++ b/drivers/firmware/efi/runtime-wrappers.c >>>@@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; >>> * @rts_arg<1-5>: efi_runtime_service() function arguments >>> * >>> * Accesses to efi_runtime_services() are serialized by a binary >>>- * semaphore (efi_runtime_lock) and caller waits until the work is >>>+ * semaphore (efi_runtime_sem) and caller waits until the work is >>> * finished, hence _only_ one work is queued at a time and the caller >>> * thread waits for completion. >>> */ >>>@@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >>> * none of the remaining functions are actually ever called at runtime. >>> * So let's just use a single lock to serialize all Runtime Services calls. >>> */ >>>-static DEFINE_SEMAPHORE(efi_runtime_lock); >>>+DEFINE_SEMAPHORE(efi_runtime_sem); >>> >>> /* >>> * Calls the appropriate efi_runtime_service() with the appropriate >>>@@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, >>> NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, >>> NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, >>> data); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, >>> NULL, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, >>> data); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, >>> { >>> efi_status_t status; >>> >>>- if (down_trylock(&efi_runtime_lock)) >>>+ if (down_trylock(&efi_runtime_sem)) >>> return EFI_NOT_READY; >>> >>> status = efi_call_virt(set_variable, name, vendor, attr, data_size, >>> data); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>> return EFI_UNSUPPORTED; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, >>> remaining_space, max_variable_size, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>> return EFI_UNSUPPORTED; >>> >>>- if (down_trylock(&efi_runtime_lock)) >>>+ if (down_trylock(&efi_runtime_sem)) >>> return EFI_NOT_READY; >>> >>> status = efi_call_virt(query_variable_info, attr, storage_space, >>> remaining_space, max_variable_size); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) >>> { >>> efi_status_t status; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, >>> NULL, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, >>> unsigned long data_size, >>> efi_char16_t *data) >>> { >>>- if (down_interruptible(&efi_runtime_lock)) { >>>+ if (down_interruptible(&efi_runtime_sem)) { >>> pr_warn("failed to invoke the reset_system() runtime service:\n" >>> "could not get exclusive access to the firmware\n"); >>> return; >>> } >>> efi_rts_work.efi_rts_id = RESET_SYSTEM; >>> __efi_call_virt(reset_system, reset_type, status, data_size, data); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> } >>> >>> static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >>>@@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>> return EFI_UNSUPPORTED; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, >>> NULL, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>@@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>> return EFI_UNSUPPORTED; >>> >>>- if (down_interruptible(&efi_runtime_lock)) >>>+ if (down_interruptible(&efi_runtime_sem)) >>> return EFI_ABORTED; >>> status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, >>> max_size, reset_type, NULL); >>>- up(&efi_runtime_lock); >>>+ up(&efi_runtime_sem); >>> return status; >>> } >>> >>>diff --git a/include/linux/efi.h b/include/linux/efi.h >>>index 45ff763fba76..930cd20842b8 100644 >>>--- a/include/linux/efi.h >>>+++ b/include/linux/efi.h >>>@@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; >>> /* Workqueue to queue EFI Runtime Services */ >>> extern struct workqueue_struct *efi_rts_wq; >>> >>>+/* EFI runtime semaphore */ >>>+extern struct semaphore efi_runtime_sem; >>>+ >>> struct linux_efi_memreserve { >>> int size; // allocated size of the array >>> atomic_t count; // number of entries used >>>-- >>>2.20.1 >>> > >-- >Be careful of reading health books, you might die of a misprint. > -- Mark Twain -- Be careful of reading health books, you might die of a misprint. -- Mark Twain ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-12 17:25 ` Hedi Berriche @ 2019-02-12 17:26 ` Ard Biesheuvel 2019-02-12 23:46 ` Hedi Berriche 1 sibling, 0 replies; 11+ messages in thread From: Ard Biesheuvel @ 2019-02-12 17:26 UTC (permalink / raw) To: Ard Biesheuvel, Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Tue, 12 Feb 2019 at 18:25, Hedi Berriche <hedi.berriche@hpe.com> wrote: > > On Thu, Feb 07, 2019 at 17:38 Hedi Berriche wrote: > >On Thu, Feb 07, 2019 at 16:05 Ard Biesheuvel wrote: > >>On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: > >>> > >>>Make efi_runtime_lock semaphore global so that it can be used by EFI > >>>runtime callers that may be defined outside efi/runtime-wrappers.c. > >>> > >>>Also now that efi_runtime_lock semaphore is no longer static, rename it > >>>to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock > >>>defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is > >>>enabled. > >>> > >>>The immediate motivation of this change is to serialise UV platform BIOS > >>>calls using the efi_runtime_sem semaphore. > >>> > >>>No functional changes. > >>> > >>>Cc: Russ Anderson <rja@hpe.com> > >>>Cc: Mike Travis <mike.travis@hpe.com> > >>>Cc: Dimitri Sivanich <sivanich@hpe.com> > >>>Cc: Steve Wahl <steve.wahl@hpe.com> > >>>Cc: stable@vger.kernel.org > >>>Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> > >> > >>Hello Hedi, > > > >Hi Ard, > > > >>Same feedback as on v1: please don't rename the lock. > > > >With the efi_runtime_lock semaphore being no longer static, not renaming it > >will cause a compile failure as it will clash with the declaration of the > >efi_runtime_lock *spinlock* in arch/x86/platform/efi/efi_64.c in the > >CONFIG_EFI_MIXED case. > > Ard, > > Any comments on whether resolving the conflict between > > {efi_runtime_lock, drivers/firmware/efi/runtime-wrappers.c} > > and > {efi_runtime_lock, arch/x86/platform/efi/efi_64.c} > > provides the required justification for renaming the efi_runtime_lock semaphore? > Hello Hedi, No it doesn't. I responded 5 days ago with a suggestion on how to address this instead. > Cheers, > Hedi. > > >>>--- > >>> drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- > >>> include/linux/efi.h | 3 ++ > >>> 2 files changed, 33 insertions(+), 30 deletions(-) > >>> > >>>diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > >>>index 8903b9ccfc2b..ec60d6227925 100644 > >>>--- a/drivers/firmware/efi/runtime-wrappers.c > >>>+++ b/drivers/firmware/efi/runtime-wrappers.c > >>>@@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; > >>> * @rts_arg<1-5>: efi_runtime_service() function arguments > >>> * > >>> * Accesses to efi_runtime_services() are serialized by a binary > >>>- * semaphore (efi_runtime_lock) and caller waits until the work is > >>>+ * semaphore (efi_runtime_sem) and caller waits until the work is > >>> * finished, hence _only_ one work is queued at a time and the caller > >>> * thread waits for completion. > >>> */ > >>>@@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > >>> * none of the remaining functions are actually ever called at runtime. > >>> * So let's just use a single lock to serialize all Runtime Services calls. > >>> */ > >>>-static DEFINE_SEMAPHORE(efi_runtime_lock); > >>>+DEFINE_SEMAPHORE(efi_runtime_sem); > >>> > >>> /* > >>> * Calls the appropriate efi_runtime_service() with the appropriate > >>>@@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, > >>> NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, > >>> NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, > >>> data); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, > >>> NULL, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, > >>> data); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_trylock(&efi_runtime_lock)) > >>>+ if (down_trylock(&efi_runtime_sem)) > >>> return EFI_NOT_READY; > >>> > >>> status = efi_call_virt(set_variable, name, vendor, attr, data_size, > >>> data); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, > >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >>> return EFI_UNSUPPORTED; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, > >>> remaining_space, max_variable_size, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, > >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >>> return EFI_UNSUPPORTED; > >>> > >>>- if (down_trylock(&efi_runtime_lock)) > >>>+ if (down_trylock(&efi_runtime_sem)) > >>> return EFI_NOT_READY; > >>> > >>> status = efi_call_virt(query_variable_info, attr, storage_space, > >>> remaining_space, max_variable_size); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) > >>> { > >>> efi_status_t status; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, > >>> NULL, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, > >>> unsigned long data_size, > >>> efi_char16_t *data) > >>> { > >>>- if (down_interruptible(&efi_runtime_lock)) { > >>>+ if (down_interruptible(&efi_runtime_sem)) { > >>> pr_warn("failed to invoke the reset_system() runtime service:\n" > >>> "could not get exclusive access to the firmware\n"); > >>> return; > >>> } > >>> efi_rts_work.efi_rts_id = RESET_SYSTEM; > >>> __efi_call_virt(reset_system, reset_type, status, data_size, data); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> } > >>> > >>> static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > >>>@@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >>> return EFI_UNSUPPORTED; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, > >>> NULL, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>@@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > >>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > >>> return EFI_UNSUPPORTED; > >>> > >>>- if (down_interruptible(&efi_runtime_lock)) > >>>+ if (down_interruptible(&efi_runtime_sem)) > >>> return EFI_ABORTED; > >>> status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, > >>> max_size, reset_type, NULL); > >>>- up(&efi_runtime_lock); > >>>+ up(&efi_runtime_sem); > >>> return status; > >>> } > >>> > >>>diff --git a/include/linux/efi.h b/include/linux/efi.h > >>>index 45ff763fba76..930cd20842b8 100644 > >>>--- a/include/linux/efi.h > >>>+++ b/include/linux/efi.h > >>>@@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; > >>> /* Workqueue to queue EFI Runtime Services */ > >>> extern struct workqueue_struct *efi_rts_wq; > >>> > >>>+/* EFI runtime semaphore */ > >>>+extern struct semaphore efi_runtime_sem; > >>>+ > >>> struct linux_efi_memreserve { > >>> int size; // allocated size of the array > >>> atomic_t count; // number of entries used > >>>-- > >>>2.20.1 > >>> > > > >-- > >Be careful of reading health books, you might die of a misprint. > > -- Mark Twain > > -- > Be careful of reading health books, you might die of a misprint. > -- Mark Twain ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock 2019-02-12 17:25 ` Hedi Berriche 2019-02-12 17:26 ` Ard Biesheuvel @ 2019-02-12 23:46 ` Hedi Berriche 1 sibling, 0 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-12 23:46 UTC (permalink / raw) To: Ard Biesheuvel Cc: Linux Kernel Mailing List, Thomas Gleixner, Bhupesh Sharma, Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, the arch/x86 maintainers, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable On Tue, Feb 12, 2019 at 17:25 Hedi Berriche wrote: >On Thu, Feb 07, 2019 at 17:38 Hedi Berriche wrote: >>On Thu, Feb 07, 2019 at 16:05 Ard Biesheuvel wrote: >>>On Thu, 7 Feb 2019 at 05:23, Hedi Berriche <hedi.berriche@hpe.com> wrote: >>>> >>>>Make efi_runtime_lock semaphore global so that it can be used by EFI >>>>runtime callers that may be defined outside efi/runtime-wrappers.c. >>>> >>>>Also now that efi_runtime_lock semaphore is no longer static, rename it >>>>to efi_runtime_sem so it doesn't clash with the efi_runtime_lock spinlock >>>>defined in arch/x86/platform/efi/efi_64.c when CONFIG_EFI_MIXED is >>>>enabled. >>>> >>>>The immediate motivation of this change is to serialise UV platform BIOS >>>>calls using the efi_runtime_sem semaphore. >>>> >>>>No functional changes. >>>> >>>>Cc: Russ Anderson <rja@hpe.com> >>>>Cc: Mike Travis <mike.travis@hpe.com> >>>>Cc: Dimitri Sivanich <sivanich@hpe.com> >>>>Cc: Steve Wahl <steve.wahl@hpe.com> >>>>Cc: stable@vger.kernel.org >>>>Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> >>> >>>Hello Hedi, >> >>Hi Ard, >> >>>Same feedback as on v1: please don't rename the lock. >> >>With the efi_runtime_lock semaphore being no longer static, not renaming it >>will cause a compile failure as it will clash with the declaration of the >>efi_runtime_lock *spinlock* in arch/x86/platform/efi/efi_64.c in the >>CONFIG_EFI_MIXED case. > >Ard, > >Any comments on whether resolving the conflict between > > {efi_runtime_lock, drivers/firmware/efi/runtime-wrappers.c} > >and > {efi_runtime_lock, arch/x86/platform/efi/efi_64.c} > >provides the required justification for renaming the efi_runtime_lock semaphore? Ard, Apologies for sending this email which must have come across as absurd given the *second* email you sent on 2019-02-07 19:38:42. The trouble is that I *never* received that email (nor the one you sent today 2019-02-12 17:26:06 as reply to this one) because for some reason my email address was dropped from the distribution list. It's only about 30 minutes ago that a colleague brought it up to my attention and forwarded both emails: Thu, 7 Feb 2019 20:38:42 +0100 Tue, 12 Feb 2019 18:26:06 +0100 No idea how my email address got dropped but I wanted to make sure to explain the seemingly absurd email I sent today as well as the lack of comment on your earlier email. Cheers, Hedi. >>>>--- >>>>drivers/firmware/efi/runtime-wrappers.c | 60 ++++++++++++------------- >>>>include/linux/efi.h | 3 ++ >>>>2 files changed, 33 insertions(+), 30 deletions(-) >>>> >>>>diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >>>>index 8903b9ccfc2b..ec60d6227925 100644 >>>>--- a/drivers/firmware/efi/runtime-wrappers.c >>>>+++ b/drivers/firmware/efi/runtime-wrappers.c >>>>@@ -53,7 +53,7 @@ struct efi_runtime_work efi_rts_work; >>>> * @rts_arg<1-5>: efi_runtime_service() function arguments >>>> * >>>> * Accesses to efi_runtime_services() are serialized by a binary >>>>- * semaphore (efi_runtime_lock) and caller waits until the work is >>>>+ * semaphore (efi_runtime_sem) and caller waits until the work is >>>> * finished, hence _only_ one work is queued at a time and the caller >>>> * thread waits for completion. >>>> */ >>>>@@ -144,7 +144,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >>>> * none of the remaining functions are actually ever called at runtime. >>>> * So let's just use a single lock to serialize all Runtime Services calls. >>>> */ >>>>-static DEFINE_SEMAPHORE(efi_runtime_lock); >>>>+DEFINE_SEMAPHORE(efi_runtime_sem); >>>> >>>>/* >>>> * Calls the appropriate efi_runtime_service() with the appropriate >>>>@@ -233,10 +233,10 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -244,10 +244,10 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -257,11 +257,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL, >>>> NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -269,11 +269,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL, >>>> NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -285,11 +285,11 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size, >>>> data); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -299,11 +299,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor, >>>> NULL, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -315,11 +315,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(SET_VARIABLE, name, vendor, &attr, &data_size, >>>> data); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -330,12 +330,12 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor, >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_trylock(&efi_runtime_lock)) >>>>+ if (down_trylock(&efi_runtime_sem)) >>>> return EFI_NOT_READY; >>>> >>>> status = efi_call_virt(set_variable, name, vendor, attr, data_size, >>>> data); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -350,11 +350,11 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, >>>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>>> return EFI_UNSUPPORTED; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(QUERY_VARIABLE_INFO, &attr, storage_space, >>>> remaining_space, max_variable_size, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -369,12 +369,12 @@ virt_efi_query_variable_info_nonblocking(u32 attr, >>>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>>> return EFI_UNSUPPORTED; >>>> >>>>- if (down_trylock(&efi_runtime_lock)) >>>>+ if (down_trylock(&efi_runtime_sem)) >>>> return EFI_NOT_READY; >>>> >>>> status = efi_call_virt(query_variable_info, attr, storage_space, >>>> remaining_space, max_variable_size); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -382,11 +382,11 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) >>>>{ >>>> efi_status_t status; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL, >>>> NULL, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -395,14 +395,14 @@ static void virt_efi_reset_system(int reset_type, >>>> unsigned long data_size, >>>> efi_char16_t *data) >>>>{ >>>>- if (down_interruptible(&efi_runtime_lock)) { >>>>+ if (down_interruptible(&efi_runtime_sem)) { >>>> pr_warn("failed to invoke the reset_system() runtime service:\n" >>>> "could not get exclusive access to the firmware\n"); >>>> return; >>>> } >>>> efi_rts_work.efi_rts_id = RESET_SYSTEM; >>>> __efi_call_virt(reset_system, reset_type, status, data_size, data); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>>} >>>> >>>>static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >>>>@@ -414,11 +414,11 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, >>>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>>> return EFI_UNSUPPORTED; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list, >>>> NULL, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>@@ -432,11 +432,11 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, >>>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >>>> return EFI_UNSUPPORTED; >>>> >>>>- if (down_interruptible(&efi_runtime_lock)) >>>>+ if (down_interruptible(&efi_runtime_sem)) >>>> return EFI_ABORTED; >>>> status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count, >>>> max_size, reset_type, NULL); >>>>- up(&efi_runtime_lock); >>>>+ up(&efi_runtime_sem); >>>> return status; >>>>} >>>> >>>>diff --git a/include/linux/efi.h b/include/linux/efi.h >>>>index 45ff763fba76..930cd20842b8 100644 >>>>--- a/include/linux/efi.h >>>>+++ b/include/linux/efi.h >>>>@@ -1745,6 +1745,9 @@ extern struct efi_runtime_work efi_rts_work; >>>>/* Workqueue to queue EFI Runtime Services */ >>>>extern struct workqueue_struct *efi_rts_wq; >>>> >>>>+/* EFI runtime semaphore */ >>>>+extern struct semaphore efi_runtime_sem; >>>>+ >>>>struct linux_efi_memreserve { >>>> int size; // allocated size of the array >>>> atomic_t count; // number of entries used >>>>-- >>>>2.20.1 >>>> >> >>-- >>Be careful of reading health books, you might die of a misprint. >> -- Mark Twain > >-- >Be careful of reading health books, you might die of a misprint. > -- Mark Twain -- Be careful of reading health books, you might die of a misprint. -- Mark Twain ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Patch v2 2/4] x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers 2019-02-07 4:22 [Patch v2 0/4] Protect against concurrent calls into UV BIOS Hedi Berriche 2019-02-07 4:22 ` [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock Hedi Berriche @ 2019-02-07 4:22 ` Hedi Berriche 2019-02-07 4:22 ` [Patch v2 3/4] x86/platform/UV: use efi_enabled() instead of test_bit() Hedi Berriche 2019-02-07 4:22 ` [Patch v2 4/4] x86/platform/UV: use efi_runtime_sem to serialise BIOS calls Hedi Berriche 3 siblings, 0 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-07 4:22 UTC (permalink / raw) To: linux-kernel, Ard Biesheuvel, Thomas Gleixner, Bhupesh Sharma Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, x86, Hedi Berriche, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable uv_bios_call_reentrant() has no callers nor is it exported, kill it. Cc: Russ Anderson <rja@hpe.com> Cc: Mike Travis <mike.travis@hpe.com> Cc: Dimitri Sivanich <sivanich@hpe.com> Cc: Steve Wahl <steve.wahl@hpe.com> Cc: stable@vger.kernel.org Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> --- arch/x86/include/asm/uv/bios.h | 1 - arch/x86/platform/uv/bios_uv.c | 12 ------------ 2 files changed, 13 deletions(-) diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h index e652a7cc6186..4eee646544b2 100644 --- a/arch/x86/include/asm/uv/bios.h +++ b/arch/x86/include/asm/uv/bios.h @@ -140,7 +140,6 @@ enum uv_memprotect { */ extern s64 uv_bios_call(enum uv_bios_cmd, u64, u64, u64, u64, u64); extern s64 uv_bios_call_irqsave(enum uv_bios_cmd, u64, u64, u64, u64, u64); -extern s64 uv_bios_call_reentrant(enum uv_bios_cmd, u64, u64, u64, u64, u64); extern s64 uv_bios_get_sn_info(int, int *, long *, long *, long *, long *); extern s64 uv_bios_freq_base(u64, u64 *); diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index 4a6a5a26c582..cd05af157763 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -66,18 +66,6 @@ s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, return ret; } -s64 uv_bios_call_reentrant(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, - u64 a4, u64 a5) -{ - s64 ret; - - preempt_disable(); - ret = uv_bios_call(which, a1, a2, a3, a4, a5); - preempt_enable(); - - return ret; -} - long sn_partition_id; EXPORT_SYMBOL_GPL(sn_partition_id); -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Patch v2 3/4] x86/platform/UV: use efi_enabled() instead of test_bit() 2019-02-07 4:22 [Patch v2 0/4] Protect against concurrent calls into UV BIOS Hedi Berriche 2019-02-07 4:22 ` [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock Hedi Berriche 2019-02-07 4:22 ` [Patch v2 2/4] x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers Hedi Berriche @ 2019-02-07 4:22 ` Hedi Berriche 2019-02-07 4:22 ` [Patch v2 4/4] x86/platform/UV: use efi_runtime_sem to serialise BIOS calls Hedi Berriche 3 siblings, 0 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-07 4:22 UTC (permalink / raw) To: linux-kernel, Ard Biesheuvel, Thomas Gleixner, Bhupesh Sharma Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, x86, Hedi Berriche, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable Use ad hoc efi_enabled() instead of fiddling with test_bit(). Cleanup, no functional changes. Cc: Russ Anderson <rja@hpe.com> Cc: Mike Travis <mike.travis@hpe.com> Cc: Dimitri Sivanich <sivanich@hpe.com> Cc: Steve Wahl <steve.wahl@hpe.com> Cc: stable@vger.kernel.org Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> --- arch/x86/platform/uv/bios_uv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index cd05af157763..8bace0ca9e57 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -44,7 +44,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI * callback method, which uses efi_call() directly, with the kernel page tables: */ - if (unlikely(test_bit(EFI_OLD_MEMMAP, &efi.flags))) + if (unlikely(efi_enabled(EFI_OLD_MEMMAP))) ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5); else ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5); -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Patch v2 4/4] x86/platform/UV: use efi_runtime_sem to serialise BIOS calls 2019-02-07 4:22 [Patch v2 0/4] Protect against concurrent calls into UV BIOS Hedi Berriche ` (2 preceding siblings ...) 2019-02-07 4:22 ` [Patch v2 3/4] x86/platform/UV: use efi_enabled() instead of test_bit() Hedi Berriche @ 2019-02-07 4:22 ` Hedi Berriche 3 siblings, 0 replies; 11+ messages in thread From: Hedi Berriche @ 2019-02-07 4:22 UTC (permalink / raw) To: linux-kernel, Ard Biesheuvel, Thomas Gleixner, Bhupesh Sharma Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-efi, x86, Hedi Berriche, Russ Anderson, Mike Travis, Dimitri Sivanich, Steve Wahl, stable Calls into UV firmware must be protected against concurrency, use the now visible efi_runtime_sem lock to serialise them. Cc: Russ Anderson <rja@hpe.com> Cc: Mike Travis <mike.travis@hpe.com> Cc: Dimitri Sivanich <sivanich@hpe.com> Cc: Steve Wahl <steve.wahl@hpe.com> Cc: stable@vger.kernel.org Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com> --- arch/x86/include/asm/uv/bios.h | 3 ++- arch/x86/platform/uv/bios_uv.c | 23 +++++++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h index 4eee646544b2..581e2978a16c 100644 --- a/arch/x86/include/asm/uv/bios.h +++ b/arch/x86/include/asm/uv/bios.h @@ -48,7 +48,8 @@ enum { BIOS_STATUS_SUCCESS = 0, BIOS_STATUS_UNIMPLEMENTED = -ENOSYS, BIOS_STATUS_EINVAL = -EINVAL, - BIOS_STATUS_UNAVAIL = -EBUSY + BIOS_STATUS_UNAVAIL = -EBUSY, + BIOS_STATUS_ABORT = -EINTR, }; /* Address map parameters */ diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index 8bace0ca9e57..e779b2a128ea 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -29,7 +29,8 @@ struct uv_systab *uv_systab; -s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) +static s64 __uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, + u64 a4, u64 a5) { struct uv_systab *tab = uv_systab; s64 ret; @@ -51,6 +52,19 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) return ret; } + +s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) +{ + s64 ret; + + if (down_interruptible(&efi_runtime_sem)) + return BIOS_STATUS_ABORT; + + ret = __uv_bios_call(which, a1, a2, a3, a4, a5); + up(&efi_runtime_sem); + + return ret; +} EXPORT_SYMBOL_GPL(uv_bios_call); s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, @@ -59,10 +73,15 @@ s64 uv_bios_call_irqsave(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, unsigned long bios_flags; s64 ret; + if (down_interruptible(&efi_runtime_sem)) + return BIOS_STATUS_ABORT; + local_irq_save(bios_flags); - ret = uv_bios_call(which, a1, a2, a3, a4, a5); + ret = __uv_bios_call(which, a1, a2, a3, a4, a5); local_irq_restore(bios_flags); + up(&efi_runtime_sem); + return ret; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-02-12 23:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-07 4:22 [Patch v2 0/4] Protect against concurrent calls into UV BIOS Hedi Berriche 2019-02-07 4:22 ` [Patch v2 1/4] efi/x86: turn EFI runtime semaphore into a global lock Hedi Berriche 2019-02-07 16:05 ` Ard Biesheuvel 2019-02-07 17:38 ` Hedi Berriche 2019-02-07 19:38 ` Ard Biesheuvel 2019-02-12 17:25 ` Hedi Berriche 2019-02-12 17:26 ` Ard Biesheuvel 2019-02-12 23:46 ` Hedi Berriche 2019-02-07 4:22 ` [Patch v2 2/4] x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers Hedi Berriche 2019-02-07 4:22 ` [Patch v2 3/4] x86/platform/UV: use efi_enabled() instead of test_bit() Hedi Berriche 2019-02-07 4:22 ` [Patch v2 4/4] x86/platform/UV: use efi_runtime_sem to serialise BIOS calls Hedi Berriche
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.