[PATCH] x86: use int instead of long to set reset vector back to 0

[PATCH] x86: use int instead of long to set reset vector back to 0

[Don Zickus]

A customer of ours, complained that when setting the reset vector
back to 0, it trashed other data and hung their box.  They noticed
when only 4 bytes were set to 0 instead of 8, everything worked
correctly.

I don't know where to find this in the spec to know if this is
correct, but looking at historical info of the code, x86_64 used
to use 'int' here whereas i386 used to use 'long'.  When they got
merged it seemed like 'long' was used.  So the change seems to
make sense from that point of view.

I am hoping someone smarter than me can verify this.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/include/asm/smpboot_hooks.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/smpboot_hooks.h b/arch/x86/include/asm/smpboot_hooks.h
index 6c22bf3..42d0366 100644
--- a/arch/x86/include/asm/smpboot_hooks.h
+++ b/arch/x86/include/asm/smpboot_hooks.h
@@ -34,7 +34,7 @@ static inline void smpboot_restore_warm_reset_vector(void)
 	 */
 	CMOS_WRITE(0, 0xf);
 
-	*((volatile long *)phys_to_virt(apic->trampoline_phys_low)) = 0;
+	*((volatile int *)phys_to_virt(apic->trampoline_phys_low)) = 0;
 }
 
 static inline void __init smpboot_setup_io_apic(void)
-- 
1.7.3.5

Re: [PATCH] x86: use int instead of long to set reset vector back to 0

[Matthew Garrett]

On Mon, Feb 07, 2011 at 11:25:00PM -0500, Don Zickus wrote:
> A customer of ours, complained that when setting the reset vector
> back to 0, it trashed other data and hung their box.  They noticed
> when only 4 bytes were set to 0 instead of 8, everything worked
> correctly.

We're supposed to be resetting trampoline_phys_low and 
trampoline_phys_high here, which are two 16-bit values. Writing 64 bits 
is definitely going to overwrite space that we're not supposed to be 
touching.

> -	*((volatile long *)phys_to_virt(apic->trampoline_phys_low)) = 0;
> +	*((volatile int *)phys_to_virt(apic->trampoline_phys_low)) = 0;

I'd suggest either using u32 here, or alternatively make it more obvious 
what's going on and set trampoline_phys_low and trampoline_phys_high 
(which are both 16 bit) independently.

Ingo? Looks like you touched this last, but it seems that the bug was 
there already.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

[tip:x86/urgent] x86: Use u32 instead of long to set reset vector back to 0

[tip-bot for Don Zickus]

Commit-ID:  299c56966a72b9109d47c71a6db52097098703dd
Gitweb:     http://git.kernel.org/tip/299c56966a72b9109d47c71a6db52097098703dd
Author:     Don Zickus <dzickus@redhat.com>
AuthorDate: Mon, 7 Feb 2011 23:25:00 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 28 Feb 2011 16:22:18 +0100

x86: Use u32 instead of long to set reset vector back to 0

A customer of ours, complained that when setting the reset
vector back to 0, it trashed other data and hung their box.
They noticed when only 4 bytes were set to 0 instead of 8,
everything worked correctly.

Mathew pointed out:

 |
 | We're supposed to be resetting trampoline_phys_low and
 | trampoline_phys_high here, which are two 16-bit values.
 | Writing 64 bits is definitely going to overwrite space
 | that we're not supposed to be touching.
 |

So limit the area modified to u32.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Acked-by: Matthew Garrett <mjg@redhat.com>
Cc: <stable@kernel.org>
LKML-Reference: <1297139100-424-1-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/smpboot_hooks.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/smpboot_hooks.h b/arch/x86/include/asm/smpboot_hooks.h
index 6c22bf3..725b778 100644
--- a/arch/x86/include/asm/smpboot_hooks.h
+++ b/arch/x86/include/asm/smpboot_hooks.h
@@ -34,7 +34,7 @@ static inline void smpboot_restore_warm_reset_vector(void)
 	 */
 	CMOS_WRITE(0, 0xf);
 
-	*((volatile long *)phys_to_virt(apic->trampoline_phys_low)) = 0;
+	*((volatile u32 *)phys_to_virt(apic->trampoline_phys_low)) = 0;
 }
 
 static inline void __init smpboot_setup_io_apic(void)

[PATCH] x86: use int instead of long to set reset vector back to 0

[Don Zickus]

A customer of ours, complained that when setting the reset vector
back to 0, it trashed other data and hung their box.  They noticed
when only 4 bytes were set to 0 instead of 8, everything worked
correctly.

I don't know where to find this in the spec to know if this is
correct, but looking at historical info of the code, x86_64 used
to use 'int' here whereas i386 used to use 'long'.  When they got
merged it seemed like 'long' was used.  So the change seems to
make sense from that point of view.

I am hoping someone smarter than me can verify this.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
Reposting this in case it was lost in the noise

---
 arch/x86/include/asm/smpboot_hooks.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/smpboot_hooks.h b/arch/x86/include/asm/smpboot_hooks.h
index 6c22bf3..42d0366 100644
--- a/arch/x86/include/asm/smpboot_hooks.h
+++ b/arch/x86/include/asm/smpboot_hooks.h
@@ -34,7 +34,7 @@ static inline void smpboot_restore_warm_reset_vector(void)
 	 */
 	CMOS_WRITE(0, 0xf);
 
-	*((volatile long *)phys_to_virt(apic->trampoline_phys_low)) = 0;
+	*((volatile int *)phys_to_virt(apic->trampoline_phys_low)) = 0;
 }
 
 static inline void __init smpboot_setup_io_apic(void)
-- 
1.7.3.5