[PATCH] x86, efi: Don't recursively acquire rtc_lock

[PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

From: Matt Fleming <matt.fleming@linux.intel.com>

A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
Serialize EFI time accesses on rtc_lock") because efi_get_time() and
friends can be called with rtc_lock already held by
read_persistent_time(), e.g.

timekeeping_init()
    read_persistent_clock()	<-- acquire rtc_lock
        efi_get_time()
            phys_efi_get_time()	<-- acquire rtc_lock <DEADLOCK>

Move the locking up into the caller of efi.get_time() and provide some
wrappers for use in other parts of the kernel instead of calling
efi.get_time(), etc directly. This way we can hide the rtc_lock dance
inside of arch/x86.

Cc: Jan Beulich <JBeulich@novell.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: "H. Peter Anvin" <hpa@linux.intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
 arch/ia64/kernel/efi.c      |   10 ++++++++
 arch/x86/platform/efi/efi.c |   49 ++++++++++++++++++++++++++----------------
 drivers/char/efirtc.c       |    6 ++--
 drivers/rtc/rtc-efi.c       |    8 +++---
 include/linux/efi.h         |    4 ++-
 5 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 6fc03af..775e05f 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -229,6 +229,16 @@ STUB_SET_VARIABLE(virt, id)
 STUB_GET_NEXT_HIGH_MONO_COUNT(virt, id)
 STUB_RESET_SYSTEM(virt, id)
 
+efi_status_t efi_get_time(efi_time_t *tm, efi_time_cap_t *cap)
+{
+	return efi.get_time(tm, cap);
+}
+
+efi_status_t efi_set_time(efi_time_t *tm)
+{
+	return efi.set_time(tm);
+}
+
 void
 efi_gettimeofday (struct timespec *ts)
 {
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3ae4128..735a470 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -89,24 +89,12 @@ 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)
 {
-	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;
+	return efi_call_virt2(get_time, tm, tc);
 }
 
 static efi_status_t virt_efi_set_time(efi_time_t *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;
+	return efi_call_virt1(set_time, tm);
 }
 
 static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
@@ -232,14 +220,11 @@ 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;
 }
 
@@ -250,6 +235,8 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	efi_time_t 	eft;
 	efi_time_cap_t 	cap;
 
+	assert_spin_locked(&rtc_lock);
+
 	status = efi.get_time(&eft, &cap);
 	if (status != EFI_SUCCESS) {
 		printk(KERN_ERR "Oops: efitime: can't read time!\n");
@@ -272,12 +259,14 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+unsigned long efi_get_time_locked(void)
 {
 	efi_status_t status;
 	efi_time_t eft;
 	efi_time_cap_t cap;
 
+	assert_spin_locked(&rtc_lock);
+
 	status = efi.get_time(&eft, &cap);
 	if (status != EFI_SUCCESS)
 		printk(KERN_ERR "Oops: efitime: can't read time!\n");
@@ -286,6 +275,28 @@ unsigned long efi_get_time(void)
 		      eft.minute, eft.second);
 }
 
+efi_status_t efi_get_time(efi_time_t *tm, efi_time_cap_t *cap)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi.get_time(tm, cap);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
+efi_status_t efi_set_time(efi_time_t *tm)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi.set_time(tm);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
 /*
  * Tell the kernel about the EFI memory map.  This might include
  * more than the max 128 entries that can fit in the e820 legacy
@@ -571,7 +582,7 @@ void __init efi_init(void)
 		do_add_efi_memmap();
 
 #ifdef CONFIG_X86_32
-	x86_platform.get_wallclock = efi_get_time;
+	x86_platform.get_wallclock = efi_get_time_locked;
 	x86_platform.set_wallclock = efi_set_rtc_mmss;
 #endif
 
diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
index 53c524e..cdb1bed 100644
--- a/drivers/char/efirtc.c
+++ b/drivers/char/efirtc.c
@@ -174,7 +174,7 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 		case RTC_RD_TIME:
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
-			status = efi.get_time(&eft, &cap);
+			status = efi_get_time(&eft, &cap);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
 
@@ -201,7 +201,7 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
-			status = efi.set_time(&eft);
+			status = efi_set_time(&eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
 
@@ -312,7 +312,7 @@ efi_rtc_get_status(char *buf)
 
 	spin_lock_irqsave(&efi_rtc_lock, flags);
 
-	efi.get_time(&eft, &cap);
+	efi_get_time(&eft, &cap);
 	efi.get_wakeup_time(&enabled, &pending, &alm);
 
 	spin_unlock_irqrestore(&efi_rtc_lock,flags);
diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c
index 5502923..6307f5c 100644
--- a/drivers/rtc/rtc-efi.c
+++ b/drivers/rtc/rtc-efi.c
@@ -153,7 +153,7 @@ static int efi_read_time(struct device *dev, struct rtc_time *tm)
 	efi_time_t eft;
 	efi_time_cap_t cap;
 
-	status = efi.get_time(&eft, &cap);
+	status = efi_get_time(&eft, &cap);
 
 	if (status != EFI_SUCCESS) {
 		/* should never happen */
