softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

[Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by Laurent Vivier 5 years, 5 months ago

commit 27ae5109a2 has introduced an assembly instruction only supported
by ISA 3.0B and it fails to execute on previous versions of the POWER
CPU (like PowerPC G5).

This patch fixes that by checking the ISA level, and falls back to
the default C function if the instruction is not supported.

Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
       (softfloat: Specialize udiv_qrnnd for ppc64)
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index c86687fa5e..fe98b33df9 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -78,6 +78,9 @@ this code that are retained.
 /* Portions of this work are licensed under the terms of the GNU GPL,
  * version 2 or later. See the COPYING file in the top-level directory.
  */
+#if defined(_ARCH_PPC64)
+extern bool have_isa_3_00;
+#endif
 
 /*----------------------------------------------------------------------------
 | Shifts `a' right by the number of bits given in `count'.  If any nonzero
@@ -647,25 +650,29 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
     asm("dlgr %0, %1" : "+r"(n) : "r"(d));
     *r = n >> 64;
     return n;
-#elif defined(_ARCH_PPC64)
-    /* From Power ISA 3.0B, programming note for divdeu.  */
-    uint64_t q1, q2, Q, r1, r2, R;
-    asm("divdeu %0,%2,%4; divdu %1,%3,%4"
-        : "=&r"(q1), "=r"(q2)
-        : "r"(n1), "r"(n0), "r"(d));
-    r1 = -(q1 * d);         /* low part of (n1<<64) - (q1 * d) */
-    r2 = n0 - (q2 * d);
-    Q = q1 + q2;
-    R = r1 + r2;
-    if (R >= d || R < r2) { /* overflow implies R > d */
-        Q += 1;
-        R -= d;
-    }
-    *r = R;
-    return Q;
 #else
     uint64_t d0, d1, q0, q1, r1, r0, m;
 
+#if defined(_ARCH_PPC64)
+    if (have_isa_3_00) {
+        /* From Power ISA 3.0B, programming note for divdeu.  */
+        uint64_t q1, q2, Q, r1, r2, R;
+        asm("divdeu %0,%2,%4; divdu %1,%3,%4"
+            : "=&r"(q1), "=r"(q2)
+            : "r"(n1), "r"(n0), "r"(d));
+        r1 = -(q1 * d);         /* low part of (n1<<64) - (q1 * d) */
+        r2 = n0 - (q2 * d);
+        Q = q1 + q2;
+        R = r1 + r2;
+        if (R >= d || R < r2) { /* overflow implies R > d */
+            Q += 1;
+            R -= d;
+        }
+        *r = R;
+        return Q;
+    }
+#endif
+
     d0 = (uint32_t)d;
     d1 = d >> 32;
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by Peter Maydell 5 years, 5 months ago

On 1 November 2018 at 17:38, Laurent Vivier <laurent@vivier.eu> wrote:
> commit 27ae5109a2 has introduced an assembly instruction only supported
> by ISA 3.0B and it fails to execute on previous versions of the POWER
> CPU (like PowerPC G5).
>
> This patch fixes that by checking the ISA level, and falls back to
> the default C function if the instruction is not supported.
>
> Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
>        (softfloat: Specialize udiv_qrnnd for ppc64)
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
> index c86687fa5e..fe98b33df9 100644
> --- a/include/fpu/softfloat-macros.h
> +++ b/include/fpu/softfloat-macros.h
> @@ -78,6 +78,9 @@ this code that are retained.
>  /* Portions of this work are licensed under the terms of the GNU GPL,
>   * version 2 or later. See the COPYING file in the top-level directory.
>   */
> +#if defined(_ARCH_PPC64)
> +extern bool have_isa_3_00;
> +#endif

I was wondering where this bool came from. The answer is
that it's defined in tcg/ppc/tcg-target.inc.c. It's ok to
use it here because fpu/softfloat.c is only compiled if
CONFIG_TCG is true, so the tcg code will be present. The
other user of this include file is target/m68k/softfloat.c,
which also will only be compiled for a ppc host if CONFIG_TCG.

It's a little awkward to be borrowing this tcg/ppc internal
flag into the softfloat code, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by Laurent Vivier 5 years, 5 months ago

On 01/11/2018 18:49, Peter Maydell wrote:
> On 1 November 2018 at 17:38, Laurent Vivier <laurent@vivier.eu> wrote:
>> commit 27ae5109a2 has introduced an assembly instruction only supported
>> by ISA 3.0B and it fails to execute on previous versions of the POWER
>> CPU (like PowerPC G5).
>>
>> This patch fixes that by checking the ISA level, and falls back to
>> the default C function if the instruction is not supported.
>>
>> Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
>>        (softfloat: Specialize udiv_qrnnd for ppc64)
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
>> index c86687fa5e..fe98b33df9 100644
>> --- a/include/fpu/softfloat-macros.h
>> +++ b/include/fpu/softfloat-macros.h
>> @@ -78,6 +78,9 @@ this code that are retained.
>>  /* Portions of this work are licensed under the terms of the GNU GPL,
>>   * version 2 or later. See the COPYING file in the top-level directory.
>>   */
>> +#if defined(_ARCH_PPC64)
>> +extern bool have_isa_3_00;
>> +#endif
> 
> I was wondering where this bool came from. The answer is
> that it's defined in tcg/ppc/tcg-target.inc.c. It's ok to
> use it here because fpu/softfloat.c is only compiled if
> CONFIG_TCG is true, so the tcg code will be present. The
> other user of this include file is target/m68k/softfloat.c,
> which also will only be compiled for a ppc host if CONFIG_TCG.
> 
> It's a little awkward to be borrowing this tcg/ppc internal
> flag into the softfloat code, though.

I agree, I was hopping Richard can advice another way to do that.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by David Gibson 5 years, 5 months ago

On Thu, Nov 01, 2018 at 06:54:59PM +0100, Laurent Vivier wrote:
> On 01/11/2018 18:49, Peter Maydell wrote:
> > On 1 November 2018 at 17:38, Laurent Vivier <laurent@vivier.eu> wrote:
> >> commit 27ae5109a2 has introduced an assembly instruction only supported
> >> by ISA 3.0B and it fails to execute on previous versions of the POWER
> >> CPU (like PowerPC G5).
> >>
> >> This patch fixes that by checking the ISA level, and falls back to
> >> the default C function if the instruction is not supported.
> >>
> >> Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
> >>        (softfloat: Specialize udiv_qrnnd for ppc64)
> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >> ---
> >>  include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
> >>  1 file changed, 23 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
> >> index c86687fa5e..fe98b33df9 100644
> >> --- a/include/fpu/softfloat-macros.h
> >> +++ b/include/fpu/softfloat-macros.h
> >> @@ -78,6 +78,9 @@ this code that are retained.
> >>  /* Portions of this work are licensed under the terms of the GNU GPL,
> >>   * version 2 or later. See the COPYING file in the top-level directory.
> >>   */
> >> +#if defined(_ARCH_PPC64)
> >> +extern bool have_isa_3_00;
> >> +#endif
> > 
> > I was wondering where this bool came from. The answer is
> > that it's defined in tcg/ppc/tcg-target.inc.c. It's ok to
> > use it here because fpu/softfloat.c is only compiled if
> > CONFIG_TCG is true, so the tcg code will be present. The
> > other user of this include file is target/m68k/softfloat.c,
> > which also will only be compiled for a ppc host if CONFIG_TCG.
> > 
> > It's a little awkward to be borrowing this tcg/ppc internal
> > flag into the softfloat code, though.
> 
> I agree, I was hopping Richard can advice another way to do that.

We already have ISA version flags in insns_flags & insns_flags2, so
I'm hoping we can use those rather than introduce a new bool.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by Laurent Vivier 5 years, 5 months ago

On 03/11/2018 13:57, David Gibson wrote:
> On Thu, Nov 01, 2018 at 06:54:59PM +0100, Laurent Vivier wrote:
>> On 01/11/2018 18:49, Peter Maydell wrote:
>>> On 1 November 2018 at 17:38, Laurent Vivier <laurent@vivier.eu> wrote:
>>>> commit 27ae5109a2 has introduced an assembly instruction only supported
>>>> by ISA 3.0B and it fails to execute on previous versions of the POWER
>>>> CPU (like PowerPC G5).
>>>>
>>>> This patch fixes that by checking the ISA level, and falls back to
>>>> the default C function if the instruction is not supported.
>>>>
>>>> Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
>>>>        (softfloat: Specialize udiv_qrnnd for ppc64)
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>>  include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
>>>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
>>>> index c86687fa5e..fe98b33df9 100644
>>>> --- a/include/fpu/softfloat-macros.h
>>>> +++ b/include/fpu/softfloat-macros.h
>>>> @@ -78,6 +78,9 @@ this code that are retained.
>>>>  /* Portions of this work are licensed under the terms of the GNU GPL,
>>>>   * version 2 or later. See the COPYING file in the top-level directory.
>>>>   */
>>>> +#if defined(_ARCH_PPC64)
>>>> +extern bool have_isa_3_00;
>>>> +#endif
>>>
>>> I was wondering where this bool came from. The answer is
>>> that it's defined in tcg/ppc/tcg-target.inc.c. It's ok to
>>> use it here because fpu/softfloat.c is only compiled if
>>> CONFIG_TCG is true, so the tcg code will be present. The
>>> other user of this include file is target/m68k/softfloat.c,
>>> which also will only be compiled for a ppc host if CONFIG_TCG.
>>>
>>> It's a little awkward to be borrowing this tcg/ppc internal
>>> flag into the softfloat code, though.
>>
>> I agree, I was hopping Richard can advice another way to do that.
> 
> We already have ISA version flags in insns_flags & insns_flags2, so
> I'm hoping we can use those rather than introduce a new bool.
> 

Richard has sent another patch using "defined(_ARCH_PWR7)"

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] softfloat: don't execute ppc64 ISA 3.0B instruction if it is not supported

Posted by Richard Henderson 5 years, 5 months ago

On 11/1/18 5:38 PM, Laurent Vivier wrote:
> commit 27ae5109a2 has introduced an assembly instruction only supported
> by ISA 3.0B and it fails to execute on previous versions of the POWER
> CPU (like PowerPC G5).
> 
> This patch fixes that by checking the ISA level, and falls back to
> the default C function if the instruction is not supported.
> 
> Fixes: 27ae5109a2ba8b6b679cce3e03e16570a34390a0
>        (softfloat: Specialize udiv_qrnnd for ppc64)
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/fpu/softfloat-macros.h | 39 ++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 16 deletions(-)

Blah.  The divdeu insn was added into v2.06.

I knew I had tested this with a power7 host, which I think of as old.
But I guess not old enough.  I'll work something up.


r~