Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction
The shld instruction does an arithmetic shift left on 64bit operants, but it's not the instruction we want, because what it actually does is shifting by cl, and storing the result in address 32.
This wasn't notices as the DRAM is up and address 32 is valid. On real hardware when CAR is running this instruction would cause a crash.
Replace the instruction with the correct 64bit arithmetic left shift.
Change-Id: Iedad9f4b693b1ea05898456eac2050a9389f6f19 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/qemu-x86/cache_as_ram_bootblock.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/45820/1
diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index 415ed24..eb7d2d9 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -30,7 +30,7 @@ /* Restore the BIST result and timestamps. */ #if defined(__x86_64__) movd %mm2, %rdi - shld %rdi, 32 + shlq $32, %rdi movd %mm1, %rsi or %rsi, %rdi
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG@13 PS1, Line 13: notices noticed with QEMU
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG@14 PS1, Line 14: would cause causes
Hello build bot (Jenkins), Patrick Georgi, Christian Walter, Paul Menzel, Angel Pons, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45820
to look at the new patch set (#2).
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction
The shld instruction does an arithmetic shift left on 64bit operants, but it's not the instruction we want, because what it actually does is shifting by cl, and storing the result in address 32.
This wasn't noticed with QEMU as the DRAM is up and address 32 is valid. On real hardware when CAR is running this instruction causes a crash.
Replace the instruction with the correct 64bit arithmetic left shift.
Change-Id: Iedad9f4b693b1ea05898456eac2050a9389f6f19 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/qemu-x86/cache_as_ram_bootblock.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/45820/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG@13 PS1, Line 13: notices
noticed with QEMU
Done
https://review.coreboot.org/c/coreboot/+/45820/1//COMMIT_MSG@14 PS1, Line 14: would cause
causes
Done
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
Patch Set 2: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45820/2/src/cpu/qemu-x86/cache_as_r... File src/cpu/qemu-x86/cache_as_ram_bootblock.S:
https://review.coreboot.org/c/coreboot/+/45820/2/src/cpu/qemu-x86/cache_as_r... PS2, Line 32: movd I quess the `d` in `movd` didn't help when trying to find this bug 😜
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45820 )
Change subject: cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction ......................................................................
cpu/qemu-x86/cache_as_ram_bootblock: Fix wrong instruction
The shld instruction does an arithmetic shift left on 64bit operants, but it's not the instruction we want, because what it actually does is shifting by cl, and storing the result in address 32.
This wasn't noticed with QEMU as the DRAM is up and address 32 is valid. On real hardware when CAR is running this instruction causes a crash.
Replace the instruction with the correct 64bit arithmetic left shift.
Change-Id: Iedad9f4b693b1ea05898456eac2050a9389f6f19 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45820 Reviewed-by: Christian Walter christian.walter@9elements.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/qemu-x86/cache_as_ram_bootblock.S 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved Christian Walter: Looks good to me, approved
diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index 148948b..197e0fd 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -33,7 +33,7 @@ /* Restore the BIST result and timestamps. */ #if defined(__x86_64__) movd %mm2, %rdi - shld %rdi, 32 + shlq $32, %rdi movd %mm1, %rsi or %rsi, %rdi
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)