Skip to contentSkip to conversation

x86/efi: Correct a tiny mistake in code comment

PATCH In review Baoquan He 6 participants 21 messages Mar 8, 2017 – Mar 15, 2017

Subsystems (3)
x86 x86 MM EFI
lore
Share & tools
lore

Baoquan He

author
#0 lore raw

PATCH 1/2 x86/efi: Correct a tiny mistake in code comment

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +1 -1
index a4695da..6cbf9e0 100644
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
  */
 static u64 efi_va = EFI_VA_START;
Signature · 1 line
2.5.5
author
#1 lore raw

PATCH 2/2 x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
 	 */
 	BUILD_BUG_ON(vaddr_start >= vaddr_end);
 	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-		     vaddr_end >= EFI_VA_START);
+		     vaddr_end >= EFI_VA_END);
 	BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
 		      IS_ENABLED(CONFIG_EFI)) &&
 		     vaddr_end >= __START_KERNEL_map);
Signature · 1 line
2.5.5
Acked-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

Quoted 37 lines · Baoquan He

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
 	 */
 	BUILD_BUG_ON(vaddr_start >= vaddr_end);
 	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-		     vaddr_end >= EFI_VA_START);
+		     vaddr_end >= EFI_VA_END);
 	BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
 		      IS_ENABLED(CONFIG_EFI)) &&
 		     vaddr_end >= __START_KERNEL_map);
Signature · 1 line
2.5.5
Acked-by Dave Young <[email protected]>

Thanks
Dave

Reviewed-by
#3 lore raw

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

Quoted 32 lines · Dave Young
Quoted 31 lines · Baoquan He

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
       */
      BUILD_BUG_ON(vaddr_start >= vaddr_end);
      BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-                  vaddr_end >= EFI_VA_START);
+                  vaddr_end >= EFI_VA_END);
Quoted 9 lines
Quoted 6 lines
arch/x86/mm/kaslr.c +0 -1
      BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
                    IS_ENABLED(CONFIG_EFI)) &&
                   vaddr_end >= __START_KERNEL_map);
--

2.5.5

Acked-by Dave Young <[email protected]>

Thanks Bao for this fix. This makes the KASLR code consistent with Address space markers hints in [1]

[1] http://lxr.free-electrons.com/source/arch/x86/mm/dump_pagetables.c#L82

Reviewed-by Bhupesh Sharma <[email protected]>

Regards,
Bhupesh

Acked-by
#4 lore raw

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

Thanks for the change.

Acked-by Thomas Garnier <[email protected]>
Quoted 52 lines · Bhupesh Sharma
Quoted 32 lines · Dave Young
Quoted 31 lines · Baoquan He

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
       */
      BUILD_BUG_ON(vaddr_start >= vaddr_end);
      BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-                  vaddr_end >= EFI_VA_START);
+                  vaddr_end >= EFI_VA_END);
Quoted 9 lines
Quoted 6 lines
arch/x86/mm/kaslr.c +0 -1
      BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
                    IS_ENABLED(CONFIG_EFI)) &&
                   vaddr_end >= __START_KERNEL_map);
--

2.5.5

Acked-by Dave Young <[email protected]>

Thanks Bao for this fix. This makes the KASLR code consistent with Address space markers hints in [1]

[1] http://lxr.free-electrons.com/source/arch/x86/mm/dump_pagetables.c#L82

Reviewed-by Bhupesh Sharma <[email protected]>

Regards,
Bhupesh

Signature · 1 line
Thomas
author
#5 lore raw

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

In reply to Baoquan He

PING!

Is there any suggestion for this code bug fix?

Boris added comment in patch 1/2 thread that it can also be fixed by swapping the naming - EFI_VA_START and EFI_VA_END. As he said the swapping can remove the confusion about the naming, while the con is changing it now could confuse more people who have the current mental picture of the mapping direction.

And there's also a well known similar use case, stack, like stack_end naming in arch/x86/boot/main.c which is the low addr boundary of stack region.

Any idea?

Thanks
Baoquan

Quoted 37 lines · Baoquan He

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
 	 */
 	BUILD_BUG_ON(vaddr_start >= vaddr_end);
 	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-		     vaddr_end >= EFI_VA_START);
+		     vaddr_end >= EFI_VA_END);
 	BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
 		      IS_ENABLED(CONFIG_EFI)) &&
 		     vaddr_end >= __START_KERNEL_map);
Signature · 1 line
2.5.5
author
#6 lore raw

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

Quoted 9 lines · Baoquan He

PING!

Is there any suggestion for this code bug fix?

