x86/efi: use GFP_ATOMIC under spin_lock

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +0 -0

index 0c2a234fef1e..f5adcadb381b 100644

Quoted 17 lines · Dan Carpenter

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +1 -1

index 0c2a234fef1e..f5adcadb381b 100644
@@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
 	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
-	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
 

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_prelog(void) · line 96

Quoted 2 lines

arch/x86/platform/efi/efi_64.c +0 -0

 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

The allocation there, if it happens within a spinlocked path, is probably a layering violation - and GFP_ATOMIC is at best a workaround.

Quoted 20 lines · Ingo Molnar

Quoted 17 lines · Dan Carpenter

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +1 -1

index 0c2a234fef1e..f5adcadb381b 100644
@@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
 	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
-	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
 

Quoted 7 lines

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_prelog(void) · line 96

Quoted 2 lines

arch/x86/platform/efi/efi_64.c +0 -0

 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

The allocation there, if it happens within a spinlocked path, is probably a layering violation - and GFP_ATOMIC is at best a workaround.

You're on your own for fixing the complicated stuff like layering violations. I just do static checker stuff and sed fixes. ;)

Quoted 29 lines · Dan Carpenter

Quoted 27 lines · Ingo Molnar

Quoted 11 lines · Dan Carpenter

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +0 -0

index 0c2a234fef1e..f5adcadb381b 100644

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_prelog(void) · line 96

Quoted 8 lines

arch/x86/platform/efi/efi_64.c +1 -1

@@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
 	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
-	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
 
 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

The allocation there, if it happens within a spinlocked path, is probably a layering violation - and GFP_ATOMIC is at best a workaround.

Quoted 2 lines

You're on your own for fixing the complicated stuff like layering violations. I just do static checker stuff and sed fixes. ;)

It's a generic pattern: GFP_KERNEL -> GFP_ATOMIC fixes are almost always wrong.

You're on your own for fixing the complicated stuff like layering violations. I just do static checker stuff and sed fixes. ;)

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

Quoted 5 lines · Matt Fleming

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

I'd naively expected that these would be more reliable after the 1:1 mapping patches, so it might actually be time to give them another go.

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

I can check again, but I'm pretty sure this ASUS machine under my desk always returns a vendor-specific error code when invoked, even with Borislav's 1:1 patches. Enabling EFI services just because they exist hasn't worked out well for us in the past.

And of course, the GPF_KERNEL alloc under spinlock bug that started this thread needs to be fixed.

Quoted 3 lines · Matt Fleming

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

In theory we get timezones, which we otherwise only get if we have an ACPI TAD device.

Quoted 4 lines

I can check again, but I'm pretty sure this ASUS machine under my desk always returns a vendor-specific error code when invoked, even with Borislav's 1:1 patches. Enabling EFI services just because they exist hasn't worked out well for us in the past.

Returning an error doesn't seem like a problem, as long as it's not actually killing the machine in the process...

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 8 lines · Matthew Garrett

Quoted 5 lines · Matt Fleming

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

Quoted 2 lines

I'd naively expected that these would be more reliable after the 1:1 mapping patches, so it might actually be time to give them another go.

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

Quoted 14 lines

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 11 lines · Matthew Garrett

Quoted 5 lines · Matt Fleming

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

I'd naively expected that these would be more reliable after the 1:1 mapping patches, so it might actually be time to give them another go.

Quoted 3 lines

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

Is it such a bad thing to be prepared for this sort of machine to arrive even if likely there are none so far?

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 18 lines · Jan Beulich

Quoted 17 lines

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 11 lines · Matthew Garrett

Quoted 5 lines · Matt Fleming

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

I'd naively expected that these would be more reliable after the 1:1 mapping patches, so it might actually be time to give them another go.

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

Quoted 2 lines

Is it such a bad thing to be prepared for this sort of machine to arrive even if likely there are none so far?

"Be prepared for a not yet existing machine" != "time to give them another go on existing machines", right?

Quoted 22 lines

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 18 lines · Jan Beulich

Quoted 17 lines

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 11 lines · Matthew Garrett

Quoted 5 lines · Matt Fleming

We have tried to use the time functions before, with little success because of various bugs in the runtime implementations, e.g. see commit bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock") and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)").

I'd naively expected that these would be more reliable after the 1:1 mapping patches, so it might actually be time to give them another go.

Is there any value in that? Do machines exist where we absolutely must have access to the EFI time services? Either because there's no other method or no other working one?

Quoted 5 lines

Quoted 2 lines

Is it such a bad thing to be prepared for this sort of machine to arrive even if likely there are none so far?

"Be prepared for a not yet existing machine" != "time to give them another go on existing machines", right?

That heavily depends on the perspective you take, [...]

[...] and I know we tend to have disagreeing perspectives here: I think things should be done the intended way on all systems where this is possible. [...]

[...] Remember the far from insignificant number of issues with the ACPI interrupt source overrides for particularly the timer interrupt years ago? Which wasn't a reason to stop trying to use ACPI wherever possible, even on affected systems (by introducing command line overrides).