@@ -166,21 +166,21 @@ static int efi_read_time(struct device *dev, struct rtc_time *tm)
 	return rtc_valid_tm(tm);
 }
 
-static int efi_set_time(struct device *dev, struct rtc_time *tm)
+static int rtc_efi_set_time(struct device *dev, struct rtc_time *tm)
 {
 	efi_status_t status;
 	efi_time_t eft;
 
 	convert_to_efi_time(tm, &eft);
 
-	status = efi.set_time(&eft);
+	status = efi_set_time(&eft);
 
 	return status == EFI_SUCCESS ? 0 : -EINVAL;
 }
 
 static const struct rtc_class_ops efi_rtc_ops = {
 	.read_time = efi_read_time,
-	.set_time = efi_set_time,
+	.set_time = rtc_efi_set_time,
 	.read_alarm = efi_read_alarm,
 	.set_alarm = efi_set_alarm,
 };
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c5709ed..57cd963 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -454,7 +454,9 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
-extern unsigned long efi_get_time(void);
+extern unsigned long efi_get_time_locked(void);
+extern efi_status_t efi_get_time(efi_time_t *tm, efi_time_cap_t *cap);
+extern efi_status_t efi_set_time(efi_time_t *tm);
 extern int efi_set_rtc_mmss(unsigned long nowtime);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
-- 
1.7.4.4

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Tony Luck]

On Wed, Aug 3, 2011 at 2:04 PM, Matt Fleming <matt@console-pimps.org> wrote:
> From: Matt Fleming <matt.fleming@linux.intel.com>
>
> A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> friends can be called with rtc_lock already held by
> read_persistent_time(), e.g.
>
> timekeeping_init()
>    read_persistent_clock()     <-- acquire rtc_lock
>        efi_get_time()
>            phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
>
> Move the locking up into the caller of efi.get_time() and provide some
> wrappers for use in other parts of the kernel instead of calling
> efi.get_time(), etc directly. This way we can hide the rtc_lock dance
> inside of arch/x86.

ia64 bits look ok (they build & boot too - which is always a plus :-)

Acked-by: Tony Luck <tony.luck@intel.com>

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[H. Peter Anvin]

On 08/03/2011 02:04 PM, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@linux.intel.com>
> 
> A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> friends can be called with rtc_lock already held by
> read_persistent_time(), e.g.
> 
> timekeeping_init()
>     read_persistent_clock()	<-- acquire rtc_lock
>         efi_get_time()
>             phys_efi_get_time()	<-- acquire rtc_lock <DEADLOCK>
> 
> Move the locking up into the caller of efi.get_time() and provide some
> wrappers for use in other parts of the kernel instead of calling
> efi.get_time(), etc directly. This way we can hide the rtc_lock dance
> inside of arch/x86.
> 
> Cc: Jan Beulich <JBeulich@novell.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: "H. Peter Anvin" <hpa@linux.intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>

For the x86 footprint:

Acked-by: H. Peter Anvin <hpa@linux.intel.com>

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[john stultz]

On Wed, Aug 3, 2011 at 2:04 PM, Matt Fleming <matt@console-pimps.org> wrote:
> From: Matt Fleming <matt.fleming@linux.intel.com>
>
> A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> friends can be called with rtc_lock already held by
> read_persistent_time(), e.g.
>
> timekeeping_init()
>    read_persistent_clock()     <-- acquire rtc_lock
>        efi_get_time()
>            phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
>
> Move the locking up into the caller of efi.get_time() and provide some
> wrappers for use in other parts of the kernel instead of calling
> efi.get_time(), etc directly. This way we can hide the rtc_lock dance
> inside of arch/x86.

Sorry if this should be obvious, but is there a reason your not using
your own internal lock for serializing the efi bits rather then using
the rtc_lock?

thanks
-john

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Thu, 2011-08-04 at 03:22 -0700, john stultz wrote:
> On Wed, Aug 3, 2011 at 2:04 PM, Matt Fleming <matt@console-pimps.org> wrote:
> > From: Matt Fleming <matt.fleming@linux.intel.com>
> >
> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> > friends can be called with rtc_lock already held by
> > read_persistent_time(), e.g.
> >
> > timekeeping_init()
> >    read_persistent_clock()     <-- acquire rtc_lock
> >        efi_get_time()
> >            phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> >
> > Move the locking up into the caller of efi.get_time() and provide some
> > wrappers for use in other parts of the kernel instead of calling
> > efi.get_time(), etc directly. This way we can hide the rtc_lock dance
> > inside of arch/x86.
> 
> Sorry if this should be obvious, but is there a reason your not using
> your own internal lock for serializing the efi bits rather then using
> the rtc_lock?

Jan wrote the original code that started using the rtc_lock inside the
EFI bits here, ef68c8f87ed1 ("x86: Serialize EFI time accesses on
rtc_lock").

Jan?
 
-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matthew Garrett]