Boris added comment in patch 1/2 thread that it can also be fixed by swapping the naming - EFI_VA_START and EFI_VA_END. As he said the swapping can remove the confusion about the naming, while the con is changing it now could confuse more people who have the current mental picture of the mapping direction.

If swapping the naming is suggested, I can post v2 to change efi code. Both of them is fine to me.

Quoted 48 lines

And there's also a well known similar use case, stack, like stack_end naming in arch/x86/boot/main.c which is the low addr boundary of stack region.

Any idea?

Thanks
Baoquan

Quoted 37 lines · Baoquan He

EFI allocates runtime services regions top-down, starting from EFI_VA_START to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of EFI region. The upper boundary of memory regions randomized by KASLR should be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/mm/kaslr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/kaslr.c +2 -2
index 887e571..aed2064 100644
@@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
 #if defined(CONFIG_X86_ESPFIX64)
 static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
 #elif defined(CONFIG_EFI)
-static const unsigned long vaddr_end = EFI_VA_START;
+static const unsigned long vaddr_end = EFI_VA_END;
 #else
 static const unsigned long vaddr_end = __START_KERNEL_map;
 #endif

@@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
 	 */
 	BUILD_BUG_ON(vaddr_start >= vaddr_end);
 	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
-		     vaddr_end >= EFI_VA_START);
+		     vaddr_end >= EFI_VA_END);
 	BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
 		      IS_ENABLED(CONFIG_EFI)) &&
 		     vaddr_end >= __START_KERNEL_map);
Signature · 1 line
2.5.5
#7 lore raw
In reply to Baoquan He
Quoted 13 lines · Baoquan He

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index a4695da..6cbf9e0 100644

arch/x86/platform/efi/efi_64.c line 50

Quoted 5 lines
arch/x86/platform/efi/efi_64.c +1 -1
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

Quoted 6 lines
arch/x86/platform/efi/efi_64.c +0 -0
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
  */
 static u64 efi_va = EFI_VA_START;
Signature · 1 line
2.5.5

Thanks
Dave

author
#8 lore raw

Forgot cc to Boris, add him.

Quoted 16 lines · Dave Young
Quoted 15 lines · Baoquan He

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index a4695da..6cbf9e0 100644
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
Quoted 7 lines

arch/x86/platform/efi/efi_64.c line 50

Quoted 3 lines
arch/x86/platform/efi/efi_64.c +1 -1
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

Thanks for reviewing!

I am not sure. Just in efi_map_region() it gets the starting va to map 'size' big of region by below code:

	efi_va -= size;

-4G and -68G just a trick which makes people understand easily, still we think kernel text mapping region is in higher addr area then vmalloc. I personnally think.

Thanks
Baoquan

#9 lore raw
Quoted 3 lines · Baoquan He

-4G and -68G just a trick which makes people understand easily, still we think kernel text mapping region is in higher addr area then vmalloc. I personnally think.

Just remove the direction: bottom-up or top-down, it will confuse people.

"We allocate runtime services regions starting from -4G, i.e. 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space is limited to 64G."

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
author
#10 lore raw
Quoted 10 lines · Borislav Petkov
Quoted 3 lines · Baoquan He

-4G and -68G just a trick which makes people understand easily, still we think kernel text mapping region is in higher addr area then vmalloc. I personnally think.

Just remove the direction: bottom-up or top-down, it will confuse people.

"We allocate runtime services regions starting from -4G, i.e. 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space is limited to 64G."

Right, will repost one.

Thanks!

#11 lore raw
In reply to Baoquan He

Hi,

Quoted 29 lines · Baoquan He

Forgot cc to Boris, add him.

Quoted 16 lines · Dave Young
Quoted 15 lines · Baoquan He

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index a4695da..6cbf9e0 100644
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
Quoted 7 lines

arch/x86/platform/efi/efi_64.c line 50

Quoted 3 lines
arch/x86/platform/efi/efi_64.c +1 -1
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

Thanks for reviewing!

Quoted 7 lines

I am not sure. Just in efi_map_region() it gets the starting va to map 'size' big of region by below code:

	efi_va -= size;

-4G and -68G just a trick which makes people understand easily, still we think kernel text mapping region is in higher addr area then vmalloc. I personnally think.

I understand your points, there is not right or wrong. So I think drop the words like the change in your V2 looks good.

Thanks
Dave

#12 lore raw
In reply to Dave Young

Hi Dave,

Quoted 16 lines · Dave Young
Quoted 15 lines · Baoquan He

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index a4695da..6cbf9e0 100644
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
Quoted 7 lines

arch/x86/platform/efi/efi_64.c line 50

Quoted 3 lines
arch/x86/platform/efi/efi_64.c +1 -1
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

