Re: [BUG] x86/efi: MMRs no longer properly mapped after switch to isolated page table

[Borislav Petkov <bp@suse.de>]

On Mon, May 02, 2016 at 07:10:36PM -0500, Alex Thorlton wrote:
> +#define uv_call_virt(f, args...) \
> +({                                                                     \
> +       efi_status_t __s;                                               \
> +       kernel_fpu_begin();                                             \
> +       __s = ((efi_##f##_t __attribute__((regparm(0)))*)               \
> +               f)(args);                                               \
> +       kernel_fpu_end();                                               \
> +       __s;                                                            \
> +})

Right, can you use the EFI-ones in
drivers/firmware/efi/runtime-wrappers.c directly?

 (So they're going to land there, I'm staring at latest -tip and those calls
  have become all fancy now:

#define efi_call_virt(f, args...)                                       \
({                                                                      \
        efi_status_t __s;                                               \
        unsigned long flags;                                            \
        arch_efi_call_virt_setup();                                     \
        local_save_flags(flags);                                        \
        __s = arch_efi_call_virt(f, args);                              \
        efi_call_virt_check_flags(flags, __stringify(f));               \
        arch_efi_call_virt_teardown();                                  \
        __s;                                                            \
})

with efi_call_virt_check_flags() checking for IRQ flags corruption... And so
on, but that's beside the point... )

> +
>  /* Use this macro if your virtual call does not return any value */
>  #define __efi_call_virt(f, args...) \
>  ({                                                                     \
> @@ -104,6 +114,32 @@ struct efi_scratch {
>         __s;                                                            \
>  })
>  
> +#define uv_call_virt(f, ...)                                           \
> +({                                                                     \
> +       efi_status_t __s;                                               \
> +                                                                       \
> +       efi_sync_low_kernel_mappings();                                 \
> +       preempt_disable();                                              \
> +       __kernel_fpu_begin();                                           \
> +                                                                       \
> +       if (efi_scratch.use_pgd) {                                      \
> +               efi_scratch.prev_cr3 = read_cr3();                      \
> +               write_cr3((unsigned long)efi_scratch.efi_pgt);          \
> +               __flush_tlb_all();                                      \
> +       }                                                               \
> +                                                                       \
> +       __s = efi_call((void *)f, __VA_ARGS__); \
> +                                                                       \
> +       if (efi_scratch.use_pgd) {                                      \
> +               write_cr3(efi_scratch.prev_cr3);                        \
> +               __flush_tlb_all();                                      \
> +       }                                                               \
> +                                                                       \
> +       __kernel_fpu_end();                                             \
> +       preempt_enable();                                               \
> +       __s;                                                            \
> +})
> +
>  /*
>   * All X86_64 virt calls return non-void values. Thus, use non-void call for
>   * virt calls that would be void on X86_32.
> 
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 1584cbe..6e99f81 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -39,8 +39,8 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
>                  */
>                 return BIOS_STATUS_UNIMPLEMENTED;
>  
> -       ret = efi_call((void *)__va(tab->function), (u64)which,
> -                       a1, a2, a3, a4, a5);
> +       ret = uv_call_virt(tab->function, (u64)which, a1, a2, a3, a4, a5);
> +

That could be simply

		efi_call_virt(tab->function, ...)

methinks.

>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(uv_bios_call);
> --->8
> 
> Note that the only change I made to efi_call_virt was to change
> efi.systab->runtime->f to simply f in the efi_call line.  This works up
> until we try to do callbacks from a loaded module.  When we try that we
> hit this:
> 
> [   56.232086] BUG: unable to handle kernel paging request at ffffffff8106148f
> [   56.239880] IP: [<fffffffedbb408ce>] 0xfffffffedbb408ce
> [   56.245721] PGD 8698e0067 PUD 1a08063 PMD 10001e1 

PMD looks ok to me.

Have you tried CONFIG_EFI_PGT_DUMP ? It will dump to dmesg so you might
be able to spot stuff.

Also, you could dump them from debugfs *right* before loading the module
and then look at stuff.

Also 2, booting with efi=debug should give you how the EFI regions get
mapped.

...

> The bad paging request here appears to be on the:
> 
> if (efi_scratch.use_pgd)
> 
> Line of uv_call_virt.  It looks like it's having trouble accessing the
> efi_scratch struct using the EFI page table.  I'm not sure why this
> is an issue with callbacks from modules and not with the ones in
> uv_system_init and friends.

Just this one module or all modules doing EFI calls?

> I'll keep investigating the module issue.  Looks like we're getting
> closer to sorting this out!
> 
> Let me know if you have thoughts about the way I'm getting stuff
> working.  I'm thinking there's probably a better way to do this than by
> copying the whole efi_call_virt macro - this was a quick and dirty
> solution.

Yeah, try using the generic facilities. We should be able to accomodate
all users...

HTH.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--