Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1268037 > unrolled thread

[PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt() Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-12 16:50 +0100
    Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Borislav Petkov <bp@alien8.de> - 2015-11-12 19:50 +0100
      Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-12 21:10 +0100
        Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Borislav Petkov <bp@alien8.de> - 2015-11-13 09:10 +0100
          Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-13 17:20 +0100
    Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Borislav Petkov <bp@alien8.de> - 2015-11-12 19:50 +0100
      Re: [PATCH 4/6] x86/efi: Hoist page table switching code into  efi_call_virt() Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-12 21:20 +0100

#1268037 — [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

This change is a prerequisite for pending patches that switch to a
dedicated EFI page table, instead of using 'trampoline_pgd' which
shares PGD entries with 'swapper_pg_dir'. The pending patches make it
impossible to dereference the runtime service function pointer without
first switching %cr3.

It's true that we now have duplicated switching code in
efi_call_virt() and efi_call_phys_{prolog,epilog}() but we are
sacrificing code duplication for a little more clarity and the ease of
writing the page table switching code in C instead of asm.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Jones <davej@codemonkey.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Denys Vlasenko <dvlasenk@redhat.com>,
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/include/asm/efi.h          | 25 +++++++++++++++++++++
 arch/x86/platform/efi/efi_64.c      | 24 ++++++++++-----------
 arch/x86/platform/efi/efi_stub_64.S | 43 -------------------------------------
 3 files changed, 36 insertions(+), 56 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cfee9d4b02af..f9d99d4e7b1a 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -3,6 +3,7 @@
 
 #include <asm/fpu/api.h>
 #include <asm/pgtable.h>
+#include <asm/tlb.h>
 
 /*
  * We map the EFI regions needed for runtime services non-contiguously,
@@ -64,6 +65,17 @@ extern u64 asmlinkage efi_call(void *fp, ...);
 
 #define efi_call_phys(f, args...)		efi_call((f), args)
 
+/*
+ * Scratch space used for switching the pagetable in the EFI stub
+ */
+struct efi_scratch {
+	u64 r15;
+	u64 prev_cr3;
+	pgd_t *efi_pgt;
+	bool use_pgd;
+	u64 phys_stack;
+} __packed;
+
 #define efi_call_virt(f, ...)						\
 ({									\
 	efi_status_t __s;						\
@@ -71,7 +83,20 @@ extern u64 asmlinkage efi_call(void *fp, ...);
 	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 *)efi.systab->runtime->f, __VA_ARGS__);	\
+									\
+	if (efi_scratch.use_pgd) { 					\
+		write_cr3(efi_scratch.prev_cr3);			\
+		__flush_tlb_all(); 					\
+	} 								\
+									\
 	__kernel_fpu_end();						\
 	preempt_enable();						\
 	__s;								\
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 634536034e32..ab5f14a886cc 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,16 +47,7 @@
  */
 static u64 efi_va = EFI_VA_START;
 
-/*
- * Scratch space used for switching the pagetable in the EFI stub
- */
-struct efi_scratch {
-	u64 r15;
-	u64 prev_cr3;
-	pgd_t *efi_pgt;
-	bool use_pgd;
-	u64 phys_stack;
-} __packed;
+struct efi_scratch efi_scratch;
 
 static void __init early_code_mapping_set_exec(int executable)
 {
@@ -83,8 +74,11 @@ pgd_t * __init efi_call_phys_prolog(void)
 	int pgd;
 	int n_pgds;
 
-	if (!efi_enabled(EFI_OLD_MEMMAP))
-		return NULL;
+	if (!efi_enabled(EFI_OLD_MEMMAP)) {
+		save_pgd = (pgd_t *)read_cr3();
+		write_cr3((unsigned long)efi_scratch.efi_pgt);
+		goto out;
+	}
 
 	early_code_mapping_set_exec(1);
 
@@ -96,6 +90,7 @@ pgd_t * __init efi_call_phys_prolog(void)
 		vaddress = (unsigned long)__va(pgd * PGDIR_SIZE);
 		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress));
 	}
+out:
 	__flush_tlb_all();
 
 	return save_pgd;