I think you have a valid point, but I think the -4G convention is probably too confusing to read and may lead to issues when we use this for future feature addition as well. It would be more useful to use the macros similar to the MODULES_{} addresses we use currently in 'arch/x86/include/asm/pgtable_64_types.h':

#define MODULES_VADDR (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
#define MODULES_END _AC(0xffffffffff000000, UL)
#define MODULES_LEN (MODULES_END - MODULES_VADDR)

May be we can use the following convention for the EFI_VA_{} addresses as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

which is less confusing to read in my opinion.

@Baoquan: Please share your views.

Regards,
Bhupesh

author
#13 lore raw
Quoted 40 lines · Bhupesh Sharma

Hi Dave,

Quoted 16 lines · Dave Young
Quoted 15 lines · Baoquan He

EFI allocate runtime services regions down from EFI_VA_START, -4G. It should be top-down handling.

Signed-off-by Baoquan He <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index a4695da..6cbf9e0 100644
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
Quoted 7 lines

arch/x86/platform/efi/efi_64.c line 50

Quoted 3 lines
arch/x86/platform/efi/efi_64.c +1 -1
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

I think you have a valid point, but I think the -4G convention is probably too confusing to read and may lead to issues when we use this for future feature addition as well. It would be more useful to use the macros similar to the MODULES_{} addresses we use currently in 'arch/x86/include/asm/pgtable_64_types.h':

#define MODULES_VADDR (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
#define MODULES_END _AC(0xffffffffff000000, UL)
#define MODULES_LEN (MODULES_END - MODULES_VADDR)

May be we can use the following convention for the EFI_VA_{} addresses as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':

Quoted 6 lines

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

which is less confusing to read in my opinion.

@Baoquan: Please share your views.

Yes, it looks better. I can repost with this change. Thanks.

maintainer
#14 lore raw
Quoted 1 line · Baoquan He

Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

#define EFI_VA_START ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END (-68 * (_AC(1, UL) << 30))

That's -4G (the shift by 30) and -68G, respectively.

Quoted 2 lines
Quoted 2 lines

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Can you guys point your attention to something which is really broken and stop wasting your time? And there's enough really broken crap left and right...

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#15 lore raw
Quoted 4 lines · Borislav Petkov
Quoted 1 line · Baoquan He

Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

All right, I will just update the code comment. Just back ported kaslr to our OS product, people reviewed and found the upper boundary of kaslr mm region is EFI_VA_START, that's not correct, it has to be corrected firstly in upstream. Then found the confusion in code comment.

Change or keep it, both is fine to me.

Thanks!

Quoted 20 lines

#define EFI_VA_START ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END (-68 * (_AC(1, UL) << 30))

That's -4G (the shift by 30) and -68G, respectively.

Quoted 2 lines
Quoted 2 lines

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Can you guys point your attention to something which is really broken and stop wasting your time? And there's enough really broken crap left and right...

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
maintainer
#16 lore raw
Quoted 4 lines · Baoquan He

All right, I will just update the code comment. Just back ported kaslr to our OS product, people reviewed and found the upper boundary of kaslr mm region is EFI_VA_START, that's not correct, it has to be corrected firstly in upstream. Then found the confusion in code comment.

        BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&

+ vaddr_end >= EFI_VA_END);

so I think that once we've done the mapping, we won't need anymore VA space so we could simply check the range [efi_va, EFI_VA_START] instead.

However, that won't work currently because evi_va is not valid at build time. And it won't work at boot time either because, AFAICT, kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...

So yours is probably OK.

I guess what's confusing there is the naming - EFI_VA_START and EFI_VA_END. They're kinda swapped because of the direction we take when we start mapping runtime services, i.e., from the higher (unsigned) address to lower.

I guess we could swap the naming so that it doesn't confuse people but that would be up to EFI maintainers.

Then stuff like that:

# ifdef CONFIG_EFI
        { EFI_VA_END,           "EFI Runtime Services" },
# endif

will make more sense when they are:

# ifdef CONFIG_EFI
        { EFI_VA_START,           "EFI Runtime Services" },
# endif

But changing it now could confuse more people who have the current mental picture of the mapping direction so I'd vote for the simple fix above.

Again, as previously, this is a maintainer decision.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#17 lore raw
Quoted 34 lines · Borislav Petkov
Quoted 4 lines · Baoquan He

All right, I will just update the code comment. Just back ported kaslr to our OS product, people reviewed and found the upper boundary of kaslr mm region is EFI_VA_START, that's not correct, it has to be corrected firstly in upstream. Then found the confusion in code comment.

        BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&

