Skip to contentSkip to conversation

ibft, x86: Change reserve_ibft_region() to find_ibft_region()

PATCH In review Yinghai Lu 3 participants 8 messages Apr 1, 2010

Subsystems (2)
x86 iSCSI boot firmware table (iBFT) driver
lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 ibft, x86: Change reserve_ibft_region() to find_ibft_region()

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

move that just sfter find_smp_config()

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymore

Signed-off-by Yinghai Lu <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>
---
 arch/x86/kernel/setup.c            |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   11 ++++++++---
 include/linux/iscsi_ibft.h         |    8 ++++++--
 3 files changed, 26 insertions(+), 7 deletions(-)
arch/x86/kernel/setup.c +12 -2
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char
 early_param("elfcorehdr", setup_elfcorehdr);
 #endif
 
+static __init void reserve_ibft_region(void)
+{
+	unsigned long addr, size = 0;
+
+	addr = find_ibft_region(&size);
+
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ifbt");
+}
+
 #ifdef CONFIG_X86_RESERVE_LOW_64K
 static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 {

@@ -910,6 +920,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	find_smp_config();
 
+	reserve_ibft_region();
+
 	reserve_trampoline_memory();
 
 #ifdef CONFIG_ACPI_SLEEP

@@ -977,8 +989,6 @@ void __init setup_arch(char **cmdline_p)
 
 	dma32_reserve_bootmem();
 
-	reserve_ibft_region();
-
 #ifdef CONFIG_KVM_CLOCK
 	kvmclock_init();
 #endif
drivers/firmware/iscsi_ibft_find.c +8 -3
Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(ibft_addr);
  * Routine used to find the iSCSI Boot Format Table. The logical
  * kernel address is set in the ibft_addr global variable.
  */
-void __init reserve_ibft_region(void)
+unsigned long __init find_ibft_region(unsigned long *sizep)
 {
 	unsigned long pos;
 	unsigned int len = 0;

@@ -78,6 +78,11 @@ void __init reserve_ibft_region(void)
 			}
 		}
 	}
-	if (ibft_addr)
-		reserve_bootmem(pos, PAGE_ALIGN(len), BOOTMEM_DEFAULT);
+	if (ibft_addr) {
+		*sizep = PAGE_ALIGN(len);
+		return pos;
+	}
+
+	*sizep = 0;
+	return 0;
 }
include/linux/iscsi_ibft.h +6 -2
Index: linux-2.6/include/linux/iscsi_ibft.h
===================================================================
@@ -42,9 +42,13 @@ extern struct ibft_table_header *ibft_ad
  * mapped address is set in the ibft_addr variable.
  */
 #ifdef CONFIG_ISCSI_IBFT_FIND
-extern void __init reserve_ibft_region(void);
+unsigned long find_ibft_region(unsigned long *sizep);
 #else
-static inline void reserve_ibft_region(void) { }
+static inline unsigned long find_ibft_region(unsigned long *sizep)
+{
+	*sizep = 0;
+	return 0;
+}
 #endif
 
 #endif /* ISCSI_IBFT_H */
#1 lore raw
Quoted 6 lines · Yinghai Lu

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

What test vehicle did you use to find this bug?

.. snip..

