[PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data (e.g.
max_low_pfn) that haven't been setup yet in such early boot phase. To fix the
issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's physical
address.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/microcode_intel_early.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index 7890bc8..5992ee8 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -90,13 +90,13 @@ microcode_phys(struct microcode_intel **mc_saved_tmp,
 	struct microcode_intel ***mc_saved;
 
 	mc_saved = (struct microcode_intel ***)
-		   __pa_symbol(&mc_saved_data->mc_saved);
+		   __pa_nodebug(&mc_saved_data->mc_saved);
 	for (i = 0; i < mc_saved_data->mc_saved_count; i++) {
 		struct microcode_intel *p;
 
 		p = *(struct microcode_intel **)
-			__pa(mc_saved_data->mc_saved + i);
-		mc_saved_tmp[i] = (struct microcode_intel *)__pa(p);
+			__pa_nodebug(mc_saved_data->mc_saved + i);
+		mc_saved_tmp[i] = (struct microcode_intel *)__pa_nodebug(p);
 	}
 }
 #endif
@@ -562,7 +562,7 @@ scan_microcode(unsigned long start, unsigned long end,
 	struct cpio_data cd;
 	long offset = 0;
 #ifdef CONFIG_X86_32
-	char *p = (char *)__pa_symbol(ucode_name);
+	char *p = (char *)__pa_nodebug(ucode_name);
 #else
 	char *p = ucode_name;
 #endif
@@ -630,8 +630,8 @@ static void __cpuinit print_ucode(struct ucode_cpu_info *uci)
 	if (mc_intel == NULL)
 		return;
 
-	delay_ucode_info_p = (int *)__pa_symbol(&delay_ucode_info);
-	current_mc_date_p = (int *)__pa_symbol(&current_mc_date);
+	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
+	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
 
 	*delay_ucode_info_p = 1;
 	*current_mc_date_p = mc_intel->hdr.date;
@@ -741,15 +741,15 @@ load_ucode_intel_bsp(void)
 #ifdef CONFIG_X86_32
 	struct boot_params *boot_params_p;
 
-	boot_params_p = (struct boot_params *)__pa_symbol(&boot_params);
+	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
 	ramdisk_image = boot_params_p->hdr.ramdisk_image;
 	ramdisk_size  = boot_params_p->hdr.ramdisk_size;
 	initrd_start_early = ramdisk_image;
 	initrd_end_early = initrd_start_early + ramdisk_size;
 
 	_load_ucode_intel_bsp(
-		(struct mc_saved_data *)__pa_symbol(&mc_saved_data),
-		(unsigned long *)__pa_symbol(&mc_saved_in_initrd),
+		(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
+		(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
 		initrd_start_early, initrd_end_early, &uci);
 #else
 	ramdisk_image = boot_params.hdr.ramdisk_image;
@@ -772,10 +772,10 @@ void __cpuinit load_ucode_intel_ap(void)
 	unsigned long *initrd_start_p;
 
 	mc_saved_in_initrd_p =
-		(unsigned long *)__pa_symbol(mc_saved_in_initrd);
-	mc_saved_data_p = (struct mc_saved_data *)__pa_symbol(&mc_saved_data);
-	initrd_start_p = (unsigned long *)__pa_symbol(&initrd_start);
-	initrd_start_addr = (unsigned long)__pa_symbol(*initrd_start_p);
+		(unsigned long *)__pa_nodebug(mc_saved_in_initrd);
+	mc_saved_data_p = (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
+	initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
+	initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);
 #else
 	mc_saved_data_p = &mc_saved_data;
 	mc_saved_in_initrd_p = mc_saved_in_initrd;
-- 
1.8.0.1

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Yinghai Lu]

On Tue, Mar 19, 2013 at 8:04 AM, Fenghua Yu <fenghua.yu@intel.com> wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
>
> In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data (e.g.
> max_low_pfn) that haven't been setup yet in such early boot phase. To fix the
> issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's physical
> address.

That explanation is not precise.

__phys_addr in 32bit when DEBUG_VIRTUAL is set, it does have check
        /* max_low_pfn is set early, but not _that_ early */
        if (max_low_pfn) {
         ....
        }
the reason should be:
in head_32.S, 32bit flat mode, you need to switch to phys addr to access
global variable max_low_pfn.

Thanks

Yinghai

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[H. Peter Anvin]

On 03/19/2013 04:15 PM, Yinghai Lu wrote:
> On Tue, Mar 19, 2013 at 8:04 AM, Fenghua Yu <fenghua.yu@intel.com> wrote:
>> From: Fenghua Yu <fenghua.yu@intel.com>
>>
>> In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data (e.g.
>> max_low_pfn) that haven't been setup yet in such early boot phase. To fix the
>> issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's physical
>> address.
> 
> That explanation is not precise.
> 
> __phys_addr in 32bit when DEBUG_VIRTUAL is set, it does have check
>         /* max_low_pfn is set early, but not _that_ early */
>         if (max_low_pfn) {
>          ....
>         }
> the reason should be:
> in head_32.S, 32bit flat mode, you need to switch to phys addr to access
> global variable max_low_pfn.
> 

Uh, no.

It already does that.  The problem is that __pa_symbol() breaks with
CONFIG_DEBUG_VIRTUAL at this point; we need to use __pa_nodebug().

	-hpa

RE: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Yu, Fenghua]

> From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
> Yinghai Lu
> On Tue, Mar 19, 2013 at 8:04 AM, Fenghua Yu <fenghua.yu@intel.com>
> wrote:
> > From: Fenghua Yu <fenghua.yu@intel.com>
> >
> > In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data
> (e.g.
> > max_low_pfn) that haven't been setup yet in such early boot phase. To
> fix the
> > issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's
> physical
> > address.
> 
> That explanation is not precise.
> 
> __phys_addr in 32bit when DEBUG_VIRTUAL is set, it does have check
>         /* max_low_pfn is set early, but not _that_ early */
>         if (max_low_pfn) {
>          ....
>         }
> the reason should be:
> in head_32.S, 32bit flat mode, you need to switch to phys addr to
> access
> global variable max_low_pfn.

Max_low_pfn is not set up yet at this point, but __pa_symbol() reads it
if CONFIG_DEBUG_VIRTUAL. There is no need to access max_low_pfn or any
other data that is not setup yet if CONFIG_DEBUG_VIRTUAL.

So I think the explanation is ok.

Thanks.

-Fenghua

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Yinghai Lu]

On Tue, Mar 19, 2013 at 4:36 PM, Yu, Fenghua <fenghua.yu@intel.com> wrote:
>> From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
>> Yinghai Lu
>> On Tue, Mar 19, 2013 at 8:04 AM, Fenghua Yu <fenghua.yu@intel.com>
>> wrote:
>> > From: Fenghua Yu <fenghua.yu@intel.com>
>> >
>> > In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data
>> (e.g.
>> > max_low_pfn) that haven't been setup yet in such early boot phase. To
>> fix the
>> > issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's
>> physical
>> > address.
>>
>> That explanation is not precise.
>>
>> __phys_addr in 32bit when DEBUG_VIRTUAL is set, it does have check
>>         /* max_low_pfn is set early, but not _that_ early */
>>         if (max_low_pfn) {
>>          ....
>>         }
>> the reason should be:
>> in head_32.S, 32bit flat mode, you need to switch to phys addr to
>> access
>> global variable max_low_pfn.
>
> Max_low_pfn is not set up yet at this point, but __pa_symbol() reads it
> if CONFIG_DEBUG_VIRTUAL. There is no need to access max_low_pfn or any
> other data that is not setup yet if CONFIG_DEBUG_VIRTUAL.
>
> So I think the explanation is ok.


Changlog said:

 In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data
(e.g. max_low_pfn) that haven't been setup yet in such early boot phase.

---

So do you mean we have to change all __pa_symbol before
setup.c::setup_arch/find_low_pfn_range ?
because only at that time max_low_pfn get setup.

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[H. Peter Anvin]

On 03/19/2013 04:48 PM, Yinghai Lu wrote:
> 
> So do you mean we have to change all __pa_symbol before
> setup.c::setup_arch/find_low_pfn_range ?
> because only at that time max_low_pfn get setup.
> 

No, I suspect the problem is that the debugging code fails to notice
that max_low_pfn isn't set up yet because it is in linear mode, and so
it reads the wrong address when it checks max_low_pfn.

	-hpa

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Tetsuo Handa]

Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data (e.g.
> max_low_pfn) that haven't been setup yet in such early boot phase. To fix the
> issue, __pa_nodebug() replaces __pa_symbol() to get a global symbol's physical
> address.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/kernel/microcode_intel_early.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
This patch fixes my problem. Thank you.

