[PATCH] x86-64: use EFI to deal with platform wall clock

[PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

Other than ix86, x86-64 on EFI so far didn't set the {g,s}et_wallclock
accessors to the EFI routines, thus incorrectly using raw RTC accesses
instead.

Simply removing the #ifdef around the respective code isn't enough,
however: While so far early get-time calls were done in physical mode,
this doesn't work properly for x86-64, as virtual addresses would still
need to be set up for all runtime regions (which wasn't the case on the
system I have access to), so instead the patch moves the call to
efi_enter_virtual_mode() ahead (which in turn allows to drop all code
related to calling efi-get-time in physical mode).

Additionally the earlier calling of efi_set_executable() requires the
CPA code to cope, i.e. during early boot it must be avoided to call
cpa_flush_array(), as the first thing this function does is a
BUG_ON(irqs_disabled()).

Also make the two EFI functions in question here static - they're not
being referenced elsewhere.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>

---
(Note that due to the lack of hardware I wasn't able to test this on
32-bit EFI - Matt offered to do so subsequently.)

---
 arch/x86/mm/pageattr.c      |   10 ++++++----
 arch/x86/platform/efi/efi.c |   30 ++++--------------------------
 include/linux/efi.h         |    2 --
 init/main.c                 |    8 ++++----
 4 files changed, 14 insertions(+), 36 deletions(-)

--- 3.4/arch/x86/mm/pageattr.c
+++ 3.4-x86_64-EFI-time/arch/x86/mm/pageattr.c
@@ -919,11 +919,13 @@ static int change_page_attr_set_clr(unsi
 
 	/*
 	 * On success we use clflush, when the CPU supports it to
-	 * avoid the wbindv. If the CPU does not support it and in the
-	 * error case we fall back to cpa_flush_all (which uses
-	 * wbindv):
+	 * avoid the wbindv. If the CPU does not support it, in the
+	 * error case, and during early boot (for EFI) we fall back
+	 * to cpa_flush_all (which uses wbinvd):
 	 */
-	if (!ret && cpu_has_clflush) {
+	if (early_boot_irqs_disabled)
+		__cpa_flush_all((void *)(long)cache);
+	else if (!ret && cpu_has_clflush) {
 		if (cpa.flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
 			cpa_flush_array(addr, numpages, cache,
 					cpa.flags, pages);
--- 3.4/arch/x86/platform/efi/efi.c
+++ 3.4-x86_64-EFI-time/arch/x86/platform/efi/efi.c
@@ -234,22 +234,7 @@ static efi_status_t __init phys_efi_set_
 	return status;
 }
 
-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, virt_to_phys(tm),
-				virt_to_phys(tc));
-	efi_call_phys_epilog();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-int efi_set_rtc_mmss(unsigned long nowtime)
+static int efi_set_rtc_mmss(unsigned long nowtime)
 {
 	int real_seconds, real_minutes;
 	efi_status_t 	status;
@@ -278,7 +263,7 @@ int efi_set_rtc_mmss(unsigned long nowti
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+static unsigned long efi_get_time(void)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -621,18 +606,13 @@ static int __init efi_runtime_init(void)
 	}
 	/*
 	 * We will only need *early* access to the following
-	 * two EFI runtime services before set_virtual_address_map
+	 * EFI runtime service before set_virtual_address_map
 	 * is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 		(efi_set_virtual_address_map_t *)
 		runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
+
 	early_iounmap(runtime, sizeof(efi_runtime_services_t));
 
 	return 0;
@@ -720,12 +700,10 @@ void __init efi_init(void)
 		efi_enabled = 0;
 		return;
 	}
-#ifdef CONFIG_X86_32
 	if (efi_native) {
 		x86_platform.get_wallclock = efi_get_time;
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
-#endif
 
 #if EFI_DEBUG
 	print_efi_memmap();
--- 3.4/include/linux/efi.h
+++ 3.4-x86_64-EFI-time/include/linux/efi.h
@@ -503,8 +503,6 @@ extern u64 efi_mem_attribute (unsigned l
 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 int efi_set_rtc_mmss(unsigned long nowtime);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
 
--- 3.4/init/main.c
+++ 3.4-x86_64-EFI-time/init/main.c
@@ -460,6 +460,10 @@ static void __init mm_init(void)
 	percpu_init_late();
 	pgtable_cache_init();
 	vmalloc_init();
+#ifdef CONFIG_X86
+	if (efi_enabled)
+		efi_enter_virtual_mode();
+#endif
 }
 
 asmlinkage void __init start_kernel(void)
@@ -604,10 +608,6 @@ asmlinkage void __init start_kernel(void
 	calibrate_delay();
 	pidmap_init();
 	anon_vma_init();
-#ifdef CONFIG_X86
-	if (efi_enabled)
-		efi_enter_virtual_mode();
-#endif
 	thread_info_cache_init();
 	cred_init();
 	fork_init(totalram_pages);

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Fri, May 25, 2012 at 04:20:31PM +0100, Jan Beulich wrote:

> Also make the two EFI functions in question here static - they're not
> being referenced elsewhere.

Looks good, but should we also be changing get_time to return in UTC? 
What are the expected semantics for get_wallclock?

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

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 25.05.12 at 17:24, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Fri, May 25, 2012 at 04:20:31PM +0100, Jan Beulich wrote:
> 
>> Also make the two EFI functions in question here static - they're not
>> being referenced elsewhere.
> 
> Looks good, but should we also be changing get_time to return in UTC? 

Imo that would be a separate change though.

> What are the expected semantics for get_wallclock?

Not sure - I think both are permitted (just like for the legacy RTC),
with the expectation that early user mode would tell the kernel.

Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Fri, May 25, 2012 at 04:30:11PM +0100, Jan Beulich wrote:
> >>> On 25.05.12 at 17:24, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Looks good, but should we also be changing get_time to return in UTC? 
> 
> Imo that would be a separate change though.

Sure, but probably a useful one. I've had complaints from firmware 
vendors that we're not handling the timezone properly here.

> > What are the expected semantics for get_wallclock?
> 
> Not sure - I think both are permitted (just like for the legacy RTC),
> with the expectation that early user mode would tell the kernel.

Yeah. I think get_time and set_time should probably both be paying 
attention to sys_tz and converting appropriately.

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

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matt Fleming]

On Fri, 2012-05-25 at 16:20 +0100, Jan Beulich wrote:
> Other than ix86, x86-64 on EFI so far didn't set the {g,s}et_wallclock
> accessors to the EFI routines, thus incorrectly using raw RTC accesses
> instead.
> 
> Simply removing the #ifdef around the respective code isn't enough,
> however: While so far early get-time calls were done in physical mode,
> this doesn't work properly for x86-64, as virtual addresses would still
> need to be set up for all runtime regions (which wasn't the case on the
> system I have access to), so instead the patch moves the call to
> efi_enter_virtual_mode() ahead (which in turn allows to drop all code
> related to calling efi-get-time in physical mode).
> 
> Additionally the earlier calling of efi_set_executable() requires the
> CPA code to cope, i.e. during early boot it must be avoided to call
> cpa_flush_array(), as the first thing this function does is a
> BUG_ON(irqs_disabled()).
> 
> Also make the two EFI functions in question here static - they're not
> being referenced elsewhere.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> 
> ---
> (Note that due to the lack of hardware I wasn't able to test this on
> 32-bit EFI - Matt offered to do so subsequently.)

Appears to work fine here on 32-bit.

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 26.05.12 at 12:26, Matt Fleming <matt.fleming@intel.com> wrote:
> On Fri, 2012-05-25 at 16:20 +0100, Jan Beulich wrote:
>> Other than ix86, x86-64 on EFI so far didn't set the {g,s}et_wallclock
>> accessors to the EFI routines, thus incorrectly using raw RTC accesses
>> instead.
>> 
>> Simply removing the #ifdef around the respective code isn't enough,
>> however: While so far early get-time calls were done in physical mode,
>> this doesn't work properly for x86-64, as virtual addresses would still
>> need to be set up for all runtime regions (which wasn't the case on the
>> system I have access to), so instead the patch moves the call to
>> efi_enter_virtual_mode() ahead (which in turn allows to drop all code
>> related to calling efi-get-time in physical mode).
>> 
>> Additionally the earlier calling of efi_set_executable() requires the
>> CPA code to cope, i.e. during early boot it must be avoided to call
>> cpa_flush_array(), as the first thing this function does is a
>> BUG_ON(irqs_disabled()).
>> 
>> Also make the two EFI functions in question here static - they're not
>> being referenced elsewhere.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Matt Fleming <matt.fleming@intel.com>
>> Cc: Matthew Garrett <mjg@redhat.com>
>> 
>> ---
>> (Note that due to the lack of hardware I wasn't able to test this on
>> 32-bit EFI - Matt offered to do so subsequently.)
> 
> Appears to work fine here on 32-bit.

Is there any further action required on my part here? Is it clear
through who's hands this would need to go in order to make it in
for 3.6 at least?

Thanks, Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Ingo Molnar]

* Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Fri, May 25, 2012 at 04:30:11PM +0100, Jan Beulich wrote:
> > >>> On 25.05.12 at 17:24, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > Looks good, but should we also be changing get_time to return in UTC? 
> > 
> > Imo that would be a separate change though.
> 
> Sure, but probably a useful one. I've had complaints from firmware 
> vendors that we're not handling the timezone properly here.

I converted this bit of the discussion into:

Acked-by: Matthew Garrett <mjg@redhat.com>

for the original patch. Holler if that's wrong.

Thanks,

	Ingo

[tip:x86/efi] x86-64/efi: Use EFI to deal with platform wall clock

[tip-bot for Jan Beulich]

Commit-ID:  bacef661acdb634170a8faddbc1cf28e8f8b9eee
Gitweb:     http://git.kernel.org/tip/bacef661acdb634170a8faddbc1cf28e8f8b9eee
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Fri, 25 May 2012 16:20:31 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Jun 2012 11:48:05 +0200

x86-64/efi: Use EFI to deal with platform wall clock

Other than ix86, x86-64 on EFI so far didn't set the
{g,s}et_wallclock accessors to the EFI routines, thus
incorrectly using raw RTC accesses instead.

Simply removing the #ifdef around the respective code isn't
enough, however: While so far early get-time calls were done in
physical mode, this doesn't work properly for x86-64, as virtual
addresses would still need to be set up for all runtime regions
(which wasn't the case on the system I have access to), so
instead the patch moves the call to efi_enter_virtual_mode()
ahead (which in turn allows to drop all code related to calling
efi-get-time in physical mode).

Additionally the earlier calling of efi_set_executable()
requires the CPA code to cope, i.e. during early boot it must be
avoided to call cpa_flush_array(), as the first thing this
function does is a BUG_ON(irqs_disabled()).

Also make the two EFI functions in question here static -
they're not being referenced elsewhere.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Matt Fleming <matt.fleming@intel.com>
Acked-by: Matthew Garrett <mjg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/4FBFBF5F020000780008637F@nat28.tlf.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/pageattr.c      |   10 ++++++----
 arch/x86/platform/efi/efi.c |   30 ++++--------------------------
 include/linux/efi.h         |    2 --
 init/main.c                 |    8 ++++----
 4 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index e1ebde3..ee09aca 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -919,11 +919,13 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages,
 
 	/*
 	 * On success we use clflush, when the CPU supports it to
-	 * avoid the wbindv. If the CPU does not support it and in the
-	 * error case we fall back to cpa_flush_all (which uses
-	 * wbindv):
+	 * avoid the wbindv. If the CPU does not support it, in the
+	 * error case, and during early boot (for EFI) we fall back
+	 * to cpa_flush_all (which uses wbinvd):
 	 */
-	if (!ret && cpu_has_clflush) {
+	if (early_boot_irqs_disabled)
+		__cpa_flush_all((void *)(long)cache);
+	else if (!ret && cpu_has_clflush) {
 		if (cpa.flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
 			cpa_flush_array(addr, numpages, cache,
 					cpa.flags, pages);
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 92660eda..2dc29f5 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -234,22 +234,7 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 	return status;
 }
 
-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, virt_to_phys(tm),
-				virt_to_phys(tc));
-	efi_call_phys_epilog();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-int efi_set_rtc_mmss(unsigned long nowtime)
+static int efi_set_rtc_mmss(unsigned long nowtime)
 {
 	int real_seconds, real_minutes;
 	efi_status_t 	status;
@@ -278,7 +263,7 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+static unsigned long efi_get_time(void)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -621,18 +606,13 @@ static int __init efi_runtime_init(void)
 	}
 	/*
 	 * We will only need *early* access to the following
-	 * two EFI runtime services before set_virtual_address_map
+	 * EFI runtime service before set_virtual_address_map
 	 * is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 		(efi_set_virtual_address_map_t *)
 		runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
+
 	early_iounmap(runtime, sizeof(efi_runtime_services_t));
 
 	return 0;
@@ -720,12 +700,10 @@ void __init efi_init(void)
 		efi_enabled = 0;
 		return;
 	}
-#ifdef CONFIG_X86_32
 	if (efi_native) {
 		x86_platform.get_wallclock = efi_get_time;
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
-#endif
 
 #if EFI_DEBUG
 	print_efi_memmap();
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec45ccd..103adc6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -503,8 +503,6 @@ 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 int efi_set_rtc_mmss(unsigned long nowtime);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
 
diff --git a/init/main.c b/init/main.c
index 1ca6b32..eef3012 100644
--- a/init/main.c
+++ b/init/main.c
@@ -460,6 +460,10 @@ static void __init mm_init(void)
 	percpu_init_late();
 	pgtable_cache_init();
 	vmalloc_init();
+#ifdef CONFIG_X86
+	if (efi_enabled)
+		efi_enter_virtual_mode();
+#endif
 }
 
 asmlinkage void __init start_kernel(void)
@@ -601,10 +605,6 @@ asmlinkage void __init start_kernel(void)
 	calibrate_delay();
 	pidmap_init();
 	anon_vma_init();
-#ifdef CONFIG_X86
-	if (efi_enabled)
-		efi_enter_virtual_mode();
-#endif
 	thread_info_cache_init();
 	cred_init();
 	fork_init(totalram_pages);

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 16.05.12 at 14:39, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, May 16, 2012 at 01:18:28PM +0100, Jan Beulich wrote:
>> So I'm afraid if the patch as I provided it isn't acceptable, and
>> if the call to efi_enter_virtual_mode() can't be moved ahead
>> of the one to timekeeping_init(), this winds down to the whole
>> logic needing a re-write.
> 
> I have zero objection to this being cleaned up, but I don't know of any 
> obvious reason why we can't do enter_virtual_mode() earlier.

So this indeed works (moving enter_virual_mode() invocation
to the end of mm_init(), which seems to most logical place to
me) on the single system I can try this out on. I'll submit an
updated patch soon.

Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matt Fleming]

On Wed, 2012-05-16 at 13:59 +0100, Jan Beulich wrote:
> I shall give that a try then. It would imply that we don't need
> phys_efi_get_time() then anymore. I don't have a 32-bit EFI box
> though to test that things don't break there...

I can test out 32-bit EFI, or at least find someone with access to such
a system.

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Wed, May 16, 2012 at 01:59:57PM +0100, Jan Beulich wrote:
> >>> On 16.05.12 at 14:39, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Could you elaborate on that a little?
> 
> There are systems where RAM on individual nodes is always
> starting at e.g. a 1Tb boundary. Obviously there can (at
> least theoretically) be anything in between, and hence
> assuming that __va() is usable here is simply wrong, as likely
> no mapping was created at all for the hole space (or if there
> is one, it would conflict with the one to be established here
> in the EfiMemoryMappedIO case).

Ok, that does sound like it needs fixing.

> > Platforms don't correctly deal with the case where you make physical 
> > calls after ExitBootServices(). We tried running in physical mode. It 
> > simply doesn't work.
> 
> Interesting, especially as we're using physical mode exclusively so
> far in Xen. That's a platform issue though, and working around
> it by (silently!) sacrificing other functionality is questionable imo.
> It should at best be an option (default off), so that on systems
> where physical mode works, kexec can work too.

There was a patchset posted that provided that option. We experimented 
with it in RHEL for a while and found that physical mode simply isn't 
reliable - no other OS uses it, so it's entirely untested on most 
platforms.

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

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 16.05.12 at 14:39, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, May 16, 2012 at 01:18:28PM +0100, Jan Beulich wrote:
> 
>> Okay, looks like calling efi_ioremap() at this point is possible.
>> But why is efi_enter_virtual_mode() being called as late as is
>> the case currently for x86 anyway?
> 
> Assuming that things are as they are for a good reason is not 
> necessarily true in the EFI code...

Okay...

>> And then again the current logic in efi_enter_virtual_mode()
>> looks flawed (it assumes two contiguous pieces of direct
>> mappings, and while on systems with dis-contiguous physical
>> memory this currently appears to be true, it's not correct - the
>> holes could have MMIO assignments in them - and hence
>> shouldn't be relied upon), and I wouldn't want to copy this
>> elsewhere.
> 
> Could you elaborate on that a little?

There are systems where RAM on individual nodes is always
starting at e.g. a 1Tb boundary. Obviously there can (at
least theoretically) be anything in between, and hence
assuming that __va() is usable here is simply wrong, as likely
no mapping was created at all for the hole space (or if there
is one, it would conflict with the one to be established here
in the EfiMemoryMappedIO case).

>> Plus the use of set_virtual_address_map is bogus in the first
>> place, as it makes it impossible for a kexec-ed kernel to also
>> use EFI services (as it would have to call the function a
>> second and possibly third time, yet it is not permitted to be
>> called more than once). Imo all calls have to happen in
>> physical mode.
> 
> Platforms don't correctly deal with the case where you make physical 
> calls after ExitBootServices(). We tried running in physical mode. It 
> simply doesn't work.

Interesting, especially as we're using physical mode exclusively so
far in Xen. That's a platform issue though, and working around
it by (silently!) sacrificing other functionality is questionable imo.
It should at best be an option (default off), so that on systems
where physical mode works, kexec can work too.

>> So I'm afraid if the patch as I provided it isn't acceptable, and
>> if the call to efi_enter_virtual_mode() can't be moved ahead
>> of the one to timekeeping_init(), this winds down to the whole
>> logic needing a re-write.
> 
> I have zero objection to this being cleaned up, but I don't know of any 
> obvious reason why we can't do enter_virtual_mode() earlier.

I shall give that a try then. It would imply that we don't need
phys_efi_get_time() then anymore. I don't have a 32-bit EFI box
though to test that things don't break there...

Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Wed, May 16, 2012 at 01:18:28PM +0100, Jan Beulich wrote:

> Okay, looks like calling efi_ioremap() at this point is possible.
> But why is efi_enter_virtual_mode() being called as late as is
> the case currently for x86 anyway?

Assuming that things are as they are for a good reason is not 
necessarily true in the EFI code...

> And then again the current logic in efi_enter_virtual_mode()
> looks flawed (it assumes two contiguous pieces of direct
> mappings, and while on systems with dis-contiguous physical
> memory this currently appears to be true, it's not correct - the
> holes could have MMIO assignments in them - and hence
> shouldn't be relied upon), and I wouldn't want to copy this
> elsewhere.

Could you elaborate on that a little?

> Plus the use of set_virtual_address_map is bogus in the first
> place, as it makes it impossible for a kexec-ed kernel to also
> use EFI services (as it would have to call the function a
> second and possibly third time, yet it is not permitted to be
> called more than once). Imo all calls have to happen in
> physical mode.

Platforms don't correctly deal with the case where you make physical 
calls after ExitBootServices(). We tried running in physical mode. It 
simply doesn't work.

> So I'm afraid if the patch as I provided it isn't acceptable, and
> if the call to efi_enter_virtual_mode() can't be moved ahead
> of the one to timekeeping_init(), this winds down to the whole
> logic needing a re-write.

I have zero objection to this being cleaned up, but I don't know of any 
obvious reason why we can't do enter_virtual_mode() earlier.

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

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 15.05.12 at 15:20, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, May 15, 2012 at 02:19:07PM +0100, Jan Beulich wrote:
> 
>> I would have expected that things work that way, but they
>> don't. In particular is the function in efi_64.c that's being
>> modified here called from efi_call_phys_{pro,epi}log(), and at
>> that point we can't expect virtual addresses to be uniformly
>> set yet. So it's a physical call that requires the fixup done
>> here, as efi_set_executable() simply expects ->virt_addr to
>> be valid. I suspect that no physical calls other than
>> phys_efi_set_virtual_address_map() were being done so far
>> at all on 64-bit, hiding the problem.
> 
> In that case we need to split the mapping code into two chunks and 
> configure the memory map earlier. You can't depend on __va() doing 
> anything useful on runtime addresses.

Okay, looks like calling efi_ioremap() at this point is possible.
But why is efi_enter_virtual_mode() being called as late as is
the case currently for x86 anyway?

And then again the current logic in efi_enter_virtual_mode()
looks flawed (it assumes two contiguous pieces of direct
mappings, and while on systems with dis-contiguous physical
memory this currently appears to be true, it's not correct - the
holes could have MMIO assignments in them - and hence
shouldn't be relied upon), and I wouldn't want to copy this
elsewhere.

Similarly for efi_ioremap() itself - it neither honors the
cacheability requested by the firmware (calling
set_memory_uc() subsequently isn't sufficient, as the already
established mappings may be used in prefetches already),
nor does its self-recursion look very reliable (why would
init_memory_mapping() do any better on a second call,
and the code doesn't deal with last_map_pfn pointing below
or precisely at phys_addr at all).

Plus the use of set_virtual_address_map is bogus in the first
place, as it makes it impossible for a kexec-ed kernel to also
use EFI services (as it would have to call the function a
second and possibly third time, yet it is not permitted to be
called more than once). Imo all calls have to happen in
physical mode.

So I'm afraid if the patch as I provided it isn't acceptable, and
if the call to efi_enter_virtual_mode() can't be moved ahead
of the one to timekeeping_init(), this winds down to the whole
logic needing a re-write.

Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Tue, May 15, 2012 at 02:19:07PM +0100, Jan Beulich wrote:

> I would have expected that things work that way, but they
> don't. In particular is the function in efi_64.c that's being
> modified here called from efi_call_phys_{pro,epi}log(), and at
> that point we can't expect virtual addresses to be uniformly
> set yet. So it's a physical call that requires the fixup done
> here, as efi_set_executable() simply expects ->virt_addr to
> be valid. I suspect that no physical calls other than
> phys_efi_set_virtual_address_map() were being done so far
> at all on 64-bit, hiding the problem.

In that case we need to split the mapping code into two chunks and 
configure the memory map earlier. You can't depend on __va() doing 
anything useful on runtime addresses.

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

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

>>> On 15.05.12 at 14:47, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, May 15, 2012 at 01:18:19PM +0100, Jan Beulich wrote:
> 
>> Simply removing the #ifdef around the respective code isn't enough,
>> however: The runtime code must not only be forced to be executable, it
>> also must have a proper virtual address set (which on at least the
>> system I'm testing on isn't the case for all necessary regions, or at
>> least not as early as they're now being required).
> 
> I don't understand this. The get_time pointer won't be updated to the 
> virtual function until the end of efi_enter_virtual_mode, at which point 
> all runtime regions should have a virtual address mapped. We also call 
> runtime_code_page_mkexec() immediately after updating that pointer, 
> although maybe the order should be swapped. So I think the bug you're 
> fixing is not the bug you think you're fixing...

I would have expected that things work that way, but they
don't. In particular is the function in efi_64.c that's being
modified here called from efi_call_phys_{pro,epi}log(), and at
that point we can't expect virtual addresses to be uniformly
set yet. So it's a physical call that requires the fixup done
here, as efi_set_executable() simply expects ->virt_addr to
be valid. I suspect that no physical calls other than
phys_efi_set_virtual_address_map() were being done so far
at all on 64-bit, hiding the problem.

Jan

Re: [PATCH] x86-64: use EFI to deal with platform wall clock

[Matthew Garrett]

On Tue, May 15, 2012 at 01:18:19PM +0100, Jan Beulich wrote:

> Simply removing the #ifdef around the respective code isn't enough,
> however: The runtime code must not only be forced to be executable, it
> also must have a proper virtual address set (which on at least the
> system I'm testing on isn't the case for all necessary regions, or at
> least not as early as they're now being required).

I don't understand this. The get_time pointer won't be updated to the 
virtual function until the end of efi_enter_virtual_mode, at which point 
all runtime regions should have a virtual address mapped. We also call 
runtime_code_page_mkexec() immediately after updating that pointer, 
although maybe the order should be swapped. So I think the bug you're 
fixing is not the bug you think you're fixing...

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

[PATCH] x86-64: use EFI to deal with platform wall clock

[Jan Beulich]

Other than ix86, x86-64 on EFI so far didn't set the {g,s}et_wallclock
accessors to the EFI routines, thus incorrectly using raw RTC accesses
instead.

Simply removing the #ifdef around the respective code isn't enough,
however: The runtime code must not only be forced to be executable, it
also must have a proper virtual address set (which on at least the
system I'm testing on isn't the case for all necessary regions, or at
least not as early as they're now being required).

The earlier calling of efi_set_executable() in turn require the CPA
code to cope, i.e. during early boot it must be avoided to call
cpa_flush_array(), as the first thing this function does is a
BUG_ON(irqs_disabled()).

Also make the two EFI functions in question here static - they're not
being referenced elsewhere.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>

---
 arch/x86/mm/pageattr.c         |   10 ++++++----
 arch/x86/platform/efi/efi.c    |    6 ++----
 arch/x86/platform/efi/efi_64.c |   10 +++++++++-
 include/linux/efi.h            |    2 --
 4 files changed, 17 insertions(+), 11 deletions(-)

--- 3.4-rc7/arch/x86/mm/pageattr.c
+++ 3.4-rc7-x86_64-EFI-time/arch/x86/mm/pageattr.c
@@ -919,11 +919,13 @@ static int change_page_attr_set_clr(unsi
 
 	/*
 	 * On success we use clflush, when the CPU supports it to
-	 * avoid the wbindv. If the CPU does not support it and in the
-	 * error case we fall back to cpa_flush_all (which uses
-	 * wbindv):
+	 * avoid the wbindv. If the CPU does not support it, in the
+	 * error case, and during early boot (for EFI) we fall back
+	 * to cpa_flush_all (which uses wbinvd):
 	 */
-	if (!ret && cpu_has_clflush) {
+	if (early_boot_irqs_disabled)
+		__cpa_flush_all((void *)(long)cache);
+	else if (!ret && cpu_has_clflush) {
 		if (cpa.flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
 			cpa_flush_array(addr, numpages, cache,
 					cpa.flags, pages);
--- 3.4-rc7/arch/x86/platform/efi/efi.c
+++ 3.4-rc7-x86_64-EFI-time/arch/x86/platform/efi/efi.c
@@ -249,7 +249,7 @@ static efi_status_t __init phys_efi_get_
 	return status;
 }
 
-int efi_set_rtc_mmss(unsigned long nowtime)
+static int efi_set_rtc_mmss(unsigned long nowtime)
 {
 	int real_seconds, real_minutes;
 	efi_status_t 	status;
@@ -278,7 +278,7 @@ int efi_set_rtc_mmss(unsigned long nowti
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+static unsigned long efi_get_time(void)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -720,12 +720,10 @@ void __init efi_init(void)
 		efi_enabled = 0;
 		return;
 	}
-#ifdef CONFIG_X86_32
 	if (efi_native) {
 		x86_platform.get_wallclock = efi_get_time;
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
-#endif
 
 #if EFI_DEBUG
 	print_efi_memmap();
--- 3.4-rc7/arch/x86/platform/efi/efi_64.c
+++ 3.4-rc7-x86_64-EFI-time/arch/x86/platform/efi/efi_64.c
@@ -53,8 +53,16 @@ static void __init early_code_mapping_se
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
 		if (md->type == EFI_RUNTIME_SERVICES_CODE ||
-		    md->type == EFI_BOOT_SERVICES_CODE)
+		    md->type == EFI_BOOT_SERVICES_CODE) {
+			if (!md->virt_addr) {
+				/*
+				 * We have to hope that the mappings set up so
+				 * far cover all that's needed for the call.
+				 */
+				md->virt_addr = (u64)__va(md->phys_addr);
+			}
 			efi_set_executable(md, executable);
+		}
 	}
 }
 
--- 3.4-rc7/include/linux/efi.h
+++ 3.4-rc7-x86_64-EFI-time/include/linux/efi.h
@@ -503,8 +503,6 @@ extern u64 efi_mem_attribute (unsigned l
 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 int efi_set_rtc_mmss(unsigned long nowtime);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;