arch/x86/kernel/setup.c static int __init setup_elfcorehdr(char · line 618

Quoted 2 lines
arch/x86/kernel/setup.c +2 -0
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ifbt");

							     ^^^^ - ibft
author
#2 lore raw
Quoted 9 lines · Konrad Rzeszutek Wilk
Quoted 6 lines · Yinghai Lu

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

What test vehicle did you use to find this bug?

code review for CONFIG_NO_BOOTMEM

reserve_bootmem() should only be used for converting entries reserved by reserved_early()

so When we are using CONFIG_NO_BOOTMEM, reserve_bootmem() is killed.

Quoted 6 lines

.. snip..

arch/x86/kernel/setup.c static int __init setup_elfcorehdr(char · line 618

Quoted 2 lines
arch/x86/kernel/setup.c +2 -0
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ifbt");

							     ^^^^ - ibft

thanks, will change it.

Yinghai

author
#3 lore raw

PATCH 1/1 ibft, x86: Change reserve_ibft_region() to find_ibft_region()

In reply to Yinghai Lu

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

move that just sfter find_smp_config()

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymorea

-v2: fix typo about ibft pointed by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>
---
 arch/x86/kernel/setup.c            |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   11 ++++++++---
 include/linux/iscsi_ibft.h         |    8 ++++++--
 3 files changed, 26 insertions(+), 7 deletions(-)
arch/x86/kernel/setup.c +12 -2
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char
 early_param("elfcorehdr", setup_elfcorehdr);
 #endif
 
+static __init void reserve_ibft_region(void)
+{
+	unsigned long addr, size = 0;
+
+	addr = find_ibft_region(&size);
+
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ibft");
+}
+
 #ifdef CONFIG_X86_RESERVE_LOW_64K
 static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 {

@@ -910,6 +920,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	find_smp_config();
 
+	reserve_ibft_region();
+
 	reserve_trampoline_memory();
 
 #ifdef CONFIG_ACPI_SLEEP

@@ -977,8 +989,6 @@ void __init setup_arch(char **cmdline_p)
 
 	dma32_reserve_bootmem();
 
-	reserve_ibft_region();
-
 #ifdef CONFIG_KVM_CLOCK
 	kvmclock_init();
 #endif
drivers/firmware/iscsi_ibft_find.c +8 -3
Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(ibft_addr);
  * Routine used to find the iSCSI Boot Format Table. The logical
  * kernel address is set in the ibft_addr global variable.
  */
-void __init reserve_ibft_region(void)
+unsigned long __init find_ibft_region(unsigned long *sizep)
 {
 	unsigned long pos;
 	unsigned int len = 0;

@@ -78,6 +78,11 @@ void __init reserve_ibft_region(void)
 			}
 		}
 	}
-	if (ibft_addr)
-		reserve_bootmem(pos, PAGE_ALIGN(len), BOOTMEM_DEFAULT);
+	if (ibft_addr) {
+		*sizep = PAGE_ALIGN(len);
+		return pos;
+	}
+
+	*sizep = 0;
+	return 0;
 }
include/linux/iscsi_ibft.h +6 -2
Index: linux-2.6/include/linux/iscsi_ibft.h
===================================================================
@@ -42,9 +42,13 @@ extern struct ibft_table_header *ibft_ad
  * mapped address is set in the ibft_addr variable.
  */
 #ifdef CONFIG_ISCSI_IBFT_FIND
-extern void __init reserve_ibft_region(void);
+unsigned long find_ibft_region(unsigned long *sizep);
 #else
-static inline void reserve_ibft_region(void) { }
+static inline unsigned long find_ibft_region(unsigned long *sizep)
+{
+	*sizep = 0;
+	return 0;
+}
 #endif
 
 #endif /* ISCSI_IBFT_H */
#4 lore raw
Quoted 32 lines · On Thu, 01 Apr 2010 14:32:43 -0700 Yinghai Lu wrote:

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

move that just sfter find_smp_config()

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymorea

-v2: fix typo about ibft pointed by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>
---
 arch/x86/kernel/setup.c            |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   11 ++++++++---
 include/linux/iscsi_ibft.h         |    8 ++++++--
 3 files changed, 26 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================

arch/x86/kernel/setup.c +0 -0
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char
 early_param("elfcorehdr", setup_elfcorehdr);

arch/x86/kernel/setup.c static int __init setup_elfcorehdr(char · line 618

Quoted 10 lines
arch/x86/kernel/setup.c +8 -0
 #endif
 
+static __init void reserve_ibft_region(void)
+{
+	unsigned long addr, size = 0;
+
+	addr = find_ibft_region(&size);
+
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ibft");

where is the correction?

Quoted 61 lines
arch/x86/kernel/setup.c +4 -2
+}
+
 #ifdef CONFIG_X86_RESERVE_LOW_64K
 static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 {

@@ -910,6 +920,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	find_smp_config();
 
+	reserve_ibft_region();
+
 	reserve_trampoline_memory();
 
 #ifdef CONFIG_ACPI_SLEEP

@@ -977,8 +989,6 @@ void __init setup_arch(char **cmdline_p)
 
 	dma32_reserve_bootmem();
 
-	reserve_ibft_region();
-
 #ifdef CONFIG_KVM_CLOCK
 	kvmclock_init();
 #endif

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================

drivers/firmware/iscsi_ibft_find.c +8 -3
@@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(ibft_addr);
  * Routine used to find the iSCSI Boot Format Table. The logical
  * kernel address is set in the ibft_addr global variable.
  */
-void __init reserve_ibft_region(void)
+unsigned long __init find_ibft_region(unsigned long *sizep)
 {
 	unsigned long pos;
 	unsigned int len = 0;

@@ -78,6 +78,11 @@ void __init reserve_ibft_region(void)
 			}
 		}
 	}