On Thu, Aug 04, 2011 at 03:22:24AM -0700, john stultz wrote:

> Sorry if this should be obvious, but is there a reason your not using
> your own internal lock for serializing the efi bits rather then using
> the rtc_lock?

On x86 systems the EFI clock is almost certainly the AT RTC, so it's 
necessary to serialise accesses between EFI and the traditional clock 
interface.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> Matt Fleming 08/03/11 11:04 PM >>> 
>From: Matt Fleming 
>
>A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86: 
>Serialize EFI time accesses on rtc_lock") because efi_get_time() and 
>friends can be called with rtc_lock already held by 
>read_persistent_time(), e.g. 
>
>timekeeping_init() 
>read_persistent_clock() <-- acquire rtc_lock 
>efi_get_time() 
>phys_efi_get_time() <-- acquire rtc_lock 
>
>Move the locking up into the caller of efi.get_time() and provide some 
>wrappers for use in other parts of the kernel instead of calling 
>efi.get_time(), etc directly. This way we can hide the rtc_lock dance 
>inside of arch/x86. 

Seems the wrong approach to me: The call happening with the lock held
is the wrong part imo, and hence the fix ought to be to drop the lock
there.

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matthew Garrett]

Well, the rtc_lock is an implementation detail of x86, while we're 
aiming to make the EFI code as general as possible. So I can see the 
argument for moving it to the callers.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Thu, 2011-08-04 at 03:53 +0100, Jan Beulich wrote:
> 
> Seems the wrong approach to me: The call happening with the lock held
> is the wrong part imo, and hence the fix ought to be to drop the lock
> there.

But what about other platforms that provide a get_wallclock()
implementation such as the kvm or xen code? If we called get_wallclock()
without rtc_lock held we'd be requiring everyone to lock it in their
clock code, which is unnecessary work and increases the amount of code
that touches rtc_lock (not to mention spreading it across several
files).

