* [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
@ 2012-09-12 18:03 Stefan Weil
2012-09-12 18:12 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Weil @ 2012-09-12 18:03 UTC (permalink / raw)
To: Blue Swirl; +Cc: Stefan Weil, qemu-devel, qemu-stable
TCG uses 6 registers for function arguments on 64 bit Linux hosts,
but only 4 registers on W64 hosts.
Commit 2999a0b20074a7e4a58f56572bb1436749368f59 increased the number
of arguments for some important helper functions from 4 to 5
which triggered a bug for W64 hosts: QEMU aborts when executing
helper_lcall_real in the guest's BIOS because function
tcg_target_get_call_iarg_regs_count always returned 6.
As W64 has only 4 registers for arguments, the 5th argument must be
passed on the stack using a correct stack offset.
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
Without that patch, QEMU 1.2 is unusable on W64 hosts.
Please apply it to the stable versions.
Thanks,
Stefan W.
tcg/i386/tcg-target.c | 2 +-
tcg/i386/tcg-target.h | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
index da17bba..43b5572 100644
--- a/tcg/i386/tcg-target.c
+++ b/tcg/i386/tcg-target.c
@@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
static inline int tcg_target_get_call_iarg_regs_count(int flags)
{
if (TCG_TARGET_REG_BITS == 64) {
- return 6;
+ return ARRAY_SIZE(tcg_target_call_iarg_regs);
}
return 0;
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index c3cfe05..87417d0 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -67,7 +67,11 @@ typedef enum {
/* used for function call generation */
#define TCG_REG_CALL_STACK TCG_REG_ESP
#define TCG_TARGET_STACK_ALIGN 16
+#if defined(_WIN64)
+#define TCG_TARGET_CALL_STACK_OFFSET 32
+#else
#define TCG_TARGET_CALL_STACK_OFFSET 0
+#endif
/* optional instructions */
#define TCG_TARGET_HAS_div2_i32 1
--
1.7.10
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
2012-09-12 18:03 [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments Stefan Weil
@ 2012-09-12 18:12 ` Peter Maydell
2012-09-12 19:14 ` Aurelien Jarno
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2012-09-12 18:12 UTC (permalink / raw)
To: Stefan Weil; +Cc: Blue Swirl, qemu-devel, qemu-stable
On 12 September 2012 19:03, Stefan Weil <sw@weilnetz.de> wrote:
> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
> index da17bba..43b5572 100644
> --- a/tcg/i386/tcg-target.c
> +++ b/tcg/i386/tcg-target.c
> @@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
> static inline int tcg_target_get_call_iarg_regs_count(int flags)
> {
> if (TCG_TARGET_REG_BITS == 64) {
> - return 6;
> + return ARRAY_SIZE(tcg_target_call_iarg_regs);
> }
>
> return 0;
Hmm. Why can't we just return the array size in all cases?
Is there something special about 32 bit x86? I checked, and
all our other TCG targets return the same value as the size of
the iarg_regs array (either using ARRAY_SIZE or by just returning
the right number)...
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
2012-09-12 18:12 ` Peter Maydell
@ 2012-09-12 19:14 ` Aurelien Jarno
2012-09-12 19:39 ` Stefan Weil
2012-09-12 20:01 ` Stefan Weil
0 siblings, 2 replies; 6+ messages in thread
From: Aurelien Jarno @ 2012-09-12 19:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: Blue Swirl, Stefan Weil, qemu-devel, qemu-stable
On Wed, Sep 12, 2012 at 07:12:47PM +0100, Peter Maydell wrote:
> On 12 September 2012 19:03, Stefan Weil <sw@weilnetz.de> wrote:
> > diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
> > index da17bba..43b5572 100644
> > --- a/tcg/i386/tcg-target.c
> > +++ b/tcg/i386/tcg-target.c
> > @@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
> > static inline int tcg_target_get_call_iarg_regs_count(int flags)
> > {
> > if (TCG_TARGET_REG_BITS == 64) {
> > - return 6;
> > + return ARRAY_SIZE(tcg_target_call_iarg_regs);
> > }
> >
> > return 0;
>
> Hmm. Why can't we just return the array size in all cases?
> Is there something special about 32 bit x86? I checked, and
> all our other TCG targets return the same value as the size of
> the iarg_regs array (either using ARRAY_SIZE or by just returning
> the right number)...
>
On 32-bit x86, all arguments are now being passed on the stack, that's
why the function returns 0. On the other hand when the change has been
done, the registers haven't been removed from tcg_target_call_iarg_regs.
I think this patch is fine enough for 1.2, but a better patch is needed
for master.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
2012-09-12 19:14 ` Aurelien Jarno
@ 2012-09-12 19:39 ` Stefan Weil
2012-09-14 17:44 ` Blue Swirl
2012-09-12 20:01 ` Stefan Weil
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Weil @ 2012-09-12 19:39 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: Blue Swirl, Peter Maydell, qemu-devel, qemu-stable
Am 12.09.2012 21:14, schrieb Aurelien Jarno:
> On Wed, Sep 12, 2012 at 07:12:47PM +0100, Peter Maydell wrote:
>> On 12 September 2012 19:03, Stefan Weil<sw@weilnetz.de> wrote:
>>> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
>>> index da17bba..43b5572 100644
>>> --- a/tcg/i386/tcg-target.c
>>> +++ b/tcg/i386/tcg-target.c
>>> @@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
>>> static inline int tcg_target_get_call_iarg_regs_count(int flags)
>>> {
>>> if (TCG_TARGET_REG_BITS == 64) {
>>> - return 6;
>>> + return ARRAY_SIZE(tcg_target_call_iarg_regs);
>>> }
>>>
>>> return 0;
>>
>> Hmm. Why can't we just return the array size in all cases?
>> Is there something special about 32 bit x86? I checked, and
>> all our other TCG targets return the same value as the size of
>> the iarg_regs array (either using ARRAY_SIZE or by just returning
>> the right number)...
>>
>
> On 32-bit x86, all arguments are now being passed on the stack, that's
> why the function returns 0. On the other hand when the change has been
> done, the registers haven't been removed from tcg_target_call_iarg_regs.
>
> I think this patch is fine enough for 1.2, but a better patch is needed
> for master.
I noticed that Blue switched from register arguments to
arguments on the stack, but don't know the reason for that
change.
Maybe 32 bit x86 can also use a mixture of register / stack
arguments. This needs more testing and is the main reason
why I did not change tcg_target_call_iarg_regs for 32 bit
and return ARRAY_SIZE for both 32 and 64 bit.
I'd prefer to get the patch in master soon because it is
a minimalistic change which fixes the now unusable
64 bit mode on Windows. An additional patch can still
be applied on top.
Of course any better patch which also fixes 64 bit Windows
and which comes soon would also be very acceptable.
Regards
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
2012-09-12 19:39 ` Stefan Weil
@ 2012-09-14 17:44 ` Blue Swirl
0 siblings, 0 replies; 6+ messages in thread
From: Blue Swirl @ 2012-09-14 17:44 UTC (permalink / raw)
To: Stefan Weil; +Cc: Peter Maydell, qemu-devel, Aurelien Jarno, qemu-stable
On Wed, Sep 12, 2012 at 7:39 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 12.09.2012 21:14, schrieb Aurelien Jarno:
>
>> On Wed, Sep 12, 2012 at 07:12:47PM +0100, Peter Maydell wrote:
>>>
>>> On 12 September 2012 19:03, Stefan Weil<sw@weilnetz.de> wrote:
>>>>
>>>> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
>>>> index da17bba..43b5572 100644
>>>> --- a/tcg/i386/tcg-target.c
>>>> +++ b/tcg/i386/tcg-target.c
>>>> @@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
>>>> static inline int tcg_target_get_call_iarg_regs_count(int flags)
>>>> {
>>>> if (TCG_TARGET_REG_BITS == 64) {
>>>> - return 6;
>>>> + return ARRAY_SIZE(tcg_target_call_iarg_regs);
>>>> }
>>>>
>>>> return 0;
>>>
>>>
>>> Hmm. Why can't we just return the array size in all cases?
>>> Is there something special about 32 bit x86? I checked, and
>>> all our other TCG targets return the same value as the size of
>>> the iarg_regs array (either using ARRAY_SIZE or by just returning
>>> the right number)...
>>>
>>
>> On 32-bit x86, all arguments are now being passed on the stack, that's
>> why the function returns 0. On the other hand when the change has been
>> done, the registers haven't been removed from tcg_target_call_iarg_regs.
>>
>> I think this patch is fine enough for 1.2, but a better patch is needed
>> for master.
>
>
> I noticed that Blue switched from register arguments to
> arguments on the stack, but don't know the reason for that
> change.
When doing the AREG0 conversion, it was already difficult to get one
way of passing arguments to work, never mind supporting both register
arguments in addition to stack.
>
> Maybe 32 bit x86 can also use a mixture of register / stack
> arguments. This needs more testing and is the main reason
> why I did not change tcg_target_call_iarg_regs for 32 bit
> and return ARRAY_SIZE for both 32 and 64 bit.
After AREG0 conversion has been finished, it should be possible to
restore register argument passing, if this gives better performance.
All files in QEMU should be compiled with the same flags then, there
should be no need to introduce any REGPARM attributes anymore.
>
> I'd prefer to get the patch in master soon because it is
> a minimalistic change which fixes the now unusable
> 64 bit mode on Windows. An additional patch can still
> be applied on top.
>
> Of course any better patch which also fixes 64 bit Windows
> and which comes soon would also be very acceptable.
>
> Regards
>
> Stefan
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments
2012-09-12 19:14 ` Aurelien Jarno
2012-09-12 19:39 ` Stefan Weil
@ 2012-09-12 20:01 ` Stefan Weil
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Weil @ 2012-09-12 20:01 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: Blue Swirl, Peter Maydell, qemu-devel
Am 12.09.2012 21:14, schrieb Aurelien Jarno:
> On Wed, Sep 12, 2012 at 07:12:47PM +0100, Peter Maydell wrote:
>> On 12 September 2012 19:03, Stefan Weil<sw@weilnetz.de> wrote:
>>> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
>>> index da17bba..43b5572 100644
>>> --- a/tcg/i386/tcg-target.c
>>> +++ b/tcg/i386/tcg-target.c
>>> @@ -118,7 +118,7 @@ static void patch_reloc(uint8_t *code_ptr, int type,
>>> static inline int tcg_target_get_call_iarg_regs_count(int flags)
>>> {
>>> if (TCG_TARGET_REG_BITS == 64) {
>>> - return 6;
>>> + return ARRAY_SIZE(tcg_target_call_iarg_regs);
>>> }
>>>
>>> return 0;
>>
>> Hmm. Why can't we just return the array size in all cases?
>> Is there something special about 32 bit x86? I checked, and
>> all our other TCG targets return the same value as the size of
>> the iarg_regs array (either using ARRAY_SIZE or by just returning
>> the right number)...
>>
>
> On 32-bit x86, all arguments are now being passed on the stack, that's
> why the function returns 0. On the other hand when the change has been
> done, the registers haven't been removed from tcg_target_call_iarg_regs.
>
> I think this patch is fine enough for 1.2, but a better patch is needed
> for master.
As soon as the special case x86 with 32 bit is fixed or eliminated,
it should be possible that all TCG targets share the same code for
tcg_target_get_call_iarg_regs_count. That function could be
removed from the target specific implementations and moved
to tcg.c.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-14 17:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12 18:03 [Qemu-devel] [PATCH] w64: Fix calls of TCG helper functions with 5 arguments Stefan Weil
2012-09-12 18:12 ` Peter Maydell
2012-09-12 19:14 ` Aurelien Jarno
2012-09-12 19:39 ` Stefan Weil
2012-09-14 17:44 ` Blue Swirl
2012-09-12 20:01 ` Stefan Weil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).