@@ -109,8 +104,11 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	int pgd_idx;
 	int nr_pgds;
 
-	if (!save_pgd)
+	if (!efi_enabled(EFI_OLD_MEMMAP)) {
+		write_cr3((unsigned long)save_pgd);
+		__flush_tlb_all();
 		return;
+	}
 
 	nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
 
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index 86d0f9e08dd9..32020cb8bb08 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -38,41 +38,6 @@
 	mov %rsi, %cr0;			\
 	mov (%rsp), %rsp
 
-	/* stolen from gcc */
-	.macro FLUSH_TLB_ALL
-	movq %r15, efi_scratch(%rip)
-	movq %r14, efi_scratch+8(%rip)
-	movq %cr4, %r15
-	movq %r15, %r14
-	andb $0x7f, %r14b
-	movq %r14, %cr4
-	movq %r15, %cr4
-	movq efi_scratch+8(%rip), %r14
-	movq efi_scratch(%rip), %r15
-	.endm
-
-	.macro SWITCH_PGT
-	cmpb $0, efi_scratch+24(%rip)
-	je 1f
-	movq %r15, efi_scratch(%rip)		# r15
-	# save previous CR3
-	movq %cr3, %r15
-	movq %r15, efi_scratch+8(%rip)		# prev_cr3
-	movq efi_scratch+16(%rip), %r15		# EFI pgt
-	movq %r15, %cr3
-	1:
-	.endm
-
-	.macro RESTORE_PGT
-	cmpb $0, efi_scratch+24(%rip)
-	je 2f
-	movq efi_scratch+8(%rip), %r15
-	movq %r15, %cr3
-	movq efi_scratch(%rip), %r15
-	FLUSH_TLB_ALL
-	2:
-	.endm
-
 ENTRY(efi_call)
 	SAVE_XMM
 	mov (%rsp), %rax
@@ -83,16 +48,8 @@ ENTRY(efi_call)
 	mov %r8, %r9
 	mov %rcx, %r8
 	mov %rsi, %rcx
-	SWITCH_PGT
 	call *%rdi
-	RESTORE_PGT
 	addq $48, %rsp
 	RESTORE_XMM
 	ret
 ENDPROC(efi_call)