-	if (ibft_addr)
-		reserve_bootmem(pos, PAGE_ALIGN(len), BOOTMEM_DEFAULT);
+	if (ibft_addr) {
+		*sizep = PAGE_ALIGN(len);
+		return pos;
+	}
+
+	*sizep = 0;
+	return 0;
 }

Index: linux-2.6/include/linux/iscsi_ibft.h
===================================================================

include/linux/iscsi_ibft.h +1 -1
@@ -42,9 +42,13 @@ extern struct ibft_table_header *ibft_ad
  * mapped address is set in the ibft_addr variable.
  */
 #ifdef CONFIG_ISCSI_IBFT_FIND
-extern void __init reserve_ibft_region(void);
+unsigned long find_ibft_region(unsigned long *sizep);
 #else

include/linux/iscsi_ibft.h extern struct ibft_table_header *ibft_ad

Quoted 10 lines
include/linux/iscsi_ibft.h +5 -2
-static inline void reserve_ibft_region(void) { }
+static inline unsigned long find_ibft_region(unsigned long *sizep)
+{
+	*sizep = 0;
+	return 0;
+}
 #endif
 
 #endif /* ISCSI_IBFT_H */
--

---
~Randy

author
#5 lore raw
Quoted 36 lines · Randy Dunlap
Quoted 34 lines · On Thu, 01 Apr 2010 14:32:43 -0700 Yinghai Lu wrote:

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

move that just sfter find_smp_config()

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymorea

-v2: fix typo about ibft pointed by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>

---
 arch/x86/kernel/setup.c            |   14 ++++++++++++-- drivers/firmware/iscsi_ibft_find.c |   11 ++++++++--- include/linux/iscsi_ibft.h         |    8 ++++++--

 3 files changed, 26 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================

arch/x86/kernel/setup.c +0 -0
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char
  early_param("elfcorehdr", setup_elfcorehdr);
  #endif
Quoted 10 lines