Quoted 6 lines

Quoted 1 line · Ingo Molnar

[...] Remember the far from insignificant number of issues with the ACPI interrupt source overrides for particularly the timer interrupt years ago? Which wasn't a reason to stop trying to use ACPI wherever possible, even on affected systems (by introducing command line overrides).

That's an apples to oranges comparison IMO, the motivation for that was real on real systems: to get high(er) resolution time and interrupt sources.

What's the upside here, for existing systems? Please don't mistake me:
if an upside exists I'm not against it, at all.

The upside isn't so much on existing systems that work with the intended model, but on those that don't: Increasing awareness and pressure on the firmware vendors' side to get their bugs fixed. Just like - afaict - the ratio of systems with broken ACPI tables has gone down over time.

Just to give an example from the Xen side: Xen uses the time interface in UEFI, as you would expect not without problems. Apart from issues with memory areas needed by those runtime calls not being properly marked for runtime use (which the respective vendors accepted they need to fix), we are also facing problems with runtime calls using XMM registers.

Rather than blindly saying "the specification isn't precise on this, and e.g. Linux has a half baked mechanism to deal with this, so let's just do so too" we're pushing for the specification to get updated, such that it becomes clear whether the firmware may do so and we need to change Xen, or whether it's the firmware people needing to fix their implementation. Doing it that way is cumbersome, yes, but I think it is in the best interest of everybody involved (and better than papering over bugs by avoiding certain functionality).

(The one real upside - getting time zone information - was already mentioned by someone else on this thread.)

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 6 lines · Jan Beulich

Just to give an example from the Xen side: Xen uses the time interface in UEFI, as you would expect not without problems. Apart from issues with memory areas needed by those runtime calls not being properly marked for runtime use (which the respective vendors accepted they need to fix), we are also facing problems with runtime calls using XMM registers.

Presumably you've got some kind of quirk mechanism to get things working?

That doesn't exist in the kernel so the time functions as-is are useless, right?

What's the benefit of using the EFI time services for Xen?

Does Xen use the current kernel functions directly?

Not sure why you call them useless. If wired up properly, they would be useful on systems where they work.

Which kernel functions? Perhaps just a misunderstanding - I gave that example with the actual hypervisor in mind, not the kernel side code sitting on top. The kernel, after all, can't directly call EFI runtime code when running on top of Xen.

I think we're all pretty much agreed that at the very least phys_efi_get_time() can go in the bin. Whatever the outcome of this thread that'll happen. The question is whether it makes sense to rip out all of the time stuff in one go.

Is it such a bad thing to be prepared for this sort of machine to arrive even if likely there are none so far?

Quoted 1 line

Quoted 1 line · Matt Fleming

Quoted 3 lines · Dan Carpenter

You're on your own for fixing the complicated stuff like layering violations. I just do static checker stuff and sed fixes. ;)

Thanks for the patch Dan. Nice catch.

But I'm wondering if we can simply delete phys_efi_get_time() to avoid the whole problem of doing GFP_KERNEL allocations under a spinlock.

In fact, the whole EFI time stuff is looking a bit crusty.

I only see two direct users of efi.get_time() outside of arch,

drivers/char/efirtc.c
drivers/rtc/rtc-efi.c

which are both "depends on IA64".

For x86, all other callers are inside
arch/x86/platform/efi/efi.c,

efi_set_rtc_mmss()
efi_get_time()

neither of which have any callers - it all appears to be dead code. The diff stat of deleting all this dead code isn't too bad either,

 arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
 arch/x86/platform/efi/efi_64.c |  90 ------------------------
 2 files changed, 5 insertions(+), 236 deletions(-)

Thoughts?

Quoted 3 lines · Dan Carpenter

You're on your own for fixing the complicated stuff like layering violations. I just do static checker stuff and sed fixes. ;)

Thanks for the patch Dan. Nice catch.

But I'm wondering if we can simply delete phys_efi_get_time() to avoid the whole problem of doing GFP_KERNEL allocations under a spinlock.

In fact, the whole EFI time stuff is looking a bit crusty.

I only see two direct users of efi.get_time() outside of arch,

drivers/char/efirtc.c
drivers/rtc/rtc-efi.c

which are both "depends on IA64". For x86, all other callers are inside arch/x86/platform/efi/efi.c,

efi_set_rtc_mmss()
efi_get_time()

neither of which have any callers - it all appears to be dead code. The diff stat of deleting all this dead code isn't too bad either,

 arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
 arch/x86/platform/efi/efi_64.c |  90 ------------------------
 2 files changed, 5 insertions(+), 236 deletions(-)

Thoughts?

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +0 -0

index 0c2a234fef1e..f5adcadb381b 100644

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock so this allocation should be atomic.

Fixes b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')

Signed-off-by Dan Carpenter <[email protected]>

arch/x86/platform/efi/efi_64.c +0 -0

index 0c2a234fef1e..f5adcadb381b 100644