[PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor}

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor}
@ 2021-02-26 16:20 Richard Henderson
  2021-02-26 16:20 ` [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt Richard Henderson
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Richard Henderson @ 2021-02-26 16:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, cfontana, ehabkost

As discussed during review of Claudio's "i386 cleanup" patch set.


r~


Richard Henderson (2):
  target/i386: Rename helper_fldt, helper_fstt
  target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor

 target/i386/tcg/fpu_helper.c | 65 +++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 24 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt
  2021-02-26 16:20 [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Richard Henderson
@ 2021-02-26 16:20 ` Richard Henderson
  2021-02-26 16:45   ` Philippe Mathieu-Daudé
  2021-02-26 16:20 ` [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor Richard Henderson
  2021-02-26 16:59 ` [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Claudio Fontana
  2 siblings, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2021-02-26 16:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, cfontana, ehabkost

Change the prefix from "helper" to "do".  The former should be
reserved for those functions that are called from TCG; the latter
is in use within the file already for those functions that are
called from the helper functions, adding a "retaddr" argument.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/fpu_helper.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index 60ed93520a..3d9b192901 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -117,8 +117,7 @@ static inline void fpop(CPUX86State *env)
     env->fpstt = (env->fpstt + 1) & 7;
 }
 
-static inline floatx80 helper_fldt(CPUX86State *env, target_ulong ptr,
-                                   uintptr_t retaddr)
+static floatx80 do_fldt(CPUX86State *env, target_ulong ptr, uintptr_t retaddr)
 {
     CPU_LDoubleU temp;
 
@@ -127,8 +126,8 @@ static inline floatx80 helper_fldt(CPUX86State *env, target_ulong ptr,
     return temp.d;
 }
 
-static inline void helper_fstt(CPUX86State *env, floatx80 f, target_ulong ptr,
-                               uintptr_t retaddr)
+static void do_fstt(CPUX86State *env, floatx80 f, target_ulong ptr,
+                    uintptr_t retaddr)
 {
     CPU_LDoubleU temp;
 
@@ -405,14 +404,14 @@ void helper_fldt_ST0(CPUX86State *env, target_ulong ptr)
     int new_fpstt;
 
     new_fpstt = (env->fpstt - 1) & 7;
-    env->fpregs[new_fpstt].d = helper_fldt(env, ptr, GETPC());
+    env->fpregs[new_fpstt].d = do_fldt(env, ptr, GETPC());
     env->fpstt = new_fpstt;
     env->fptags[new_fpstt] = 0; /* validate stack entry */
 }
 
 void helper_fstt_ST0(CPUX86State *env, target_ulong ptr)
 {
-    helper_fstt(env, ST0, ptr, GETPC());
+    do_fstt(env, ST0, ptr, GETPC());
 }
 
 void helper_fpush(CPUX86State *env)
@@ -2468,7 +2467,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
     ptr += (14 << data32);
     for (i = 0; i < 8; i++) {
         tmp = ST(i);
-        helper_fstt(env, tmp, ptr, GETPC());
+        do_fstt(env, tmp, ptr, GETPC());
         ptr += 10;
     }
 
@@ -2495,7 +2494,7 @@ void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
     ptr += (14 << data32);
 
     for (i = 0; i < 8; i++) {
-        tmp = helper_fldt(env, ptr, GETPC());
+        tmp = do_fldt(env, ptr, GETPC());
         ST(i) = tmp;
         ptr += 10;
     }
@@ -2539,7 +2538,7 @@ static void do_xsave_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     addr = ptr + XO(legacy.fpregs);
     for (i = 0; i < 8; i++) {
         floatx80 tmp = ST(i);
-        helper_fstt(env, tmp, addr, ra);
+        do_fstt(env, tmp, addr, ra);
         addr += 16;
     }
 }
@@ -2703,7 +2702,7 @@ static void do_xrstor_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 
     addr = ptr + XO(legacy.fpregs);
     for (i = 0; i < 8; i++) {
-        floatx80 tmp = helper_fldt(env, addr, ra);
+        floatx80 tmp = do_fldt(env, addr, ra);
         ST(i) = tmp;
         addr += 16;
     }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt
  2021-02-26 16:20 ` [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt Richard Henderson
@ 2021-02-26 16:45   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-26 16:45 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: pbonzini, cfontana, ehabkost

On 2/26/21 5:20 PM, Richard Henderson wrote:
> Change the prefix from "helper" to "do".  The former should be
> reserved for those functions that are called from TCG; the latter
> is in use within the file already for those functions that are
> called from the helper functions, adding a "retaddr" argument.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/tcg/fpu_helper.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
  2021-02-26 16:20 [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Richard Henderson
  2021-02-26 16:20 ` [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt Richard Henderson
@ 2021-02-26 16:20 ` Richard Henderson
  2021-02-26 16:47   ` Philippe Mathieu-Daudé
  2021-02-26 16:59 ` [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Claudio Fontana
  2 siblings, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2021-02-26 16:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, philmd, cfontana, ehabkost

The helper_* functions must use GETPC() to unwind from TCG.
The cpu_x86_* functions cannot, and directly calling the
helper_* functions is a bug.  Split out new functions that
perform the work and can be used by both.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/fpu_helper.c | 50 ++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index 3d9b192901..20e4d2e715 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -2457,17 +2457,18 @@ void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32)
     do_fldenv(env, ptr, data32, GETPC());
 }
 
-void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
+static void do_fsave(CPUX86State *env, target_ulong ptr, int data32,
+                     uintptr_t retaddr)
 {
     floatx80 tmp;
     int i;
 
-    do_fstenv(env, ptr, data32, GETPC());
+    do_fstenv(env, ptr, data32, retaddr);
 
     ptr += (14 << data32);
     for (i = 0; i < 8; i++) {
         tmp = ST(i);
-        do_fstt(env, tmp, ptr, GETPC());
+        do_fstt(env, tmp, ptr, retaddr);
         ptr += 10;
     }
 
@@ -2485,30 +2486,41 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
     env->fptags[7] = 1;
 }
 
-void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
+void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
+{
+    do_fsave(env, ptr, data32, GETPC());
+}
+
+static void do_frstor(CPUX86State *env, target_ulong ptr, int data32,
+                      uintptr_t retaddr)
 {
     floatx80 tmp;
     int i;
 
-    do_fldenv(env, ptr, data32, GETPC());
+    do_fldenv(env, ptr, data32, retaddr);
     ptr += (14 << data32);
 
     for (i = 0; i < 8; i++) {
-        tmp = do_fldt(env, ptr, GETPC());
+        tmp = do_fldt(env, ptr, retaddr);
         ST(i) = tmp;
         ptr += 10;
     }
 }
 
+void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
+{
+    do_frstor(env, ptr, data32, GETPC());
+}
+
 #if defined(CONFIG_USER_ONLY)
 void cpu_x86_fsave(CPUX86State *env, target_ulong ptr, int data32)
 {
-    helper_fsave(env, ptr, data32);
+    do_fsave(env, ptr, data32, 0);
 }
 
 void cpu_x86_frstor(CPUX86State *env, target_ulong ptr, int data32)
 {
-    helper_frstor(env, ptr, data32);
+    do_frstor(env, ptr, data32, 0);
 }
 #endif
 
@@ -2593,10 +2605,8 @@ static void do_xsave_pkru(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     cpu_stq_data_ra(env, ptr, env->pkru, ra);
 }
 
-void helper_fxsave(CPUX86State *env, target_ulong ptr)
+static void do_fxsave(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
-
     /* The operand must be 16 byte aligned */
     if (ptr & 0xf) {
         raise_exception_ra(env, EXCP0D_GPF, ra);
@@ -2615,6 +2625,11 @@ void helper_fxsave(CPUX86State *env, target_ulong ptr)
     }
 }
 
+void helper_fxsave(CPUX86State *env, target_ulong ptr)
+{
+    do_fxsave(env, ptr, GETPC());
+}
+
 static uint64_t get_xinuse(CPUX86State *env)
 {
     uint64_t inuse = -1;
@@ -2757,10 +2772,8 @@ static void do_xrstor_pkru(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     env->pkru = cpu_ldq_data_ra(env, ptr, ra);
 }
 
-void helper_fxrstor(CPUX86State *env, target_ulong ptr)
+static void do_fxrstor(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
-
     /* The operand must be 16 byte aligned */
     if (ptr & 0xf) {
         raise_exception_ra(env, EXCP0D_GPF, ra);
@@ -2779,15 +2792,20 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr)
     }
 }
 
+void helper_fxrstor(CPUX86State *env, target_ulong ptr)
+{
+    do_fxrstor(env, ptr, GETPC());
+}
+
 #if defined(CONFIG_USER_ONLY)
 void cpu_x86_fxsave(CPUX86State *env, target_ulong ptr)
 {
-    helper_fxsave(env, ptr);
+    do_fxsave(env, ptr, 0);
 }
 
 void cpu_x86_fxrstor(CPUX86State *env, target_ulong ptr)
 {
-    helper_fxrstor(env, ptr);
+    do_fxrstor(env, ptr, 0);
 }
 #endif
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
  2021-02-26 16:20 ` [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor Richard Henderson
@ 2021-02-26 16:47   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-26 16:47 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: pbonzini, cfontana, ehabkost

On 2/26/21 5:20 PM, Richard Henderson wrote:
> The helper_* functions must use GETPC() to unwind from TCG.
> The cpu_x86_* functions cannot, and directly calling the
> helper_* functions is a bug.  Split out new functions that
> perform the work and can be used by both.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/tcg/fpu_helper.c | 50 ++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor}
  2021-02-26 16:20 [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Richard Henderson
  2021-02-26 16:20 ` [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt Richard Henderson
  2021-02-26 16:20 ` [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor Richard Henderson
@ 2021-02-26 16:59 ` Claudio Fontana
  2021-02-26 17:12   ` Paolo Bonzini
  2 siblings, 1 reply; 8+ messages in thread
From: Claudio Fontana @ 2021-02-26 16:59 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: pbonzini, philmd, ehabkost

On 2/26/21 5:20 PM, Richard Henderson wrote:
> As discussed during review of Claudio's "i386 cleanup" patch set.
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>   target/i386: Rename helper_fldt, helper_fstt
>   target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
> 
>  target/i386/tcg/fpu_helper.c | 65 +++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
> 
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Tested-by: Claudio Fontana <cfontana@suse.de>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor}
  2021-02-26 16:59 ` [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Claudio Fontana
@ 2021-02-26 17:12   ` Paolo Bonzini
  2021-02-26 17:13     ` Claudio Fontana
  0 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2021-02-26 17:12 UTC (permalink / raw)
  To: Claudio Fontana, Richard Henderson, qemu-devel; +Cc: philmd, ehabkost

On 26/02/21 17:59, Claudio Fontana wrote:
> On 2/26/21 5:20 PM, Richard Henderson wrote:
>> As discussed during review of Claudio's "i386 cleanup" patch set.
>>
>>
>> r~
>>
>>
>> Richard Henderson (2):
>>    target/i386: Rename helper_fldt, helper_fstt
>>    target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
>>
>>   target/i386/tcg/fpu_helper.c | 65 +++++++++++++++++++++++-------------
>>   1 file changed, 41 insertions(+), 24 deletions(-)
>>
> Reviewed-by: Claudio Fontana <cfontana@suse.de>
> Tested-by: Claudio Fontana <cfontana@suse.de>
> 

Claudio, can you merge them in your patch set?

Thanks,

Paolo



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor}
  2021-02-26 17:12   ` Paolo Bonzini
@ 2021-02-26 17:13     ` Claudio Fontana
  0 siblings, 0 replies; 8+ messages in thread
From: Claudio Fontana @ 2021-02-26 17:13 UTC (permalink / raw)
  To: Paolo Bonzini, Richard Henderson, qemu-devel; +Cc: philmd, ehabkost

On 2/26/21 6:12 PM, Paolo Bonzini wrote:
> On 26/02/21 17:59, Claudio Fontana wrote:
>> On 2/26/21 5:20 PM, Richard Henderson wrote:
>>> As discussed during review of Claudio's "i386 cleanup" patch set.
>>>
>>>
>>> r~
>>>
>>>
>>> Richard Henderson (2):
>>>    target/i386: Rename helper_fldt, helper_fstt
>>>    target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
>>>
>>>   target/i386/tcg/fpu_helper.c | 65 +++++++++++++++++++++++-------------
>>>   1 file changed, 41 insertions(+), 24 deletions(-)
>>>
>> Reviewed-by: Claudio Fontana <cfontana@suse.de>
>> Tested-by: Claudio Fontana <cfontana@suse.de>
>>
> 
> Claudio, can you merge them in your patch set?
> 
> Thanks,
> 
> Paolo
> 

Yep, doing that.

Ciao,

Claudio


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-26 17:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-26 16:20 [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Richard Henderson
2021-02-26 16:20 ` [PATCH 1/2] target/i386: Rename helper_fldt, helper_fstt Richard Henderson
2021-02-26 16:45   ` Philippe Mathieu-Daudé
2021-02-26 16:20 ` [PATCH 2/2] target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor Richard Henderson
2021-02-26 16:47   ` Philippe Mathieu-Daudé
2021-02-26 16:59 ` [PATCH 0/2] target/i386: Fix cpu_x86_{fsave,frstor,fxsave,fxrstor} Claudio Fontana
2021-02-26 17:12   ` Paolo Bonzini
2021-02-26 17:13     ` Claudio Fontana

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).