arch/x86/kernel/setup.c static int __init setup_elfcorehdr(char · line 618

Quoted 8 lines
arch/x86/kernel/setup.c +8 -0
+static __init void reserve_ibft_region(void)
+{
+     unsigned long addr, size = 0;
+
+     addr = find_ibft_region(&size);
+
+     if (size)
+             reserve_early_overlap_ok(addr, addr + size, "ibft");

       where is the correction?

-v1 has

+ if (size)
+ reserve_early_overlap_ok(addr, addr + size, "ifbt");

Yinghai

#6 lore raw
Quoted 48 lines · On Thu, 1 Apr 2010 15:00:05 -0700 Yinghai Lu wrote:
Quoted 36 lines · Randy Dunlap
Quoted 34 lines · On Thu, 01 Apr 2010 14:32:43 -0700 Yinghai Lu wrote:

So arch code could decide the way to reserve the ibft.

And We should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS.

move that just sfter find_smp_config()

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymorea

-v2: fix typo about ibft pointed by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>

---
 arch/x86/kernel/setup.c            |   14 ++++++++++++-- drivers/firmware/iscsi_ibft_find.c |   11 ++++++++--- include/linux/iscsi_ibft.h         |    8 ++++++--

 3 files changed, 26 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================

arch/x86/kernel/setup.c +0 -0
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char
  early_param("elfcorehdr", setup_elfcorehdr);
  #endif
Quoted 10 lines

arch/x86/kernel/setup.c static int __init setup_elfcorehdr(char · line 618

Quoted 8 lines
arch/x86/kernel/setup.c +8 -0
+static __init void reserve_ibft_region(void)
+{
+     unsigned long addr, size = 0;
+
+     addr = find_ibft_region(&size);
+
+     if (size)
+             reserve_early_overlap_ok(addr, addr + size, "ibft");

       where is the correction?

Quoted 4 lines

-v1 has

+ if (size)
+ reserve_early_overlap_ok(addr, addr + size, "ifbt");

Thanks. My eyes are blurry. ;)

---
~Randy

author
#7 lore raw
In reply to Yinghai Lu
Commit-ID:  042be38e6106ed70b42d096ab4a1ed4187e510e6
Gitweb:     http://git.kernel.org/tip/042be38e6106ed70b42d096ab4a1ed4187e510e6
Author:     Yinghai Lu <[email protected]>
AuthorDate: Thu, 1 Apr 2010 14:32:43 -0700
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Thu, 1 Apr 2010 16:12:48 -0700

ibft, x86: Change reserve_ibft_region() to find_ibft_region()

This allows arch code could decide the way to reserve the ibft.

And we should reserve ibft as early as possible, instead of BOOTMEM stage, in case the table is in RAM range and is not reserved by BIOS (this will often be the case.)

Move to just after find_smp_config().

Also when CONFIG_NO_BOOTMEM=y, We will not have reserve_bootmem() anymore.

-v2: fix typo about ibft pointed by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

LKML-Reference: <[email protected]>

Cc Pekka Enberg <[email protected]>
Cc Peter Jones <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
CC Jan Beulich <[email protected]>
Signed-off-by H. Peter Anvin <[email protected]>
---
 arch/x86/kernel/setup.c            |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   11 ++++++++---
 include/linux/iscsi_ibft.h         |    8 ++++++--
 3 files changed, 26 insertions(+), 7 deletions(-)
arch/x86/kernel/setup.c +12 -2
index d76e185..580e6b3 100644
@@ -608,6 +608,16 @@ static int __init setup_elfcorehdr(char *arg)
 early_param("elfcorehdr", setup_elfcorehdr);
 #endif
 
+static __init void reserve_ibft_region(void)
+{
+	unsigned long addr, size = 0;
+
+	addr = find_ibft_region(&size);
+
+	if (size)
+		reserve_early_overlap_ok(addr, addr + size, "ibft");
+}
+
 #ifdef CONFIG_X86_RESERVE_LOW_64K
 static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 {

@@ -910,6 +920,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	find_smp_config();
 
+	reserve_ibft_region();
+
 	reserve_trampoline_memory();
 
 #ifdef CONFIG_ACPI_SLEEP

@@ -977,8 +989,6 @@ void __init setup_arch(char **cmdline_p)
 
 	dma32_reserve_bootmem();
 
-	reserve_ibft_region();
-
 #ifdef CONFIG_KVM_CLOCK
 	kvmclock_init();
 #endif
drivers/firmware/iscsi_ibft_find.c +8 -3
index dfb15c0..8f5d9e2 100644
@@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(ibft_addr);
  * Routine used to find the iSCSI Boot Format Table. The logical
  * kernel address is set in the ibft_addr global variable.
  */
-void __init reserve_ibft_region(void)
+unsigned long __init find_ibft_region(unsigned long *sizep)
 {
 	unsigned long pos;
 	unsigned int len = 0;

@@ -78,6 +78,11 @@ void __init reserve_ibft_region(void)
 			}
 		}
 	}
-	if (ibft_addr)
-		reserve_bootmem(pos, PAGE_ALIGN(len), BOOTMEM_DEFAULT);
+	if (ibft_addr) {
+		*sizep = PAGE_ALIGN(len);
+		return pos;
+	}
+
+	*sizep = 0;
+	return 0;
 }
include/linux/iscsi_ibft.h +6 -2
index 6092487..d2e4042 100644
@@ -42,9 +42,13 @@ extern struct ibft_table_header *ibft_addr;
  * mapped address is set in the ibft_addr variable.
  */
 #ifdef CONFIG_ISCSI_IBFT_FIND
-extern void __init reserve_ibft_region(void);
+unsigned long find_ibft_region(unsigned long *sizep);
 #else
-static inline void reserve_ibft_region(void) { }
+static inline unsigned long find_ibft_region(unsigned long *sizep)
+{
+	*sizep = 0;
+	return 0;
+}
 #endif
 
 #endif /* ISCSI_IBFT_H */