On Sat, Feb 28, 2009 at 07:10:18PM -0800, Yinghai Lu wrote:
> Kevin O'Connor wrote:
> > On Sun, Feb 22, 2009 at 10:14:56PM -0800, Yinghai Lu wrote:
> >> please check
> >>
> >> [PATCH] x86: check physptr with max_low_pfn on 32bit
> >
> > Thanks for looking at this issue.
> >
> > Unfortunately, the kernel still does not work with the applied patch.
> > Neither 32bit nor 64bit kernels boot. (Note, I applied the patch to
> > Linus' git, and I had to replace mpf->physptr with mpf->mpf_physptr.)
>
> please add this patch too
>
> [PATCH] x86: ioremap mptable
Thanks. Both 32bit and 64bit kernels are working now.
-Kevin
Impact: fix boot with mptable above max_low_mapped
try to use early_ioremap it.
v2: also get the exact size for reserve_bootmem in case we got big size than 4k
Signed-off-by: Yinghai Lu <[email protected]>
Reported-and-tested-by: Kevin O'Connor <[email protected]>
---
arch/x86/kernel/mpparse.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/x86/kernel/mpparse.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/mpparse.c
+++ linux-2.6/arch/x86/kernel/mpparse.c
@@ -611,12 +611,20 @@ static void __init __get_smp_config(unsi
construct_default_ISA_mptable(mpf->feature1);
} else if (mpf->physptr) {
+ struct mpc_table *mpc;
+ unsigned long size;
+ mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
+ size = mpc->length;
+ apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", (unsigned long)mpf->physptr,
+ mpf->physptr + size);
+ early_iounmap(mpc, PAGE_SIZE);
+ mpc = early_ioremap(mpf->physptr, size);
/*
* Read the physical hardware table. Anything here will
* override the defaults.
*/
- if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
+ if (!smp_read_mpc(mpc, early)) {
#ifdef CONFIG_X86_LOCAL_APIC
smp_found_config = 0;
#endif
@@ -624,9 +632,12 @@ static void __init __get_smp_config(unsi
"BIOS bug, MP table errors detected!...\n");
printk(KERN_ERR "... disabling SMP support. "
"(tell your hw vendor)\n");
+ early_iounmap(mpc, size);
return;
}
+ early_iounmap(mpc, size);
+
if (early)
return;
#ifdef CONFIG_X86_IO_APIC
@@ -700,7 +711,12 @@ static int __init smp_scan_config(unsign
reserve_bootmem_generic(virt_to_phys(mpf), PAGE_SIZE,
BOOTMEM_DEFAULT);
if (mpf->physptr) {
- unsigned long size = PAGE_SIZE;
+ struct mpc_table *mpc;
+ unsigned long size;
+
+ mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
+ size = mpc->length;
+ early_iounmap(mpc, PAGE_SIZE);
#ifdef CONFIG_X86_32
/*
* We cannot access to MPC table to compute[PATCH] x86: ioremap mptable -v2
Impact: fix boot with mptable above max_low_mapped
try to use early_ioremap it.
v2: also get the exact size for reserve_bootmem in case we got big size than 4k
Signed-off-by: Yinghai Lu <[email protected]>
Reported-and-tested-by: Kevin O'Connor <[email protected]>
---
arch/x86/kernel/mpparse.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/x86/kernel/mpparse.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/mpparse.c
+++ linux-2.6/arch/x86/kernel/mpparse.c
@@ -611,12 +611,20 @@ static void __init __get_smp_config(unsi
construct_default_ISA_mptable(mpf->feature1);
} else if (mpf->physptr) {
+ struct mpc_table *mpc;
+ unsigned long size;
+ mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
+ size = mpc->length;
+ apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", (unsigned long)mpf->physptr,
+ mpf->physptr + size);
+ early_iounmap(mpc, PAGE_SIZE);
+ mpc = early_ioremap(mpf->physptr, size);
/*
* Read the physical hardware table. Anything here will
* override the defaults.
*/
- if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
+ if (!smp_read_mpc(mpc, early)) {
#ifdef CONFIG_X86_LOCAL_APIC
smp_found_config = 0;
#endif
@@ -624,9 +632,12 @@ static void __init __get_smp_config(unsi
"BIOS bug, MP table errors detected!...\n");
printk(KERN_ERR "... disabling SMP support. "
"(tell your hw vendor)\n");
+ early_iounmap(mpc, size);
return;
}
+ early_iounmap(mpc, size);
+
if (early)
return;
#ifdef CONFIG_X86_IO_APIC
@@ -700,7 +711,12 @@ static int __init smp_scan_config(unsign
reserve_bootmem_generic(virt_to_phys(mpf), PAGE_SIZE,
BOOTMEM_DEFAULT);
if (mpf->physptr) {
- unsigned long size = PAGE_SIZE;
+ struct mpc_table *mpc;
+ unsigned long size;
+
+ mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
+ size = mpc->length;
+ early_iounmap(mpc, PAGE_SIZE);
#ifdef CONFIG_X86_32
/*
* We cannot access to MPC table to compute
* Yinghai Lu <[email protected]> wrote:
> Impact: fix boot with mptable above max_low_mapped
>
> try to use early_ioremap it.
>
> v2: also get the exact size for reserve_bootmem in case we got big size than 4k
>
> Signed-off-by: Yinghai Lu <[email protected]>
> Reported-and-tested-by: Kevin O'Connor <[email protected]>
>
> ---
> arch/x86/kernel/mpparse.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/arch/x86/kernel/mpparse.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/mpparse.c
> +++ linux-2.6/arch/x86/kernel/mpparse.c
> @@ -611,12 +611,20 @@ static void __init __get_smp_config(unsi
> construct_default_ISA_mptable(mpf->feature1);
>
> } else if (mpf->physptr) {
> + struct mpc_table *mpc;
> + unsigned long size;
>
> + mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
> + size = mpc->length;
> + apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", (unsigned long)mpf->physptr,
> + mpf->physptr + size);
> + early_iounmap(mpc, PAGE_SIZE);
> + mpc = early_ioremap(mpf->physptr, size);
no objections, but this bit of __get_smp_config() needs to be
done cleaner - the whole mpf->physptr != 0 bit should probably
go into a helper function.
Ingo
* Ingo Molnar <[email protected]> wrote:
> > } else if (mpf->physptr) {
> > + struct mpc_table *mpc;
> > + unsigned long size;
> >
> > + mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
> > + size = mpc->length;
> > + apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", (unsigned long)mpf->physptr,
> > + mpf->physptr + size);
> > + early_iounmap(mpc, PAGE_SIZE);
> > + mpc = early_ioremap(mpf->physptr, size);
>
> no objections, but this bit of __get_smp_config() needs to be
> done cleaner - the whole mpf->physptr != 0 bit should probably
> go into a helper function.
and if you do that it should be done via two patches, in two
steps: first patch is a pure cleanup that moves this bit of
__get_smp_config() into a helper function. The second patch then
adds the early_ioremap().
Ingo
Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> Impact: fix boot with mptable above max_low_mapped
>>
>> try to use early_ioremap it.
>>
>> v2: also get the exact size for reserve_bootmem in case we got big size than 4k
>>
>> Signed-off-by: Yinghai Lu <[email protected]>
>> Reported-and-tested-by: Kevin O'Connor <[email protected]>
>>
>> ---
>> arch/x86/kernel/mpparse.c | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> Index: linux-2.6/arch/x86/kernel/mpparse.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/mpparse.c
>> +++ linux-2.6/arch/x86/kernel/mpparse.c
>> @@ -611,12 +611,20 @@ static void __init __get_smp_config(unsi
>> construct_default_ISA_mptable(mpf->feature1);
>>
>> } else if (mpf->physptr) {
>> + struct mpc_table *mpc;
>> + unsigned long size;
>>
>> + mpc = early_ioremap(mpf->physptr, PAGE_SIZE);
>> + size = mpc->length;
>> + apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", (unsigned long)mpf->physptr,
>> + mpf->physptr + size);
>> + early_iounmap(mpc, PAGE_SIZE);
>> + mpc = early_ioremap(mpf->physptr, size);
>
> no objections, but this bit of __get_smp_config() needs to be
> done cleaner - the whole mpf->physptr != 0 bit should probably
> go into a helper function.
>
please check
[PATCH] x86: ioremap mptable -v3
Impact: fix boot with mptable above max_low_mapped
try to use early_ioremap it.
v2: also get the exact size for reserve_bootmem in case we got big size than 4k
V3: according to Ingo, seperate get_mpc_size()
Signed-off-by: Yinghai Lu <[email protected]>
Reported-and-tested-by: Kevin O'Connor <[email protected]>
---
arch/x86/kernel/mpparse.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/x86/kernel/mpparse.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/mpparse.c
+++ linux-2.6/arch/x86/kernel/mpparse.c
@@ -558,6 +558,19 @@ static inline void __init construct_defa
static struct mpf_intel *mpf_found;
+static unsigned long __init get_mpc_size(unsigned long physptr)
+{
+ struct mpc_table *mpc;
+ unsigned long size;
+
+ mpc = early_ioremap(physptr, PAGE_SIZE);
+ size = mpc->length;
+ early_iounmap(mpc, PAGE_SIZE);
+ apic_printk(APIC_VERBOSE, " mpc: %lx-%lx\n", physptr, physptr + size);
+
+ return size;
+}
+
/*
* Scan the memory blocks for an SMP configuration block.
*/
@@ -611,12 +624,16 @@ static void __init __get_smp_config(unsi
construct_default_ISA_mptable(mpf->feature1);
} else if (mpf->physptr) {
+ struct mpc_table *mpc;
+ unsigned long size;
+ size = get_mpc_size(mpf->physptr);
+ mpc = early_ioremap(mpf->physptr, size);
/*
* Read the physical hardware table. Anything here will
* override the defaults.
*/
- if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
+ if (!smp_read_mpc(mpc, early)) {
#ifdef CONFIG_X86_LOCAL_APIC
smp_found_config = 0;
#endif
@@ -624,9 +641,12 @@ static void __init __get_smp_config(unsi
"BIOS bug, MP table errors detected!...\n");
printk(KERN_ERR "... disabling SMP support. "
"(tell your hw vendor)\n");
+ early_iounmap(mpc, size);
return;
}
+ early_iounmap(mpc, size);
+
if (early)
return;
#ifdef CONFIG_X86_IO_APIC
@@ -700,7 +720,7 @@ static int __init smp_scan_config(unsign
reserve_bootmem_generic(virt_to_phys(mpf), PAGE_SIZE,
BOOTMEM_DEFAULT);
if (mpf->physptr) {
- unsigned long size = PAGE_SIZE;
+ unsigned long size = get_mpc_size(mpf->physptr);
#ifdef CONFIG_X86_32
/*
* We cannot access to MPC table to compute
* Yinghai Lu <[email protected]> wrote:
> V3: according to Ingo, seperate get_mpc_size()
No, that was not my suggestion. My suggestion was to separate
this whole 'else if' branch:
> } else if (mpf->physptr) {
> + struct mpc_table *mpc;
> + unsigned long size;
>
> + size = get_mpc_size(mpf->physptr);
> + mpc = early_ioremap(mpf->physptr, size);
> /*
> * Read the physical hardware table. Anything here will
> * override the defaults.
> */
> - if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
> + if (!smp_read_mpc(mpc, early)) {
> #ifdef CONFIG_X86_LOCAL_APIC
> smp_found_config = 0;
> #endif
... into a helper function - if that improves the code. Your
patch does early_ioremap, iounmap then ioremap and iounmap -
quite pointlessly.
You should resist cleanup suggestions that make the code worse,
even if it comes from a maintainer :-)
Ingo
Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> V3: according to Ingo, seperate get_mpc_size()
>
> No, that was not my suggestion. My suggestion was to separate
> this whole 'else if' branch:
>
>> } else if (mpf->physptr) {
>> + struct mpc_table *mpc;
>> + unsigned long size;
>>
>> + size = get_mpc_size(mpf->physptr);
>> + mpc = early_ioremap(mpf->physptr, size);
>> /*
>> * Read the physical hardware table. Anything here will
>> * override the defaults.
>> */
>> - if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
>> + if (!smp_read_mpc(mpc, early)) {
>> #ifdef CONFIG_X86_LOCAL_APIC
>> smp_found_config = 0;
>> #endif
>
> ... into a helper function - if that improves the code.
oh, i missed it
> Your patch does early_ioremap, iounmap then ioremap and iounmap -
> quite pointlessly.
try to get exact mpc size.
>
> You should resist cleanup suggestions that make the code worse,
> even if it comes from a maintainer :-)
we could do that later. to make __get_smp_config smaller and readable.
YH
* Yinghai Lu <[email protected]> wrote:
> Ingo Molnar wrote:
> > * Yinghai Lu <[email protected]> wrote:
> >
> >> V3: according to Ingo, seperate get_mpc_size()
> >
> > No, that was not my suggestion. My suggestion was to separate
> > this whole 'else if' branch:
> >
> >> } else if (mpf->physptr) {
> >> + struct mpc_table *mpc;
> >> + unsigned long size;
> >>
> >> + size = get_mpc_size(mpf->physptr);
> >> + mpc = early_ioremap(mpf->physptr, size);
> >> /*
> >> * Read the physical hardware table. Anything here will
> >> * override the defaults.
> >> */
> >> - if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
> >> + if (!smp_read_mpc(mpc, early)) {
> >> #ifdef CONFIG_X86_LOCAL_APIC
> >> smp_found_config = 0;
> >> #endif
> >
> > ... into a helper function - if that improves the code.
> oh, i missed it
> > Your patch does early_ioremap, iounmap then ioremap and iounmap -
> > quite pointlessly.
> try to get exact mpc size.
> >
> > You should resist cleanup suggestions that make the code worse,
> > even if it comes from a maintainer :-)
>
> we could do that later. to make __get_smp_config smaller and readable.
No, do it in two separate patches please: _first_ do the whole
cleanup of these functions - on the assumption and expectation
that it wont break anything. Then add the early_ioremap() change
in a second patch - on top of the cleanup patch.
If we do a cleanup _after_ a functional change then we make the
feature patch harder to revert and harder to fix as well. We'd
always have to 'see through' the cleanup patch when considering
breakages caused by the functional patch.
Like i suggested in my first reply ;-)
Ingo