I think it's much better to do the locking as high up the callstack as
possible and preferably in as few places as possible.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> Matt Fleming  08/04/11 5:37 PM >>>
>On Thu, 2011-08-04 at 03:22 -0700, john stultz wrote:
>> Sorry if this should be obvious, but is there a reason your not using
>> your own internal lock for serializing the efi bits rather then using
>> the rtc_lock?
>
>Jan wrote the original code that started using the rtc_lock inside the
>EFI bits here, ef68c8f87ed1 ("x86: Serialize EFI time accesses on
>rtc_lock").
>
>Jan?
 
Matthew Garrett already replied to this - it's (on x86) a requirement of the EFI spec.

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> Matt Fleming  08/04/11 11:33 AM >>>
>On Thu, 2011-08-04 at 03:53 +0100, Jan Beulich wrote:
>> 
>> Seems the wrong approach to me: The call happening with the lock held
>> is the wrong part imo, and hence the fix ought to be to drop the lock
>> there.
>
>But what about other platforms that provide a get_wallclock()
>implementation such as the kvm or xen code? If we called get_wallclock()

Virtual platforms will have to take care of the serialization in the
host anyway, so the guest side implementation of getwallclock et al
is entirely unaffected.

>without rtc_lock held we'd be requiring everyone to lock it in their
>clock code, which is unnecessary work and increases the amount of code
>that touches rtc_lock (not to mention spreading it across several
>files).
>
>I think it's much better to do the locking as high up the callstack as
>possible and preferably in as few places as possible.

I agree to the "as few places as possible" part.

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
> 
> Virtual platforms will have to take care of the serialization in the
> host anyway, so the guest side implementation of getwallclock et al
> is entirely unaffected.

Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
actually required by any other code. In which case, yes, it completely
makes sense to push the locking of rtc_lock down into the
implementations that actually need it.

It'd be great if I could get some ACK's from the virtualization guys.

--------8<--------

>From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Mon, 8 Aug 2011 12:59:35 +0100
Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock

A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
Serialize EFI time accesses on rtc_lock") because efi_get_time() and
friends can be called with rtc_lock already held by
read_persistent_time(), e.g.

timekeeping_init()
    read_persistent_clock()     <-- acquire rtc_lock
        efi_get_time()
            phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>

To fix this let's push the locking down into the get_wallclock() and
set_wallclock() implementations. Only the clock implementations that
access the x86 RTC directly need to acquire rtc_lock, so it makes
sense to push the locking down into the rtc, vrtc and efi code.

The virtualization implementations don't require rtc_lock to be held
because they provide their own serialization.

Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/x86/kernel/rtc.c         |   23 ++++++++++++-----------
 arch/x86/platform/mrst/vrtc.c |    9 +++++++++
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 3f2ad26..ccdbc16 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -42,8 +42,11 @@ int mach_set_rtc_mmss(unsigned long nowtime)
 {
 	int real_seconds, real_minutes, cmos_minutes;
 	unsigned char save_control, save_freq_select;
+	unsigned long flags;
 	int retval = 0;
 
+	spin_lock_irqsave(&rtc_lock, flags);
+
 	 /* tell the clock it's being set */
 	save_control = CMOS_READ(RTC_CONTROL);
 	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
@@ -93,12 +96,17 @@ int mach_set_rtc_mmss(unsigned long nowtime)
 	CMOS_WRITE(save_control, RTC_CONTROL);
 	CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
 
+	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	return retval;
 }
 
 unsigned long mach_get_cmos_time(void)
 {
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtc_lock, flags);
 
 	/*
 	 * If UIP is clear, then we have >= 244 microseconds before
@@ -125,6 +133,8 @@ unsigned long mach_get_cmos_time(void)
 	status = CMOS_READ(RTC_CONTROL);
 	WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
 
+	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
 		sec = bcd2bin(sec);
 		min = bcd2bin(min);
@@ -169,24 +179,15 @@ EXPORT_SYMBOL(rtc_cmos_write);
 
 int update_persistent_clock(struct timespec now)
 {
-	unsigned long flags;
-	int retval;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	retval = x86_platform.set_wallclock(now.tv_sec);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-
-	return retval;
+	return x86_platform.set_wallclock(now.tv_sec);
 }
 
 /* not static: needed by APM */
 void read_persistent_clock(struct timespec *ts)
 {
-	unsigned long retval, flags;
+	unsigned long retval;
 
-	spin_lock_irqsave(&rtc_lock, flags);
 	retval = x86_platform.get_wallclock();
-	spin_unlock_irqrestore(&rtc_lock, flags);
 
 	ts->tv_sec = retval;
 	ts->tv_nsec = 0;
diff --git a/arch/x86/platform/mrst/vrtc.c b/arch/x86/platform/mrst/vrtc.c
index 73d70d6..6d5dbcd 100644
--- a/arch/x86/platform/mrst/vrtc.c
+++ b/arch/x86/platform/mrst/vrtc.c
@@ -58,8 +58,11 @@ EXPORT_SYMBOL_GPL(vrtc_cmos_write);
 unsigned long vrtc_get_time(void)
 {
 	u8 sec, min, hour, mday, mon;
+	unsigned long flags;
 	u32 year;
 
+	spin_lock_irqsave(&rtc_lock, flags);
+
 	while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP))
 		cpu_relax();
 
@@ -70,6 +73,8 @@ unsigned long vrtc_get_time(void)
 	mon = vrtc_cmos_read(RTC_MONTH);
 	year = vrtc_cmos_read(RTC_YEAR);
 
+	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	/* vRTC YEAR reg contains the offset to 1960 */
 	year += 1960;
 
@@ -83,8 +88,10 @@ unsigned long vrtc_get_time(void)
 int vrtc_set_mmss(unsigned long nowtime)
 {
 	int real_sec, real_min;
+	unsigned long flags;
 	int vrtc_min;
 
+	spin_lock_irqsave(&rtc_lock, flags);
 	vrtc_min = vrtc_cmos_read(RTC_MINUTES);
 
 	real_sec = nowtime % 60;
@@ -95,6 +102,8 @@ int vrtc_set_mmss(unsigned long nowtime)
 
 	vrtc_cmos_write(real_sec, RTC_SECONDS);
 	vrtc_cmos_write(real_min, RTC_MINUTES);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	return 0;
 }
 
-- 
1.7.4.4

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> On 08.08.11 at 15:40, Matt Fleming <matt@console-pimps.org> wrote:
> On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
>> 
>> Virtual platforms will have to take care of the serialization in the
>> host anyway, so the guest side implementation of getwallclock et al
>> is entirely unaffected.
> 
> Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
> actually required by any other code. In which case, yes, it completely
> makes sense to push the locking of rtc_lock down into the
> implementations that actually need it.
> 
> It'd be great if I could get some ACK's from the virtualization guys.
> 
> --------8<--------
> 
> From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Mon, 8 Aug 2011 12:59:35 +0100
> Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock
> 
> A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> friends can be called with rtc_lock already held by
> read_persistent_time(), e.g.
> 
> timekeeping_init()
>     read_persistent_clock()     <-- acquire rtc_lock
>         efi_get_time()
>             phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> 
> To fix this let's push the locking down into the get_wallclock() and
> set_wallclock() implementations. Only the clock implementations that
> access the x86 RTC directly need to acquire rtc_lock, so it makes
> sense to push the locking down into the rtc, vrtc and efi code.
> 
> The virtualization implementations don't require rtc_lock to be held
> because they provide their own serialization.
> 
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

Acked-by: Jan Beulich <jbeulich@novell.com>

> ---
>  arch/x86/kernel/rtc.c         |   23 ++++++++++++-----------
>  arch/x86/platform/mrst/vrtc.c |    9 +++++++++
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 3f2ad26..ccdbc16 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -42,8 +42,11 @@ int mach_set_rtc_mmss(unsigned long nowtime)
>  {
>  	int real_seconds, real_minutes, cmos_minutes;
>  	unsigned char save_control, save_freq_select;
> +	unsigned long flags;
>  	int retval = 0;
>  
> +	spin_lock_irqsave(&rtc_lock, flags);
> +
>  	 /* tell the clock it's being set */
>  	save_control = CMOS_READ(RTC_CONTROL);
>  	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
> @@ -93,12 +96,17 @@ int mach_set_rtc_mmss(unsigned long nowtime)
>  	CMOS_WRITE(save_control, RTC_CONTROL);
>  	CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
>  
> +	spin_unlock_irqrestore(&rtc_lock, flags);
> +
>  	return retval;
>  }
>  
>  unsigned long mach_get_cmos_time(void)
>  {
>  	unsigned int status, year, mon, day, hour, min, sec, century = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rtc_lock, flags);
>  
>  	/*
>  	 * If UIP is clear, then we have >= 244 microseconds before
> @@ -125,6 +133,8 @@ unsigned long mach_get_cmos_time(void)
>  	status = CMOS_READ(RTC_CONTROL);
>  	WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
>  
> +	spin_unlock_irqrestore(&rtc_lock, flags);
> +
>  	if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
>  		sec = bcd2bin(sec);
>  		min = bcd2bin(min);
> @@ -169,24 +179,15 @@ EXPORT_SYMBOL(rtc_cmos_write);
>  
>  int update_persistent_clock(struct timespec now)
>  {
> -	unsigned long flags;
> -	int retval;
> -
> -	spin_lock_irqsave(&rtc_lock, flags);
> -	retval = x86_platform.set_wallclock(now.tv_sec);
> -	spin_unlock_irqrestore(&rtc_lock, flags);
> -
> -	return retval;
> +	return x86_platform.set_wallclock(now.tv_sec);
>  }
>  
>  /* not static: needed by APM */
>  void read_persistent_clock(struct timespec *ts)
>  {
> -	unsigned long retval, flags;
> +	unsigned long retval;
>  
> -	spin_lock_irqsave(&rtc_lock, flags);
>  	retval = x86_platform.get_wallclock();
> -	spin_unlock_irqrestore(&rtc_lock, flags);
>  
>  	ts->tv_sec = retval;
>  	ts->tv_nsec = 0;
> diff --git a/arch/x86/platform/mrst/vrtc.c b/arch/x86/platform/mrst/vrtc.c
> index 73d70d6..6d5dbcd 100644
> --- a/arch/x86/platform/mrst/vrtc.c
> +++ b/arch/x86/platform/mrst/vrtc.c
> @@ -58,8 +58,11 @@ EXPORT_SYMBOL_GPL(vrtc_cmos_write);
>  unsigned long vrtc_get_time(void)
>  {
>  	u8 sec, min, hour, mday, mon;
> +	unsigned long flags;
>  	u32 year;
>  
> +	spin_lock_irqsave(&rtc_lock, flags);
> +
>  	while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP))
>  		cpu_relax();
>  
> @@ -70,6 +73,8 @@ unsigned long vrtc_get_time(void)
>  	mon = vrtc_cmos_read(RTC_MONTH);
>  	year = vrtc_cmos_read(RTC_YEAR);
>  
> +	spin_unlock_irqrestore(&rtc_lock, flags);
> +
>  	/* vRTC YEAR reg contains the offset to 1960 */
>  	year += 1960;
>  
> @@ -83,8 +88,10 @@ unsigned long vrtc_get_time(void)
>  int vrtc_set_mmss(unsigned long nowtime)
>  {
>  	int real_sec, real_min;
> +	unsigned long flags;
>  	int vrtc_min;
>  
> +	spin_lock_irqsave(&rtc_lock, flags);
>  	vrtc_min = vrtc_cmos_read(RTC_MINUTES);
>  
>  	real_sec = nowtime % 60;
> @@ -95,6 +102,8 @@ int vrtc_set_mmss(unsigned long nowtime)
>  
>  	vrtc_cmos_write(real_sec, RTC_SECONDS);
>  	vrtc_cmos_write(real_min, RTC_MINUTES);
> +	spin_unlock_irqrestore(&rtc_lock, flags);
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.4.4

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> >>> On 08.08.11 at 15:40, Matt Fleming <matt@console-pimps.org> wrote:
> > On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
> >> 
> >> Virtual platforms will have to take care of the serialization in the
> >> host anyway, so the guest side implementation of getwallclock et al
> >> is entirely unaffected.
> > 
> > Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
> > actually required by any other code. In which case, yes, it completely
> > makes sense to push the locking of rtc_lock down into the
> > implementations that actually need it.
> > 
> > It'd be great if I could get some ACK's from the virtualization guys.
> > 
> > --------8<--------
> > 
> > From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
> > From: Matt Fleming <matt.fleming@intel.com>
> > Date: Mon, 8 Aug 2011 12:59:35 +0100
> > Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock
> > 
> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> > friends can be called with rtc_lock already held by
> > read_persistent_time(), e.g.
> > 
> > timekeeping_init()
> >     read_persistent_clock()     <-- acquire rtc_lock
> >         efi_get_time()
> >             phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> > 
> > To fix this let's push the locking down into the get_wallclock() and
> > set_wallclock() implementations. Only the clock implementations that
> > access the x86 RTC directly need to acquire rtc_lock, so it makes
> > sense to push the locking down into the rtc, vrtc and efi code.
> > 
> > The virtualization implementations don't require rtc_lock to be held
> > because they provide their own serialization.
> > 
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> 
> Acked-by: Jan Beulich <jbeulich@novell.com>

How urgent is thix fix - can it wait until Thomas comes back and 
starts processing patches again?

Thanks,

	Ingo

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> On 10.08.11 at 11:03, Ingo Molnar <mingo@elte.hu> wrote:

> * Jan Beulich <JBeulich@novell.com> wrote:
> 
>> >>> On 08.08.11 at 15:40, Matt Fleming <matt@console-pimps.org> wrote:
>> > On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
>> >> 
>> >> Virtual platforms will have to take care of the serialization in the
>> >> host anyway, so the guest side implementation of getwallclock et al
>> >> is entirely unaffected.
>> > 
>> > Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
>> > actually required by any other code. In which case, yes, it completely
>> > makes sense to push the locking of rtc_lock down into the
>> > implementations that actually need it.
>> > 
>> > It'd be great if I could get some ACK's from the virtualization guys.
>> > 
>> > --------8<--------
>> > 
>> > From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
>> > From: Matt Fleming <matt.fleming@intel.com>
>> > Date: Mon, 8 Aug 2011 12:59:35 +0100
>> > Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock
>> > 
>> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
>> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
>> > friends can be called with rtc_lock already held by
>> > read_persistent_time(), e.g.
>> > 
>> > timekeeping_init()
>> >     read_persistent_clock()     <-- acquire rtc_lock
>> >         efi_get_time()
>> >             phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
>> > 
>> > To fix this let's push the locking down into the get_wallclock() and
>> > set_wallclock() implementations. Only the clock implementations that
>> > access the x86 RTC directly need to acquire rtc_lock, so it makes
>> > sense to push the locking down into the rtc, vrtc and efi code.
>> > 
>> > The virtualization implementations don't require rtc_lock to be held
>> > because they provide their own serialization.
>> > 
>> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> 
>> Acked-by: Jan Beulich <jbeulich@novell.com>
> 
> How urgent is thix fix - can it wait until Thomas comes back and 
> starts processing patches again?

Without it booting on EFI can't be expected to work (and while I was
really sure I tested this before submitting, I apparently must have
used a stale kernel then - I'm really sorry for that), so getting it
applied soon would be rather desirable.

But then again I don't seem to be able to boot 3.0.1 with both patches
applied on my only EFI box, so there must be something else that
broke, and hence maybe I really did test it (on 3.0-rc7) and it worked
then. Debugging...

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> >>> On 10.08.11 at 11:03, Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Jan Beulich <JBeulich@novell.com> wrote:
> > 
> >> >>> On 08.08.11 at 15:40, Matt Fleming <matt@console-pimps.org> wrote:
> >> > On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
> >> >> 
> >> >> Virtual platforms will have to take care of the serialization in the
> >> >> host anyway, so the guest side implementation of getwallclock et al
> >> >> is entirely unaffected.
> >> > 
> >> > Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
> >> > actually required by any other code. In which case, yes, it completely
> >> > makes sense to push the locking of rtc_lock down into the
> >> > implementations that actually need it.
> >> > 
> >> > It'd be great if I could get some ACK's from the virtualization guys.
> >> > 
> >> > --------8<--------
> >> > 
> >> > From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
> >> > From: Matt Fleming <matt.fleming@intel.com>
> >> > Date: Mon, 8 Aug 2011 12:59:35 +0100
> >> > Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock
> >> > 
> >> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> >> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> >> > friends can be called with rtc_lock already held by
> >> > read_persistent_time(), e.g.
> >> > 
> >> > timekeeping_init()
> >> >     read_persistent_clock()     <-- acquire rtc_lock
> >> >         efi_get_time()
> >> >             phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> >> > 
> >> > To fix this let's push the locking down into the get_wallclock() and
> >> > set_wallclock() implementations. Only the clock implementations that
> >> > access the x86 RTC directly need to acquire rtc_lock, so it makes
> >> > sense to push the locking down into the rtc, vrtc and efi code.
> >> > 
> >> > The virtualization implementations don't require rtc_lock to be held
> >> > because they provide their own serialization.
> >> > 
> >> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> >> 
> >> Acked-by: Jan Beulich <jbeulich@novell.com>
> > 
> > How urgent is thix fix - can it wait until Thomas comes back and 
> > starts processing patches again?
> 
> Without it booting on EFI can't be expected to work (and while I 
> was really sure I tested this before submitting, I apparently must 
> have used a stale kernel then - I'm really sorry for that), so 
> getting it applied soon would be rather desirable.
> 
> But then again I don't seem to be able to boot 3.0.1 with both 
> patches applied on my only EFI box, so there must be something else 
> that broke, and hence maybe I really did test it (on 3.0-rc7) and 
> it worked then. Debugging...

Ok, once you figure it out we can merge all fixes upstream ASAP.

Thanks,

	Ingo

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> On 10.08.11 at 11:51, Ingo Molnar <mingo@elte.hu> wrote:

> * Jan Beulich <JBeulich@novell.com> wrote:
> 
>> >>> On 10.08.11 at 11:03, Ingo Molnar <mingo@elte.hu> wrote:
>> 
>> > * Jan Beulich <JBeulich@novell.com> wrote:
>> > 
>> >> >>> On 08.08.11 at 15:40, Matt Fleming <matt@console-pimps.org> wrote:
>> >> > On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
>> >> >> 
>> >> >> Virtual platforms will have to take care of the serialization in the
>> >> >> host anyway, so the guest side implementation of getwallclock et al
>> >> >> is entirely unaffected.
>> >> > 
>> >> > Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
>> >> > actually required by any other code. In which case, yes, it completely
>> >> > makes sense to push the locking of rtc_lock down into the
>> >> > implementations that actually need it.
>> >> > 
>> >> > It'd be great if I could get some ACK's from the virtualization guys.
>> >> > 
>> >> > --------8<--------
>> >> > 
>> >> > From a0a39dbb69f6ac675846bf00f30ad153506a4567 Mon Sep 17 00:00:00 2001
>> >> > From: Matt Fleming <matt.fleming@intel.com>
>> >> > Date: Mon, 8 Aug 2011 12:59:35 +0100
>> >> > Subject: [PATCH] x86, efi: Don't recursively acquire rtc_lock
>> >> > 
>> >> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
>> >> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
>> >> > friends can be called with rtc_lock already held by
>> >> > read_persistent_time(), e.g.
>> >> > 
>> >> > timekeeping_init()
>> >> >     read_persistent_clock()     <-- acquire rtc_lock
>> >> >         efi_get_time()
>> >> >             phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
>> >> > 
>> >> > To fix this let's push the locking down into the get_wallclock() and
>> >> > set_wallclock() implementations. Only the clock implementations that
>> >> > access the x86 RTC directly need to acquire rtc_lock, so it makes
>> >> > sense to push the locking down into the rtc, vrtc and efi code.
>> >> > 
>> >> > The virtualization implementations don't require rtc_lock to be held
>> >> > because they provide their own serialization.
>> >> > 
>> >> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> >> 
>> >> Acked-by: Jan Beulich <jbeulich@novell.com>
>> > 
>> > How urgent is thix fix - can it wait until Thomas comes back and 
>> > starts processing patches again?
>> 
>> Without it booting on EFI can't be expected to work (and while I 
>> was really sure I tested this before submitting, I apparently must 
>> have used a stale kernel then - I'm really sorry for that), so 
>> getting it applied soon would be rather desirable.
>> 
>> But then again I don't seem to be able to boot 3.0.1 with both 
>> patches applied on my only EFI box, so there must be something else 
>> that broke, and hence maybe I really did test it (on 3.0-rc7) and 
>> it worked then. Debugging...
> 
> Ok, once you figure it out we can merge all fixes upstream ASAP.

So this is a combination of two recent events: In my patch set, I
recently removed efi.c's X86_32 conditional around setting
x86_platform.[gs]et_wallclock. Without that removal, the deadlock
wouldn't occur (because mach_get_cmos_time() would get called
instead) on 64-bits, which is the only thing I run.

With that conditional removed, Matthew's NX consolidation patch
gets into the way: For the physical call path it is simply wrong
(correct only for the phys_efi_set_virtual_address_map() call, as
there the memory map got virtual addresses assigned already) -
it switches from early_runtime_code_mapping_set_exec()'s using
of md->phys_addr to efi_set_executable() using md->virt_addr.
But the latter isn't set at that point yet. And it's unclear to me why
this works on 32-bit.

Bottom line - the fix here is needed for 32-bit booting only (and
hence isn't - afaic - as critical as I first thought). But the adjustment
I'm holding is a necessary one (and I don't even understand why,
when 32- and 64-bit EFI code got merged, this was left 32-bit only),
and the NX consolidation change needs to be fixed too (basically as
a prerequisite).

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Wed, 2011-08-10 at 14:12 +0100, Jan Beulich wrote:
> Bottom line - the fix here is needed for 32-bit booting only (and
> hence isn't - afaic - as critical as I first thought). But the adjustment
> I'm holding is a necessary one (and I don't even understand why,
> when 32- and 64-bit EFI code got merged, this was left 32-bit only),
> and the NX consolidation change needs to be fixed too (basically as
> a prerequisite).

But the rtc-lock patch is independent and can be merged on its own,
right? Sure the other issues need fixing but there's no sense in holding
up the rtc-lock patch, since with it applied my machine actually boots.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> On 15.08.11 at 20:18, Matt Fleming <matt@console-pimps.org> wrote:
> On Wed, 2011-08-10 at 14:12 +0100, Jan Beulich wrote:
>> Bottom line - the fix here is needed for 32-bit booting only (and
>> hence isn't - afaic - as critical as I first thought). But the adjustment
>> I'm holding is a necessary one (and I don't even understand why,
>> when 32- and 64-bit EFI code got merged, this was left 32-bit only),
>> and the NX consolidation change needs to be fixed too (basically as
>> a prerequisite).
> 
> But the rtc-lock patch is independent and can be merged on its own,
> right? Sure the other issues need fixing but there's no sense in holding
> up the rtc-lock patch, since with it applied my machine actually boots.

Oh, yes, absolutely - I'm actually waiting for this one to get applied so
that the enabling of the use of the EFI RTC routines on x86-64 won't
cause the same problem there.

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Tue, 2011-08-16 at 07:22 +0100, Jan Beulich wrote:
> >>> On 15.08.11 at 20:18, Matt Fleming <matt@console-pimps.org> wrote:
> > On Wed, 2011-08-10 at 14:12 +0100, Jan Beulich wrote:
> >> Bottom line - the fix here is needed for 32-bit booting only (and
> >> hence isn't - afaic - as critical as I first thought). But the adjustment
> >> I'm holding is a necessary one (and I don't even understand why,
> >> when 32- and 64-bit EFI code got merged, this was left 32-bit only),
> >> and the NX consolidation change needs to be fixed too (basically as
> >> a prerequisite).
> > 
> > But the rtc-lock patch is independent and can be merged on its own,
> > right? Sure the other issues need fixing but there's no sense in holding
> > up the rtc-lock patch, since with it applied my machine actually boots.
> 
> Oh, yes, absolutely - I'm actually waiting for this one to get applied so
> that the enabling of the use of the EFI RTC routines on x86-64 won't
> cause the same problem there.

OK cool, thanks Jan. I suspect Ingo will pick this up, or Thomas once
he's worked through his email backlog ;-)

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matthew Garrett]