-
-	.data
-ENTRY(efi_scratch)
-	.fill 3,8,0
-	.byte 0
-	.quad 0
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1268197 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Thu, Nov 12, 2015 at 03:40:21PM +0000, Matt Fleming wrote:
> This change is a prerequisite for pending patches that switch to a
> dedicated EFI page table, instead of using 'trampoline_pgd' which
> shares PGD entries with 'swapper_pg_dir'. The pending patches make it
> impossible to dereference the runtime service function pointer without
> first switching %cr3.
> 
> It's true that we now have duplicated switching code in
> efi_call_virt() and efi_call_phys_{prolog,epilog}() but we are
> sacrificing code duplication for a little more clarity and the ease of
> writing the page table switching code in C instead of asm.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Dave Jones <davej@codemonkey.org.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>,
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  arch/x86/include/asm/efi.h          | 25 +++++++++++++++++++++
>  arch/x86/platform/efi/efi_64.c      | 24 ++++++++++-----------
>  arch/x86/platform/efi/efi_stub_64.S | 43 -------------------------------------
>  3 files changed, 36 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cfee9d4b02af..f9d99d4e7b1a 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -3,6 +3,7 @@
>  
>  #include <asm/fpu/api.h>
>  #include <asm/pgtable.h>
> +#include <asm/tlb.h>
>  
>  /*
>   * We map the EFI regions needed for runtime services non-contiguously,
> @@ -64,6 +65,17 @@ extern u64 asmlinkage efi_call(void *fp, ...);
>  
>  #define efi_call_phys(f, args...)		efi_call((f), args)
>  
> +/*
> + * Scratch space used for switching the pagetable in the EFI stub
> + */
> +struct efi_scratch {
> +	u64 r15;
> +	u64 prev_cr3;
> +	pgd_t *efi_pgt;
> +	bool use_pgd;
> +	u64 phys_stack;
> +} __packed;
> +
>  #define efi_call_virt(f, ...)						\
>  ({									\
>  	efi_status_t __s;						\
> @@ -71,7 +83,20 @@ extern u64 asmlinkage efi_call(void *fp, ...);
>  	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 *)efi.systab->runtime->f, __VA_ARGS__);	\
> +									\
> +	if (efi_scratch.use_pgd) { 					\
> +		write_cr3(efi_scratch.prev_cr3);			\
> +		__flush_tlb_all(); 					\
> +	} 								\
> +									\
>  	__kernel_fpu_end();						\
>  	preempt_enable();						\
>  	__s;								\
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 634536034e32..ab5f14a886cc 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -47,16 +47,7 @@
>   */
>  static u64 efi_va = EFI_VA_START;
>  
> -/*
> - * Scratch space used for switching the pagetable in the EFI stub
> - */
> -struct efi_scratch {
> -	u64 r15;
> -	u64 prev_cr3;
> -	pgd_t *efi_pgt;
> -	bool use_pgd;
> -	u64 phys_stack;
> -} __packed;
> +struct efi_scratch efi_scratch;
>  
>  static void __init early_code_mapping_set_exec(int executable)
>  {
> @@ -83,8 +74,11 @@ pgd_t * __init efi_call_phys_prolog(void)
>  	int pgd;
>  	int n_pgds;
>  
> -	if (!efi_enabled(EFI_OLD_MEMMAP))
> -		return NULL;
> +	if (!efi_enabled(EFI_OLD_MEMMAP)) {
> +		save_pgd = (pgd_t *)read_cr3();
> +		write_cr3((unsigned long)efi_scratch.efi_pgt);
> +		goto out;
> +	}
>  
>  	early_code_mapping_set_exec(1);
>  

So this one is called in phys_efi_set_virtual_address_map() like this:

----
        save_pgd = efi_call_phys_prolog();

        /* Disable interrupts around EFI calls: */
        local_irq_save(flags);

	<--- MARKER

        status = efi_call_phys(efi_phys.set_virtual_address_map,
                               memory_map_size, descriptor_size,
                               descriptor_version, virtual_map);
        local_irq_restore(flags);

        efi_call_phys_epilog(save_pgd);
---


Now, if you look at MARKER, the asm looks like this here:

        .loc 1 91 0
        call    efi_call_phys_prolog    #
        movq    %rax, %r15      #, save_pgd

        .file 6 "./arch/x86/include/asm/irqflags.h"
        .loc 6 20 0
#APP
# 20 "./arch/x86/include/asm/irqflags.h" 1
        # __raw_save_flags
        pushf ; pop %r14        # flags


That PUSHF implicitly pushes on the stack pointed by %rsp. But(!) we
have switched the pagetable (i.e., %cr3 has efi_scratch.efi_pgt) and
we're pushing to the VA where the stack *was* but is not anymore.

Or maybe it is because you're copying all the PUDs. It is still not 100%
clean, IMHO.

Can you do the prolog/epilog calls inside the IRQs-off section?

Btw, it was crap like that why I wanted to do SWITCH_PGT in asm...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268240 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Thu, 12 Nov, at 07:44:32PM, Borislav Petkov wrote:
> 
> So this one is called in phys_efi_set_virtual_address_map() like this:
> 
> ----
>         save_pgd = efi_call_phys_prolog();
> 
>         /* Disable interrupts around EFI calls: */
>         local_irq_save(flags);
> 
> 	<--- MARKER
> 
>         status = efi_call_phys(efi_phys.set_virtual_address_map,
>                                memory_map_size, descriptor_size,
>                                descriptor_version, virtual_map);
>         local_irq_restore(flags);
> 
>         efi_call_phys_epilog(save_pgd);
> ---
> 
> 
> Now, if you look at MARKER, the asm looks like this here:
> 
>         .loc 1 91 0
>         call    efi_call_phys_prolog    #
>         movq    %rax, %r15      #, save_pgd
> 
>         .file 6 "./arch/x86/include/asm/irqflags.h"
>         .loc 6 20 0
> #APP
> # 20 "./arch/x86/include/asm/irqflags.h" 1
>         # __raw_save_flags
>         pushf ; pop %r14        # flags
> 
> 
> That PUSHF implicitly pushes on the stack pointed by %rsp. But(!) we
> have switched the pagetable (i.e., %cr3 has efi_scratch.efi_pgt) and
> we're pushing to the VA where the stack *was* but is not anymore.
 
All the kernel mappings will still exist in the page table we switch
to, so pushing to the stack should be fine.

The mappings have to exist so that the firmware can dereference
pointer arguments, e.g. when writing variable data to a kernel buffer.

Or have I misunderstood your point?

> Or maybe it is because you're copying all the PUDs. It is still not 100%
> clean, IMHO.
> 
> Can you do the prolog/epilog calls inside the IRQs-off section?

Not really because in the efi_enabled(EFI_OLD_MEMMAP) case we perform
kmalloc(), see commit 23a0d4e8fa6d ("efi: Disable interrupts around
EFI calls, not in the epilog/prolog calls").
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268674 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Thu, Nov 12, 2015 at 08:01:08PM +0000, Matt Fleming wrote:
> > That PUSHF implicitly pushes on the stack pointed by %rsp. But(!) we
> > have switched the pagetable (i.e., %cr3 has efi_scratch.efi_pgt) and
> > we're pushing to the VA where the stack *was* but is not anymore.
>  
> All the kernel mappings will still exist in the page table we switch
> to, so pushing to the stack should be fine.
> 
> The mappings have to exist so that the firmware can dereference
> pointer arguments, e.g. when writing variable data to a kernel buffer.
> 
> Or have I misunderstood your point?
> 
> > Or maybe it is because you're copying all the PUDs. It is still not 100%
> > clean, IMHO.
^^^^^^^^^^^^^^^^^

I think we're on the same page - you're copying the PUDs in
efi_sync_low_kernel_mappings() so the stack should be there.

> > Can you do the prolog/epilog calls inside the IRQs-off section?
> 
> Not really because in the efi_enabled(EFI_OLD_MEMMAP) case we perform
> kmalloc(), see commit 23a0d4e8fa6d ("efi: Disable interrupts around
> EFI calls, not in the epilog/prolog calls").

        n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
        save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);

This?

That n_pgds thing is of static size so you can pre-alloc it maybe even
once during boot and reuse it all the time when EFI_OLD_MEMMAP is
enabled.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269056 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Fri, 13 Nov, at 08:59:43AM, Borislav Petkov wrote:
> On Thu, Nov 12, 2015 at 08:01:08PM +0000, Matt Fleming wrote:
> > > That PUSHF implicitly pushes on the stack pointed by %rsp. But(!) we
> > > have switched the pagetable (i.e., %cr3 has efi_scratch.efi_pgt) and
> > > we're pushing to the VA where the stack *was* but is not anymore.
> >  
> > All the kernel mappings will still exist in the page table we switch
> > to, so pushing to the stack should be fine.
> > 
> > The mappings have to exist so that the firmware can dereference
> > pointer arguments, e.g. when writing variable data to a kernel buffer.
> > 
> > Or have I misunderstood your point?
> > 
> > > Or maybe it is because you're copying all the PUDs. It is still not 100%
> > > clean, IMHO.
> ^^^^^^^^^^^^^^^^^
> 
> I think we're on the same page - you're copying the PUDs in
> efi_sync_low_kernel_mappings() so the stack should be there.

Correct.

> > > Can you do the prolog/epilog calls inside the IRQs-off section?
> > 
> > Not really because in the efi_enabled(EFI_OLD_MEMMAP) case we perform
> > kmalloc(), see commit 23a0d4e8fa6d ("efi: Disable interrupts around
> > EFI calls, not in the epilog/prolog calls").
> 
>         n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
>         save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> 
> This?
> 
> That n_pgds thing is of static size so you can pre-alloc it maybe even
> once during boot and reuse it all the time when EFI_OLD_MEMMAP is
> enabled.

Well, this code is only executed once during boot anyway, for
phys_efi_set_virtual_address_map().

FYI, I'm still planning on ripping out all the EFI_OLD_MEMMAP code, as
it's getting particularly crufty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268203 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Thu, Nov 12, 2015 at 03:40:21PM +0000, Matt Fleming wrote:
> This change is a prerequisite for pending patches that switch to a
> dedicated EFI page table, instead of using 'trampoline_pgd' which
> shares PGD entries with 'swapper_pg_dir'. The pending patches make it
> impossible to dereference the runtime service function pointer without
> first switching %cr3.
> 
> It's true that we now have duplicated switching code in
> efi_call_virt() and efi_call_phys_{prolog,epilog}() but we are
> sacrificing code duplication for a little more clarity and the ease of
> writing the page table switching code in C instead of asm.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Dave Jones <davej@codemonkey.org.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>,
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  arch/x86/include/asm/efi.h          | 25 +++++++++++++++++++++
>  arch/x86/platform/efi/efi_64.c      | 24 ++++++++++-----------
>  arch/x86/platform/efi/efi_stub_64.S | 43 -------------------------------------
>  3 files changed, 36 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cfee9d4b02af..f9d99d4e7b1a 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -3,6 +3,7 @@
>  
>  #include <asm/fpu/api.h>
>  #include <asm/pgtable.h>
> +#include <asm/tlb.h>
>  
>  /*
>   * We map the EFI regions needed for runtime services non-contiguously,
> @@ -64,6 +65,17 @@ extern u64 asmlinkage efi_call(void *fp, ...);
>  
>  #define efi_call_phys(f, args...)		efi_call((f), args)
>  
> +/*
> + * Scratch space used for switching the pagetable in the EFI stub
> + */
> +struct efi_scratch {
> +	u64 r15;
> +	u64 prev_cr3;
> +	pgd_t *efi_pgt;
> +	bool use_pgd;
> +	u64 phys_stack;
> +} __packed;
> +
>  #define efi_call_virt(f, ...)						\
>  ({									\
>  	efi_status_t __s;						\
> @@ -71,7 +83,20 @@ extern u64 asmlinkage efi_call(void *fp, ...);
>  	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 *)efi.systab->runtime->f, __VA_ARGS__);	\
> +									\
> +	if (efi_scratch.use_pgd) { 					\
> +		write_cr3(efi_scratch.prev_cr3);			\
> +		__flush_tlb_all(); 					\
> +	} 								\
> +									\
>  	__kernel_fpu_end();						\
>  	preempt_enable();						\
>  	__s;								\

checkpatch is bitching here - not that I agree with it:

WARNING: please, no space before tabs
#87: FILE: arch/x86/include/asm/efi.h:88:
+^I^Iefi_scratch.prev_cr3 = read_cr3(); ^I^I^I\$

WARNING: please, no space before tabs
#89: FILE: arch/x86/include/asm/efi.h:90:
+^I^I__flush_tlb_all(); ^I^I^I^I^I\$

WARNING: please, no space before tabs
#94: FILE: arch/x86/include/asm/efi.h:95:
+^Iif (efi_scratch.use_pgd) { ^I^I^I^I^I\$

WARNING: please, no space before tabs
#96: FILE: arch/x86/include/asm/efi.h:97:
+^I^I__flush_tlb_all(); ^I^I^I^I^I\$

WARNING: please, no space before tabs
#97: FILE: arch/x86/include/asm/efi.h:98:
+^I} ^I^I^I^I^I^I^I^I\$


> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 634536034e32..ab5f14a886cc 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -47,16 +47,7 @@
>   */
>  static u64 efi_va = EFI_VA_START;
>  
> -/*
> - * Scratch space used for switching the pagetable in the EFI stub
> - */
> -struct efi_scratch {
> -	u64 r15;
> -	u64 prev_cr3;
> -	pgd_t *efi_pgt;
> -	bool use_pgd;
> -	u64 phys_stack;
> -} __packed;
> +struct efi_scratch efi_scratch;
>  
>  static void __init early_code_mapping_set_exec(int executable)
>  {
> @@ -83,8 +74,11 @@ pgd_t * __init efi_call_phys_prolog(void)
>  	int pgd;
>  	int n_pgds;
>  
> -	if (!efi_enabled(EFI_OLD_MEMMAP))
> -		return NULL;
> +	if (!efi_enabled(EFI_OLD_MEMMAP)) {
> +		save_pgd = (pgd_t *)read_cr3();
> +		write_cr3((unsigned long)efi_scratch.efi_pgt);
> +		goto out;
> +	}
>  
>  	early_code_mapping_set_exec(1);
>  
> @@ -96,6 +90,7 @@ pgd_t * __init efi_call_phys_prolog(void)
>  		vaddress = (unsigned long)__va(pgd * PGDIR_SIZE);
>  		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress));
>  	}
> +out:
>  	__flush_tlb_all();
>  
>  	return save_pgd;
> @@ -109,8 +104,11 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)

