[Qemu-devel] [PATCH] Fix up pxe boot

[Qemu-devel] [PATCH] Fix up pxe boot

[Glauber Costa]

As discussed in
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00667.html,
current pxe boot is broken for some use cases. The problem
goes away if we reduce the number of allowed bits in the address space
to 32 (which has the side effect of reducing guest max mem size to 4Gb).

After digging for a while, it turns out that it happens because pxelinux
tries to access address 0x10009e9a6, which does not fit a 32-bit address.
A closer look, however, reveals this access is totally valid: It's just
0x9e9a6 with an add carry.

To avoid this, this patch casts the address passed to the POPL macro to
a 32-bit value. This is also done, although just theorectically, for
PUSHL too.

Signed-off-by: Glauber Costa <glommer@redhat.com>
Reported-by: Chris Lalancette <clalance@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/op_helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 0b5fdc0..433aa3f 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -600,7 +600,7 @@ do {\
 #define PUSHL(ssp, sp, sp_mask, val)\
 {\
     sp -= 4;\
-    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
+    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
 }
 
 #define POPW(ssp, sp, sp_mask, val)\
@@ -611,7 +611,7 @@ do {\
 
 #define POPL(ssp, sp, sp_mask, val)\
 {\
-    val = (uint32_t)ldl_kernel((ssp) + (sp & (sp_mask)));\
+    val = (uint32_t)ldl_kernel((uint32_t)((ssp) + (sp & (sp_mask))));\
     sp += 4;\
 }
 
-- 
1.5.5.1

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Avi Kivity]

Glauber Costa wrote:
> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
> index 0b5fdc0..433aa3f 100644
> --- a/target-i386/op_helper.c
> +++ b/target-i386/op_helper.c
> @@ -600,7 +600,7 @@ do {\
>  #define PUSHL(ssp, sp, sp_mask, val)\
>  {\
>      sp -= 4;\
> -    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
> +    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
>  }
>   

Surly it is better to push this into the underlying virtual->physical 
translation functions, so it applies everywhere?

btw, the cast is wrong for x86-64, so it must be qualified for 32-bit 
operating modes.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Glauber Costa]

On Tue, Sep 2, 2008 at 5:39 AM, Avi Kivity <avi@qumranet.com> wrote:
> Glauber Costa wrote:
>>
>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>> index 0b5fdc0..433aa3f 100644
>> --- a/target-i386/op_helper.c
>> +++ b/target-i386/op_helper.c
>> @@ -600,7 +600,7 @@ do {\
>>  #define PUSHL(ssp, sp, sp_mask, val)\
>>  {\
>>     sp -= 4;\
>> -    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
>> +    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
>>  }
>>
>
> Surly it is better to push this into the underlying virtual->physical
> translation functions, so it applies everywhere?
>
> btw, the cast is wrong for x86-64, so it must be qualified for 32-bit
> operating modes.
The tests were all done with x86_64. This is a PUSHL macro, so it's
32-bit anyway.
A x86_64-only PUSHQ seems to do the right thing.

>
> --
> error compiling committee.c: too many arguments to function
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Avi Kivity]

Glauber Costa wrote:
> On Tue, Sep 2, 2008 at 5:39 AM, Avi Kivity <avi@qumranet.com> wrote:
>   
>> Glauber Costa wrote:
>>     
>>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>>> index 0b5fdc0..433aa3f 100644
>>> --- a/target-i386/op_helper.c
>>> +++ b/target-i386/op_helper.c
>>> @@ -600,7 +600,7 @@ do {\
>>>  #define PUSHL(ssp, sp, sp_mask, val)\
>>>  {\
>>>     sp -= 4;\
>>> -    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
>>> +    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
>>>  }
>>>
>>>       
>> Surly it is better to push this into the underlying virtual->physical
>> translation functions, so it applies everywhere?
>>
>> btw, the cast is wrong for x86-64, so it must be qualified for 32-bit
>> operating modes.
>>     
> The tests were all done with x86_64. This is a PUSHL macro, so it's
> 32-bit anyway.
> A x86_64-only PUSHQ seems to do the right thing.
>
>   

Right.

It's still odd to see this in an op helper rather than in somewhere generic.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Glauber Costa]

On Tue, Sep 2, 2008 at 12:20 PM, Avi Kivity <avi@qumranet.com> wrote:
> Glauber Costa wrote:
>>
>> On Tue, Sep 2, 2008 at 5:39 AM, Avi Kivity <avi@qumranet.com> wrote:
>>
>>>
>>> Glauber Costa wrote:
>>>
>>>>
>>>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>>>> index 0b5fdc0..433aa3f 100644
>>>> --- a/target-i386/op_helper.c
>>>> +++ b/target-i386/op_helper.c
>>>> @@ -600,7 +600,7 @@ do {\
>>>>  #define PUSHL(ssp, sp, sp_mask, val)\
>>>>  {\
>>>>    sp -= 4;\
>>>> -    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
>>>> +    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
>>>>  }
>>>>
>>>>
>>>
>>> Surly it is better to push this into the underlying virtual->physical
>>> translation functions, so it applies everywhere?
>>>
>>> btw, the cast is wrong for x86-64, so it must be qualified for 32-bit
>>> operating modes.
>>>
>>
>> The tests were all done with x86_64. This is a PUSHL macro, so it's
>> 32-bit anyway.
>> A x86_64-only PUSHQ seems to do the right thing.
>>
>>
>
> Right.
>
> It's still odd to see this in an op helper rather than in somewhere generic.
>
I'll take a closer look today to see if I can improve it better.

> --
> error compiling committee.c: too many arguments to function
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Glauber Costa]

On Tue, Sep 02, 2008 at 06:20:55PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Tue, Sep 2, 2008 at 5:39 AM, Avi Kivity <avi@qumranet.com> wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>>>> index 0b5fdc0..433aa3f 100644
>>>> --- a/target-i386/op_helper.c
>>>> +++ b/target-i386/op_helper.c
>>>> @@ -600,7 +600,7 @@ do {\
>>>>  #define PUSHL(ssp, sp, sp_mask, val)\
>>>>  {\
>>>>     sp -= 4;\
>>>> -    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
>>>> +    stl_kernel((uint32_t)((ssp) + (sp & (sp_mask))), (uint32_t)(val));\
>>>>  }
>>>>
>>>>       
>>> Surly it is better to push this into the underlying virtual->physical
>>> translation functions, so it applies everywhere?
>>>
>>> btw, the cast is wrong for x86-64, so it must be qualified for 32-bit
>>> operating modes.
>>>     
>> The tests were all done with x86_64. This is a PUSHL macro, so it's
>> 32-bit anyway.
>> A x86_64-only PUSHQ seems to do the right thing.
>>
>>   
>
> Right.
>
> It's still odd to see this in an op helper rather than in somewhere generic.

After a second look, here's what it seems to me:

It's not in a generic place, such as ldl, because in general, we may want to grab
a 32-bit value from a 64-bit address. This is perfectly valid.

It's a specifity that the pop instruction, when not in long mode (manual says that in 64-bit mode
no 32-bit operand is valid, but then again, qemu should use the POPQ macro), that ssp:sp may overflow,
but we don't want it.

It would be possible to do something more generic if we had a segment_to_linear() function, that returned
the linear address, but we don't.

Does it make more sense to you?
>
> -- 
> error compiling committee.c: too many arguments to function
>

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Avi Kivity]

Glauber Costa wrote:
> After a second look, here's what it seems to me:
>
> It's not in a generic place, such as ldl, because in general, we may want to grab
> a 32-bit value from a 64-bit address. This is perfectly valid.
>
> It's a specifity that the pop instruction, when not in long mode (manual says that in 64-bit mode
> no 32-bit operand is valid, but then again, qemu should use the POPQ macro), that ssp:sp may overflow,
> but we don't want it.
>
> It would be possible to do something more generic if we had a segment_to_linear() function, that returned
> the linear address, but we don't.
>
> Does it make more sense to you?
>   

