* Re: [PATCH] x86: serialize EFI time accesses on rtc_lock
2011-07-19 10:53 [PATCH] x86: serialize EFI time accesses on rtc_lock Jan Beulich
@ 2011-07-19 11:19 ` Matthew Garrett
2011-07-19 17:32 ` Rakib Mullick
2011-07-21 10:08 ` [tip:timers/rtc] x86: Serialize " tip-bot for Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Matthew Garrett @ 2011-07-19 11:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: mingo, tglx, hpa, linux-kernel
On Tue, Jul 19, 2011 at 11:53:07AM +0100, Jan Beulich wrote:
> The EFI specification requires that callers of the time related runtime
> functions serialize with other CMOS accesses in the kernel, as the EFI
> time functions may choose to also use the legacy CMOS RTC.
>
> Besides fixing a latent bug, this is a prerequisite to safely enable
> the rtc-efi driver for x86, which ought to be preferred over rtc-cmos
> on all EFI platforms.
ACK in principle, although I've just merged this and the IA64 code
together which makes that more awkward. I can fix it up, though.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: serialize EFI time accesses on rtc_lock
2011-07-19 10:53 [PATCH] x86: serialize EFI time accesses on rtc_lock Jan Beulich
2011-07-19 11:19 ` Matthew Garrett
@ 2011-07-19 17:32 ` Rakib Mullick
2011-07-19 17:54 ` Matthew Garrett
2011-07-21 10:08 ` [tip:timers/rtc] x86: Serialize " tip-bot for Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Rakib Mullick @ 2011-07-19 17:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: mingo, tglx, hpa, mjg, linux-kernel
On Tue, Jul 19, 2011 at 4:53 PM, Jan Beulich <JBeulich@novell.com> wrote:
> The EFI specification requires that callers of the time related runtime
> functions serialize with other CMOS accesses in the kernel, as the EFI
> time functions may choose to also use the legacy CMOS RTC.
>
> Besides fixing a latent bug, this is a prerequisite to safely enable
> the rtc-efi driver for x86, which ought to be preferred over rtc-cmos
> on all EFI platforms.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Matthew Garrett <mjg@redhat.com>
>
> ---
> arch/x86/platform/efi/efi.c | 39 +++++++++++++++++++++++++++++++++------
> 1 file changed, 33 insertions(+), 6 deletions(-)
>
> --- 3.0-rc7/arch/x86/platform/efi/efi.c
> +++ 3.0-rc7-x86-rtc-lock-EFI/arch/x86/platform/efi/efi.c
> @@ -79,26 +79,50 @@ early_param("add_efi_memmap", setup_add_
>
> static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> {
> - return efi_call_virt2(get_time, tm, tc);
> + unsigned long flags;
> + efi_status_t status;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + status = efi_call_virt2(get_time, tm, tc);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> + return status;
> }
>
> static efi_status_t virt_efi_set_time(efi_time_t *tm)
> {
> - return efi_call_virt1(set_time, tm);
> + unsigned long flags;
> + efi_status_t status;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + status = efi_call_virt1(set_time, tm);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> + return status;
> }
>
> static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
> efi_bool_t *pending,
> efi_time_t *tm)
> {
> - return efi_call_virt3(get_wakeup_time,
> - enabled, pending, tm);
> + unsigned long flags;
> + efi_status_t status;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + status = efi_call_virt3(get_wakeup_time,
> + enabled, pending, tm);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> + return status;
> }
>
> static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
> {
> - return efi_call_virt2(set_wakeup_time,
> - enabled, tm);
> + unsigned long flags;
> + efi_status_t status;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + status = efi_call_virt2(set_wakeup_time,
> + enabled, tm);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> + return status;
> }
>
> static efi_status_t virt_efi_get_variable(efi_char16_t *name,
> @@ -164,11 +188,14 @@ static efi_status_t __init phys_efi_set_
> static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
> efi_time_cap_t *tc)
> {
> + unsigned long flags;
> efi_status_t status;
>
> + spin_lock_irqsave(&rtc_lock, flags);
> efi_call_phys_prelog();
> status = efi_call_phys2(efi_phys.get_time, tm, tc);
> efi_call_phys_epilog();
> + spin_unlock_irqrestore(&rtc_lock, flags);
> return status;
> }
>
If I'm not missing anything, this implementation (serialization) could
be simpler by holding rtc_lock at the time of calling those functions.
I mean, holding rtc_lock before calling EFI services
virt_efi_get/set_time and virt_efi_get/set_wakeup_time - something
like below:
spin_lock_irqsave(&rtc_lock, flags);
efi.get_time = virt_efi_get_time;
efi.set_time = virt_efi_set_time;
efi.get_wakeup_time = virt_efi_get_wakeup_time;
efi.set_wakeup_time = virt_efi_set_wakeup_time;
spin_unlock_irqrestore(&rtc_lock, flags);
Thanks,
Rakib
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: serialize EFI time accesses on rtc_lock
2011-07-19 17:32 ` Rakib Mullick
@ 2011-07-19 17:54 ` Matthew Garrett
2011-07-20 12:46 ` Rakib Mullick
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Garrett @ 2011-07-19 17:54 UTC (permalink / raw)
To: Rakib Mullick; +Cc: Jan Beulich, mingo, tglx, hpa, linux-kernel
On Tue, Jul 19, 2011 at 11:32:05PM +0600, Rakib Mullick wrote:
> If I'm not missing anything, this implementation (serialization) could
> be simpler by holding rtc_lock at the time of calling those functions.
> I mean, holding rtc_lock before calling EFI services
> virt_efi_get/set_time and virt_efi_get/set_wakeup_time - something
> like below:
>
> spin_lock_irqsave(&rtc_lock, flags);
> efi.get_time = virt_efi_get_time;
> efi.set_time = virt_efi_set_time;
> efi.get_wakeup_time = virt_efi_get_wakeup_time;
> efi.set_wakeup_time = virt_efi_set_wakeup_time;
> spin_unlock_irqrestore(&rtc_lock, flags);
That's just assigning some function pointers, not actually making the
calls. There's potentially several routes to each of the calls, so it
makes sense to do the locking there.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: serialize EFI time accesses on rtc_lock
2011-07-19 17:54 ` Matthew Garrett
@ 2011-07-20 12:46 ` Rakib Mullick
0 siblings, 0 replies; 6+ messages in thread
From: Rakib Mullick @ 2011-07-20 12:46 UTC (permalink / raw)
To: Matthew Garrett; +Cc: Jan Beulich, mingo, tglx, hpa, linux-kernel
On Tue, Jul 19, 2011 at 11:54 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jul 19, 2011 at 11:32:05PM +0600, Rakib Mullick wrote:
> That's just assigning some function pointers, not actually making the
> calls. There's potentially several routes to each of the calls, so it
> makes sense to do the locking there.
>
I got confused by the patch subject. I thought, this serialization is
required at the time of startup.
Thanks,
Rakib
> --
> Matthew Garrett | mjg59@srcf.ucam.org
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:timers/rtc] x86: Serialize EFI time accesses on rtc_lock
2011-07-19 10:53 [PATCH] x86: serialize EFI time accesses on rtc_lock Jan Beulich
2011-07-19 11:19 ` Matthew Garrett
2011-07-19 17:32 ` Rakib Mullick
@ 2011-07-21 10:08 ` tip-bot for Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Jan Beulich @ 2011-07-21 10:08 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, mjg, hpa, mingo, mjg59, jbeulich, JBeulich, tglx,
mingo
Commit-ID: ef68c8f87ed13f65df867dddf36c0e185b27b942
Gitweb: http://git.kernel.org/tip/ef68c8f87ed13f65df867dddf36c0e185b27b942
Author: Jan Beulich <JBeulich@novell.com>
AuthorDate: Tue, 19 Jul 2011 11:53:07 +0100
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 21 Jul 2011 09:21:00 +0200
x86: Serialize EFI time accesses on rtc_lock
The EFI specification requires that callers of the time related
runtime functions serialize with other CMOS accesses in the
kernel, as the EFI time functions may choose to also use the
legacy CMOS RTC.
Besides fixing a latent bug, this is a prerequisite to safely
enable the rtc-efi driver for x86, which ought to be preferred
over rtc-cmos on all EFI platforms.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Acked-by: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: <mjg@redhat.com>
Link: http://lkml.kernel.org/r/4E257E33020000780004E319@nat28.tlf.novell.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Matthew Garrett <mjg@redhat.com>
---
arch/x86/platform/efi/efi.c | 39 +++++++++++++++++++++++++++++++++------
1 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 474356b..b8823d5 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -79,26 +79,50 @@ early_param("add_efi_memmap", setup_add_efi_memmap);
static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
{
- return efi_call_virt2(get_time, tm, tc);
+ unsigned long flags;
+ efi_status_t status;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ status = efi_call_virt2(get_time, tm, tc);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ return status;
}
static efi_status_t virt_efi_set_time(efi_time_t *tm)
{
- return efi_call_virt1(set_time, tm);
+ unsigned long flags;
+ efi_status_t status;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ status = efi_call_virt1(set_time, tm);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ return status;
}
static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
efi_bool_t *pending,
efi_time_t *tm)
{
- return efi_call_virt3(get_wakeup_time,
- enabled, pending, tm);
+ unsigned long flags;
+ efi_status_t status;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ status = efi_call_virt3(get_wakeup_time,
+ enabled, pending, tm);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ return status;
}
static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
{
- return efi_call_virt2(set_wakeup_time,
- enabled, tm);
+ unsigned long flags;
+ efi_status_t status;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ status = efi_call_virt2(set_wakeup_time,
+ enabled, tm);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ return status;
}
static efi_status_t virt_efi_get_variable(efi_char16_t *name,
@@ -164,11 +188,14 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
efi_time_cap_t *tc)
{
+ unsigned long flags;
efi_status_t status;
+ spin_lock_irqsave(&rtc_lock, flags);
efi_call_phys_prelog();
status = efi_call_phys2(efi_phys.get_time, tm, tc);
efi_call_phys_epilog();
+ spin_unlock_irqrestore(&rtc_lock, flags);
return status;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread