diff mbox

[3/8] sparc64: fix 32bit load sign extension

Message ID 20100601201232.5908.51923.stgit@skyserv
State New
Headers show

Commit Message

Igor V. Kovalenko June 1, 2010, 8:12 p.m. UTC
From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- change return type of ldl_* to uint32_t to prevent unwanted sign extension
  visible in sparc64 load alternate address space methods
- note this change makes ldl_* softmmu implementations match ldl_phys one
Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 softmmu_header.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Paolo Bonzini June 3, 2010, 1:18 p.m. UTC | #1
On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>
> - change return type of ldl_* to uint32_t to prevent unwanted sign extension
>    visible in sparc64 load alternate address space methods
> - note this change makes ldl_* softmmu implementations match ldl_phys one

This patch breaks -kernel/-initrd.

Paolo
Alexander Graf June 3, 2010, 3:25 p.m. UTC | #2
Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>
>> - change return type of ldl_* to uint32_t to prevent unwanted sign  
>> extension
>>   visible in sparc64 load alternate address space methods
>> - note this change makes ldl_* softmmu implementations match  
>> ldl_phys one
>
> This patch breaks -kernel/-initrd.

Breaks it where and when?

Alex
>
Paolo Bonzini June 3, 2010, 3:42 p.m. UTC | #3
On 06/03/2010 05:25 PM, Alexander Graf wrote:
>
> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>
>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>
>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>> extension
>>> visible in sparc64 load alternate address space methods
>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>> one
>>
>> This patch breaks -kernel/-initrd.
>
> Breaks it where and when?

x86_64 TCG reboots after the "Probing EDD" step.

Paolo
Igor V. Kovalenko June 3, 2010, 7:59 p.m. UTC | #4
On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>
>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>
>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>
>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>
>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>> extension
>>>> visible in sparc64 load alternate address space methods
>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>> one
>>>
>>> This patch breaks -kernel/-initrd.
>>
>> Breaks it where and when?
>
> x86_64 TCG reboots after the "Probing EDD" step.

My local build appears to work, qemu-system-x86_64 loads my gentoo linux setup.
I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
--prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu
Paolo Bonzini June 4, 2010, 7:53 a.m. UTC | #5
On 06/03/2010 09:59 PM, Igor Kovalenko wrote:
> On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>>
>>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini<pbonzini@redhat.com>:
>>>
>>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>>
>>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>>
>>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>>> extension
>>>>> visible in sparc64 load alternate address space methods
>>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>>> one
>>>>
>>>> This patch breaks -kernel/-initrd.
>>>
>>> Breaks it where and when?
>>
>> x86_64 TCG reboots after the "Probing EDD" step.
>
> My local build appears to work, qemu-system-x86_64 loads my gentoo linux setup.
> I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
> --prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu

Normal boot works.  Only -kernel/-initrd fails.

Paolo
Paolo Bonzini June 4, 2010, 10:18 a.m. UTC | #6
On 06/04/2010 09:53 AM, Paolo Bonzini wrote:
> On 06/03/2010 09:59 PM, Igor Kovalenko wrote:
>> On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini<pbonzini@redhat.com> wrote:
>>> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>>>
>>>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini<pbonzini@redhat.com>:
>>>>
>>>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>>>
>>>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>>>
>>>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>>>> extension
>>>>>> visible in sparc64 load alternate address space methods
>>>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>>>> one
>>>>>
>>>>> This patch breaks -kernel/-initrd.
>>>>
>>>> Breaks it where and when?
>>>
>>> x86_64 TCG reboots after the "Probing EDD" step.
>>
>> My local build appears to work, qemu-system-x86_64 loads my gentoo
>> linux setup.
>> I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
>> --prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu
>
> Normal boot works. Only -kernel/-initrd fails.

Hmm, PEBKAC.  Boot of Fedora and RHEL5 guests always fails, so it's not 
related to -kernel/-initrd.  (Of course, without -kernel/-initrd it 
reboots into GRUB rather than looping quickly).

I've placed a failing vmlinuz at 
http://people.redhat.com/people/vmlinuz-fail -- if it fails it should 
reboot continuously.  The failure happens pretty soon after the kernel 
starts running.  The sequence is:

   lock_kernel
   -> __lock_kernel
   -> preempt_disable
   -> current_thread_info()

   IN:
   0xffffffff80063064:  push   %rbp
   0xffffffff80063065:  mov    %rsp,%rbp
   0xffffffff80063068:  mov    %gs:0x10,%rax
   0xffffffff80063071:  mov    -0x1fc8(%rax),%eax
   0xffffffff80063077:  test   $0x8,%al
   0xffffffff80063079:  je     0xffffffff800630a2

%rax is 0xffffffff803f1fd8, but it page faults with 
%cr2=0x00000000803f0010.  The reason is that in the generated x86 
assembly -0x1fc8 is erroneously zero extended:

0x4180347b:  mov    %rbp,%rbx
0x4180347e:  mov    $0xffffe038,%r12d
0x41803484:  add    %r12,%rbx

so it gives the wrong address:

(gdb) info reg rbp
rbp            0xffffffff803f1fd8	0xffffffff803f1fd8
(gdb) info reg r12
r12            0xffffe038	4294959160
(gdb) info reg rbx
rbx            0x803f0010	2151612432

 From there it's obvious: general protection, double fault, general 
protection, triple fault.

So it's a TCG bug that is expecting ldl_* to sign extend.  I'll send a 
patch after I come back from lunch.

Paolo
diff mbox

Patch

diff --git a/softmmu_header.h b/softmmu_header.h
index 6a36e01..2f95c33 100644
--- a/softmmu_header.h
+++ b/softmmu_header.h
@@ -60,7 +60,7 @@ 
 #if DATA_SIZE == 8
 #define RES_TYPE uint64_t
 #else
-#define RES_TYPE int
+#define RES_TYPE uint32_t
 #endif
 
 #if ACCESS_TYPE == (NB_MMU_MODES + 1)