[tip:x86/urgent] x86-32, microcode_intel_early: Fix crash with CONFIG_DEBUG_VIRTUAL

[tip-bot for Fenghua Yu]

Commit-ID:  c83a9d5e425d4678b05ca058fec6254f18601474
Gitweb:     http://git.kernel.org/tip/c83a9d5e425d4678b05ca058fec6254f18601474
Author:     Fenghua Yu <fenghua.yu@intel.com>
AuthorDate: Tue, 19 Mar 2013 08:04:44 -0700
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Tue, 19 Mar 2013 19:51:08 -0700

x86-32, microcode_intel_early: Fix crash with CONFIG_DEBUG_VIRTUAL

In 32-bit, __pa_symbol() in CONFIG_DEBUG_VIRTUAL accesses kernel data
(e.g.  max_low_pfn) that not only hasn't been setup yet in such early
boot phase, but since we are in linear mode, cannot even be detected
as uninitialized.

Thus, use __pa_nodebug() rather than __pa_symbol() to get a global
symbol's physical address.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Link: http://lkml.kernel.org/r/1363705484-27645-1-git-send-email-fenghua.yu@intel.com
Reported-and-tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/microcode_intel_early.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index 7890bc8..5992ee8 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -90,13 +90,13 @@ microcode_phys(struct microcode_intel **mc_saved_tmp,
 	struct microcode_intel ***mc_saved;
 
 	mc_saved = (struct microcode_intel ***)
-		   __pa_symbol(&mc_saved_data->mc_saved);
+		   __pa_nodebug(&mc_saved_data->mc_saved);
 	for (i = 0; i < mc_saved_data->mc_saved_count; i++) {
 		struct microcode_intel *p;
 
 		p = *(struct microcode_intel **)
-			__pa(mc_saved_data->mc_saved + i);
-		mc_saved_tmp[i] = (struct microcode_intel *)__pa(p);
+			__pa_nodebug(mc_saved_data->mc_saved + i);
+		mc_saved_tmp[i] = (struct microcode_intel *)__pa_nodebug(p);
 	}
 }
 #endif