There's a comment here:

        /*
         * After the lock is released, the original page table is restored.
         */

Which lock are we talking about?

>  	int pgd_idx;
>  	int nr_pgds;
>  
> -	if (!save_pgd)
> +	if (!efi_enabled(EFI_OLD_MEMMAP)) {
> +		write_cr3((unsigned long)save_pgd);
> +		__flush_tlb_all();
>  		return;
> +	}
>  
>  	nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
>  

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268244 — Re: [PATCH 4/6] x86/efi: Hoist page table switching code into efi_call_virt()

On Thu, 12 Nov, at 07:47:14PM, Borislav Petkov wrote:
> 
> checkpatch is bitching here - not that I agree with it:
> 
> WARNING: please, no space before tabs
> #87: FILE: arch/x86/include/asm/efi.h:88:
> +^I^Iefi_scratch.prev_cr3 = read_cr3(); ^I^I^I\$
> 
> WARNING: please, no space before tabs
> #89: FILE: arch/x86/include/asm/efi.h:90:
> +^I^I__flush_tlb_all(); ^I^I^I^I^I\$
> 
> WARNING: please, no space before tabs
> #94: FILE: arch/x86/include/asm/efi.h:95:
> +^Iif (efi_scratch.use_pgd) { ^I^I^I^I^I\$
> 
> WARNING: please, no space before tabs
> #96: FILE: arch/x86/include/asm/efi.h:97:
> +^I^I__flush_tlb_all(); ^I^I^I^I^I\$
> 
> WARNING: please, no space before tabs
> #97: FILE: arch/x86/include/asm/efi.h:98:
> +^I} ^I^I^I^I^I^I^I^I\$
 