Yes.

I guess tcg code is mostly safe since it generates 32-bit additions for 
segment bases, so this is limited to the places you identified.  And a 
helper to add segment bases would be helpful.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Glauber Costa]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Sun, Sep 07, 2008 at 09:42:07AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> After a second look, here's what it seems to me:
>>
>> It's not in a generic place, such as ldl, because in general, we may want to grab
>> a 32-bit value from a 64-bit address. This is perfectly valid.
>>
>> It's a specifity that the pop instruction, when not in long mode (manual says that in 64-bit mode
>> no 32-bit operand is valid, but then again, qemu should use the POPQ macro), that ssp:sp may overflow,
>> but we don't want it.
>>
>> It would be possible to do something more generic if we had a segment_to_linear() function, that returned
>> the linear address, but we don't.
>>
>> Does it make more sense to you?
>>   
>
> Yes.
>
> I guess tcg code is mostly safe since it generates 32-bit additions for  
> segment bases, so this is limited to the places you identified.  And a  
> helper to add segment bases would be helpful.
>
> -- 
> error compiling committee.c: too many arguments to function
>

what do you think of the attached version?

[-- Attachment #2: 0001-Fix-up-pxe-boot.patch --]
[-- Type: text/plain, Size: 2166 bytes --]

>From e185d17904febce8b9fe0b0d403c0ee9df92ca38 Mon Sep 17 00:00:00 2001
From: Glauber Costa <glommer@redhat.com>
Date: Mon, 1 Sep 2008 17:49:23 -0300
Subject: [PATCH] Fix up pxe boot

As discussed in
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00667.html,
current pxe boot is broken for some use cases. The problem
goes away if we reduce the number of allowed bits in the address space
to 32 (which has the side effect of reducing guest max mem size to 4Gb).

After digging for a while, it turns out that it happens because pxelinux
tries to access address 0x10009e9a6, which does not fit a 32-bit address.
A closer look, however, reveals this access is totally valid: It's just
0x9e9a6 with an add carry.

To avoid this, this patch casts the address passed to the POPL macro to
a 32-bit value. This is also done, although just theorectically, for
PUSHL too.

Signed-off-by: Glauber Costa <glommer@redhat.com>
Reported-by: Chris Lalancette <clalance@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/op_helper.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 0b5fdc0..4c3ee06 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -590,6 +590,10 @@ do {\
 #define SET_ESP(val, sp_mask) ESP = (ESP & ~(sp_mask)) | ((val) & (sp_mask))
 #endif
 
+/* in 64-bit machines, this can overflow. So this segment addition macro
+ * can be used to trim the value to 32-bit whenever needed */
+#define SEG_ADDL(ssp, sp, sp_mask) ((uint32_t)((ssp) + (sp & (sp_mask))))
+
 /* XXX: add a is_user flag to have proper security support */
 #define PUSHW(ssp, sp, sp_mask, val)\
 {\
@@ -600,7 +604,7 @@ do {\
 #define PUSHL(ssp, sp, sp_mask, val)\
 {\
     sp -= 4;\
-    stl_kernel((ssp) + (sp & (sp_mask)), (val));\
+    stl_kernel(SEG_ADDL(ssp, sp, sp_mask), (uint32_t)(val));\
 }
 
 #define POPW(ssp, sp, sp_mask, val)\
@@ -611,7 +615,7 @@ do {\
 
 #define POPL(ssp, sp, sp_mask, val)\
 {\
-    val = (uint32_t)ldl_kernel((ssp) + (sp & (sp_mask)));\
+    val = (uint32_t)ldl_kernel(SEG_ADDL(ssp, sp, sp_mask));\
     sp += 4;\
 }
 
-- 
1.5.5.1

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Avi Kivity]

Glauber Costa wrote:
> On Sun, Sep 07, 2008 at 09:42:07AM +0300, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> After a second look, here's what it seems to me:
>>>
>>> It's not in a generic place, such as ldl, because in general, we may want to grab
>>> a 32-bit value from a 64-bit address. This is perfectly valid.
>>>
>>> It's a specifity that the pop instruction, when not in long mode (manual says that in 64-bit mode
>>> no 32-bit operand is valid, but then again, qemu should use the POPQ macro), that ssp:sp may overflow,
>>> but we don't want it.
>>>
>>> It would be possible to do something more generic if we had a segment_to_linear() function, that returned
>>> the linear address, but we don't.
>>>
>>> Does it make more sense to you?
>>>   
>>>       
>> Yes.
>>
>> I guess tcg code is mostly safe since it generates 32-bit additions for  
>> segment bases, so this is limited to the places you identified.  And a  
>> helper to add segment bases would be helpful.
>>
>> -- 
>> error compiling committee.c: too many arguments to function
>>
>>     
>
> what do you think of the attached version?
>   

Looks fine to me.  One can thing of a cleverer helper (that receives the 
segment number and adds its base to the offset), but there's not reason 
to do this all at one.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] Fix up pxe boot

[Anthony Liguori]

Glauber Costa wrote:
> On Sun, Sep 07, 2008 at 09:42:07AM +0300, Avi Kivity wrote:
>   
> what do you think of the attached version?
>   
Applied.  Thanks.

Regards,

Anthony Liguori