Could this be applied? 32-bit EFI boot is currently completely broken.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Matt Fleming]

On Tue, 2011-08-30 at 16:45 +0100, Matthew Garrett wrote:
> Could this be applied? 32-bit EFI boot is currently completely broken.

Thanks for the reminder Matthew, I'll resend this now.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[H. Peter Anvin]

On 08/30/2011 09:07 AM, Matt Fleming wrote:
> On Tue, 2011-08-30 at 16:45 +0100, Matthew Garrett wrote:
>> Could this be applied? 32-bit EFI boot is currently completely broken.
> 
> Thanks for the reminder Matthew, I'll resend this now.

Point me to the current version and I'll put it in.

	-hpa

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Jan Beulich]

>>> On 10.08.11 at 15:12, Jan Beulich wrote:
> >>> On 10.08.11 at 11:51, Ingo Molnar <mingo@elte.hu> wrote:
> > * Jan Beulich <JBeulich@novell.com> wrote:
> >> But then again I don't seem to be able to boot 3.0.1 with both 
> >> patches applied on my only EFI box, so there must be something else 
> >> that broke, and hence maybe I really did test it (on 3.0-rc7) and 
> >> it worked then. Debugging...
> > 
> > Ok, once you figure it out we can merge all fixes upstream ASAP.
> 
> So this is a combination of two recent events: In my patch set, I
> recently removed efi.c's X86_32 conditional around setting
> x86_platform.[gs]et_wallclock. Without that removal, the deadlock
> wouldn't occur (because mach_get_cmos_time() would get called
> instead) on 64-bits, which is the only thing I run.
> 
> With that conditional removed, Matthew's NX consolidation patch
> gets into the way: For the physical call path it is simply wrong
> (correct only for the phys_efi_set_virtual_address_map() call, as
> there the memory map got virtual addresses assigned already) -
> it switches from early_runtime_code_mapping_set_exec()'s using
> of md->phys_addr to efi_set_executable() using md->virt_addr.
> But the latter isn't set at that point yet. And it's unclear to me why
> this works on 32-bit.

Worse - even the original code would not have worked afaict. Calling
set_memory_{,n}x() this early cannot work without a small adjustment
to the CPA logic, as all of the flushing functions have
BUG_ON(irqs_disabled()) at their beginning.

Jan

Re: [PATCH] x86, efi: Don't recursively acquire rtc_lock

[Avi Kivity]

On 08/08/2011 04:40 PM, Matt Fleming wrote:
> On Fri, 2011-08-05 at 18:04 +0100, Jan Beulich wrote:
> >
> >  Virtual platforms will have to take care of the serialization in the
> >  host anyway, so the guest side implementation of getwallclock et al
> >  is entirely unaffected.
>
> Ah, OK, that's the important part. I didn't realise that rtc_lock isn't
> actually required by any other code. In which case, yes, it completely
> makes sense to push the locking of rtc_lock down into the
> implementations that actually need it.
>
> It'd be great if I could get some ACK's from the virtualization guys.

Ack (for that aspect).

-- 
error compiling committee.c: too many arguments to function