Crap. These look legit, I'll fix this up in v2.

> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index 634536034e32..ab5f14a886cc 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -47,16 +47,7 @@
> >   */
> >  static u64 efi_va = EFI_VA_START;
> >  
> > -/*
> > - * Scratch space used for switching the pagetable in the EFI stub
> > - */
> > -struct efi_scratch {
> > -	u64 r15;
> > -	u64 prev_cr3;
> > -	pgd_t *efi_pgt;
> > -	bool use_pgd;
> > -	u64 phys_stack;
> > -} __packed;
> > +struct efi_scratch efi_scratch;
> >  
> >  static void __init early_code_mapping_set_exec(int executable)
> >  {
> > @@ -83,8 +74,11 @@ pgd_t * __init efi_call_phys_prolog(void)
> >  	int pgd;
> >  	int n_pgds;
> >  
> > -	if (!efi_enabled(EFI_OLD_MEMMAP))
> > -		return NULL;
> > +	if (!efi_enabled(EFI_OLD_MEMMAP)) {
> > +		save_pgd = (pgd_t *)read_cr3();
> > +		write_cr3((unsigned long)efi_scratch.efi_pgt);
> > +		goto out;
> > +	}
> >  
> >  	early_code_mapping_set_exec(1);
> >  
> > @@ -96,6 +90,7 @@ pgd_t * __init efi_call_phys_prolog(void)
> >  		vaddress = (unsigned long)__va(pgd * PGDIR_SIZE);
> >  		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress));
> >  	}
> > +out:
> >  	__flush_tlb_all();
> >  
> >  	return save_pgd;
> > @@ -109,8 +104,11 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
> 
> There's a comment here:
> 
>         /*
>          * After the lock is released, the original page table is restored.
>          */
> 
> Which lock are we talking about?

No idea, we don't take any locks. Looks like a stale comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web