On Thu, Jun 02, 2016 at 09:19:49PM +0100, Matt Fleming wrote:
> On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote:
> > +#define arch_efi_call_virt(p, f, ...)
> > \
> > +({ \
> > + u32 func = runtime_service32(f); \
> > + efi64_thunk(func, __VA_ARGS__); \
> > +})
> > +
>
> This isn't correct because you're turning the runtime decision of
> whether we're executing the thunking code into a build time one.
Ahh, yep, you're absolutely correct. That's not what I intended to do,
but that's definitely the effect that this change has.
> Would something like this work instead? It's not as neat as your
> suggestion but it's a damn sight better than what we have today.
>
> ---
>
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 6e7242be1c87..b976084e56ef 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -469,18 +469,13 @@ extern efi_status_t efi64_thunk(u32, ...);
> unsigned long flags; \
> u32 func; \
> \
> - efi_sync_low_kernel_mappings(); \
> local_irq_save(flags); \
> - \
> - efi_scratch.prev_cr3 = read_cr3(); \
> - write_cr3((unsigned long)efi_scratch.efi_pgt); \
> - __flush_tlb_all(); \
> + arch_efi_call_virt_setup(); \
> \
> func = runtime_service32(f); \
> __s = efi64_thunk(func, __VA_ARGS__); \
> \
> - write_cr3(efi_scratch.prev_cr3); \
> - __flush_tlb_all(); \
> + arch_efi_call_virt_teardown(); \
> local_irq_restore(flags); \
> \
> __s; \
This looks good to me. We're at least making use of the
arch_efi_call_virt_* stuff where possible, and only using the special
thunk code where necessary. I think it's a good middle ground between
the two approaches (especially considering the fact that mine won't
work :)
I will re-work that last patch to include this change instead of my
original, broken one.
Thanks, Matt!
- Alex