+ vaddr_end >= EFI_VA_END);

so I think that once we've done the mapping, we won't need anymore VA space so we could simply check the range [efi_va, EFI_VA_START] instead.

However, that won't work currently because evi_va is not valid at build time. And it won't work at boot time either because, AFAICT, kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...

So yours is probably OK.

I guess what's confusing there is the naming - EFI_VA_START and EFI_VA_END. They're kinda swapped because of the direction we take when we start mapping runtime services, i.e., from the higher (unsigned) address to lower.

I guess we could swap the naming so that it doesn't confuse people but that would be up to EFI maintainers.

Then stuff like that:

# ifdef CONFIG_EFI
        { EFI_VA_END,           "EFI Runtime Services" },
# endif

will make more sense when they are:

Quoted 7 lines
# ifdef CONFIG_EFI
        { EFI_VA_START,           "EFI Runtime Services" },
# endif

But changing it now could confuse more people who have the current mental picture of the mapping direction so I'd vote for the simple fix above.

People should understand the meaning of the macro then use it correctly, one should not assume START == lower address unless they are sure.

Quoted 3 lines

Again, as previously, this is a maintainer decision.

Personally I think current way is just fine, but agreed it is up to efi maintainer.

Thanks
Dave

#18 lore raw
In reply to Borislav Petkov
Quoted 8 lines · Borislav Petkov
Quoted 1 line · Baoquan He

Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

#define EFI_VA_START ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END (-68 * (_AC(1, UL) << 30))

Quoted 6 lines

That's -4G (the shift by 30) and -68G, respectively.

Quoted 2 lines
Quoted 2 lines

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Right, my point was that this -4G convention stands out, as compared to the rest of the addressing convention used throughout 'arch/x86/include/asm/pgtable_64_types.h'.

For e.g.:

#define __VMALLOC_BASE _AC(0xffffc90000000000, UL)
#define __VMEMMAP_BASE _AC(0xffffea0000000000, UL)
..
#define MODULES_END _AC(0xffffffffff000000, UL)
..
and so on.

Also it seems inconsistent to the convention used in 'Documentation/x86/x86_64/mm.txt'

As you noted in one of your other comments in this thread, this causes a confusion as to whether the EFI_VA_END and EFI_VA_START macros need to be swapped or the comments elsewhere in the x86 code which use these MACROS are incorrect.

However, I think it is more a matter of code readability and each of these styles have their own advantages.

I would be happy to cook up a patch to have uniformity in the addressing conventions used across pgtable_64_types.h (if it is required at all), but it would be a different topic and not related to this patch.

Regards,
Bhupesh

author
#19 lore raw
In reply to Bhupesh Sharma
Quoted 21 lines · Bhupesh Sharma

Hi Dave,

Quoted 7 lines · Dave Young
Quoted 2 lines · Baoquan He

- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address although from mathematics view it is lower then positive addresses.

I think you have a valid point, but I think the -4G convention is probably too confusing to read and may lead to issues when we use this for future feature addition as well. It would be more useful to use the macros similar to the MODULES_{} addresses we use currently in 'arch/x86/include/asm/pgtable_64_types.h':

#define MODULES_VADDR (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
#define MODULES_END _AC(0xffffffffff000000, UL)
#define MODULES_LEN (MODULES_END - MODULES_VADDR)

Quoted 5 lines

May be we can use the following convention for the EFI_VA_{} addresses as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':

#define EFI_VA_START _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

Isn't it like this:

#define EFI_VA_START _AC(0xffffffff00000000, UL)
#define EFI_VA_END _AC(0xffffffef00000000, UL)

Just make them be equal to value which computer stores -4G and -68G?

You can see in arch/x86/platform/efi/efi_64.c, it's using efi_va minus size directly, here size should be 4K, page aligned.

	efi_va -= size;

Above formula has considered the open interval attribute of EFI_VA_START. Making EFI_VA_START be 0xfffffffeffffffff could be wrong.

Right?

author
#20 lore raw

PATCH v2 1/2 x86/efi/64: Clean up code comment about efi region

In reply to Baoquan He

EFI allocates runtime services regions starting from EFI_VA_START, -4G, decrement to EFI_VA_END. So remove the bottom-up term to avoid confusion.

Signed-off-by Baoquan He <[email protected]>

---

 arch/x86/platform/efi/efi_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi_64.c +3 -2
index a4695da..e941b29 100644
@@ -47,8 +47,9 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
- * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
+ * We allocate runtime services regions starting from -4G, i.e.
+ * 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
+ * is limited to 64G.
  */
 static u64 efi_va = EFI_VA_START;
Signature · 1 line
2.5.5