@@ -562,7 +562,7 @@ scan_microcode(unsigned long start, unsigned long end,
 	struct cpio_data cd;
 	long offset = 0;
 #ifdef CONFIG_X86_32
-	char *p = (char *)__pa_symbol(ucode_name);
+	char *p = (char *)__pa_nodebug(ucode_name);
 #else
 	char *p = ucode_name;
 #endif
@@ -630,8 +630,8 @@ static void __cpuinit print_ucode(struct ucode_cpu_info *uci)
 	if (mc_intel == NULL)
 		return;
 
-	delay_ucode_info_p = (int *)__pa_symbol(&delay_ucode_info);
-	current_mc_date_p = (int *)__pa_symbol(&current_mc_date);
+	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
+	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
 
 	*delay_ucode_info_p = 1;
 	*current_mc_date_p = mc_intel->hdr.date;
@@ -741,15 +741,15 @@ load_ucode_intel_bsp(void)
 #ifdef CONFIG_X86_32
 	struct boot_params *boot_params_p;
 
-	boot_params_p = (struct boot_params *)__pa_symbol(&boot_params);
+	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
 	ramdisk_image = boot_params_p->hdr.ramdisk_image;
 	ramdisk_size  = boot_params_p->hdr.ramdisk_size;
 	initrd_start_early = ramdisk_image;
 	initrd_end_early = initrd_start_early + ramdisk_size;
 
 	_load_ucode_intel_bsp(
-		(struct mc_saved_data *)__pa_symbol(&mc_saved_data),
-		(unsigned long *)__pa_symbol(&mc_saved_in_initrd),
+		(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
+		(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
 		initrd_start_early, initrd_end_early, &uci);
 #else
 	ramdisk_image = boot_params.hdr.ramdisk_image;
@@ -772,10 +772,10 @@ void __cpuinit load_ucode_intel_ap(void)
 	unsigned long *initrd_start_p;
 
 	mc_saved_in_initrd_p =
-		(unsigned long *)__pa_symbol(mc_saved_in_initrd);
-	mc_saved_data_p = (struct mc_saved_data *)__pa_symbol(&mc_saved_data);
-	initrd_start_p = (unsigned long *)__pa_symbol(&initrd_start);
-	initrd_start_addr = (unsigned long)__pa_symbol(*initrd_start_p);
+		(unsigned long *)__pa_nodebug(mc_saved_in_initrd);
+	mc_saved_data_p = (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
+	initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
+	initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);
 #else
 	mc_saved_data_p = &mc_saved_data;
 	mc_saved_in_initrd_p = mc_saved_in_initrd;

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> +	initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
> +	initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);

We should probably use __pa_early() for that - aliased to __pa_nodebug(). 
That would self-document the purpose.

Thanks,

	Ingo

Re: [PATCH] x86/microcode_intel_early.c: Get 32-bit physical address by __pa_nodebug()

[H. Peter Anvin]

On 03/21/2013 03:40 AM, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
>> +	initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
>> +	initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);
> 
> We should probably use __pa_early() for that - aliased to __pa_nodebug(). 
> That would self-document the purpose.
> 

Well, it's not that it is early, it is that it actually can execute in
linear mode (paging off).

	-hpa