[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/ppc: Fix bcdsub. emulation when result overflows
|
From: |
David Gibson |
|
Subject: |
Re: [PATCH] target/ppc: Fix bcdsub. emulation when result overflows |
|
Date: |
Tue, 23 Feb 2021 11:29:04 +1100 |
On Mon, Feb 22, 2021 at 04:40:35PM -0300, Fabiano Rosas wrote:
65;6203;1c> The commit d03b174a83 (target/ppc: simplify bcdadd/sub functions)
> meant to simplify some of the code but it inadvertently altered the
> way the CR6 field is set after the operation has overflowed.
>
> The CR6 bits are set based on the *unbounded* result of the operation,
> so we need to look at the result before returning from bcd_add_mag,
> otherwise we will look at 0 when it overflows.
>
> Consider the following subtraction:
>
> v0 = 0x9999999999999999999999999999999c (maximum positive BCD value)
> v1 = 0x0000000000000000000000000000001d (negative one BCD value)
> bcdsub. v0,v0,v1,0
>
> The Power ISA 2.07B says:
> If the unbounded result is greater than zero, do the following.
> If PS=0, the sign code of the result is set to 0b1100.
> If PS=1, the sign code of the result is set to 0b1111.
> If the operation overflows, CR field 6 is set to 0b0101. Otherwise,
> CR field 6 is set to 0b0100.
>
> POWER9 hardware:
> vr0 = 0x0000000000000000000000000000000c (positive zero BCD value)
> cr6 = 0b0101 (0x5) (positive, overflow)
>
> QEMU:
> vr0 = 0x0000000000000000000000000000000c (positive zero BCD value)
> cr6 = 0b0011 (0x3) (zero, overflow) <--- wrong
>
> This patch reverts the part of d03b174a83 that introduced the
> problem and adds a test-case to avoid further regressions:
>
> before:
> $ make run-tcg-tests-ppc64le-linux-user
> (...)
> TEST bcdsub on ppc64le
> bcdsub: qemu/tests/tcg/ppc64le/bcdsub.c:58: test_bcdsub_gt:
> Assertion `(cr >> 4) == ((1 << 2) | (1 << 0))' failed.
>
> Fixes: d03b174a83 (target/ppc: simplify bcdadd/sub functions)
> Reported-by: Paul Clarke <pc@us.ibm.com>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Applied to ppc-for-6.0, thanks.
> ---
> target/ppc/int_helper.c | 13 ++-
> tests/tcg/configure.sh | 6 ++
> tests/tcg/ppc64/Makefile.target | 13 +++
> tests/tcg/ppc64le/Makefile.target | 12 +++
> tests/tcg/ppc64le/bcdsub.c | 130 ++++++++++++++++++++++++++++++
> 5 files changed, 171 insertions(+), 3 deletions(-)
> create mode 100644 tests/tcg/ppc64/Makefile.target
> create mode 100644 tests/tcg/ppc64le/Makefile.target
> create mode 100644 tests/tcg/ppc64le/bcdsub.c
>
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index 0b682a1f94..429de28494 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -2175,14 +2175,17 @@ static int bcd_cmp_mag(ppc_avr_t *a, ppc_avr_t *b)
> return 0;
> }
>
> -static void bcd_add_mag(ppc_avr_t *t, ppc_avr_t *a, ppc_avr_t *b, int
> *invalid,
> +static int bcd_add_mag(ppc_avr_t *t, ppc_avr_t *a, ppc_avr_t *b, int
> *invalid,
> int *overflow)
> {
> int carry = 0;
> int i;
> + int is_zero = 1;
> +
> for (i = 1; i <= 31; i++) {
> uint8_t digit = bcd_get_digit(a, i, invalid) +
> bcd_get_digit(b, i, invalid) + carry;
> + is_zero &= (digit == 0);
> if (digit > 9) {
> carry = 1;
> digit -= 10;
> @@ -2194,6 +2197,7 @@ static void bcd_add_mag(ppc_avr_t *t, ppc_avr_t *a,
> ppc_avr_t *b, int *invalid,
> }
>
> *overflow = carry;
> + return is_zero;
> }
>
> static void bcd_sub_mag(ppc_avr_t *t, ppc_avr_t *a, ppc_avr_t *b, int
> *invalid,
> @@ -2225,14 +2229,15 @@ uint32_t helper_bcdadd(ppc_avr_t *r, ppc_avr_t *a,
> ppc_avr_t *b, uint32_t ps)
> int sgnb = bcd_get_sgn(b);
> int invalid = (sgna == 0) || (sgnb == 0);
> int overflow = 0;
> + int zero = 0;
> uint32_t cr = 0;
> ppc_avr_t result = { .u64 = { 0, 0 } };
>
> if (!invalid) {
> if (sgna == sgnb) {
> result.VsrB(BCD_DIG_BYTE(0)) = bcd_preferred_sgn(sgna, ps);
> - bcd_add_mag(&result, a, b, &invalid, &overflow);
> - cr = bcd_cmp_zero(&result);
> + zero = bcd_add_mag(&result, a, b, &invalid, &overflow);
> + cr = (sgna > 0) ? CRF_GT : CRF_LT;
> } else {
> int magnitude = bcd_cmp_mag(a, b);
> if (magnitude > 0) {
> @@ -2255,6 +2260,8 @@ uint32_t helper_bcdadd(ppc_avr_t *r, ppc_avr_t *a,
> ppc_avr_t *b, uint32_t ps)
> cr = CRF_SO;
> } else if (overflow) {
> cr |= CRF_SO;
> + } else if (zero) {
> + cr |= CRF_EQ;
> }
>
> *r = result;
> diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
> index 551c02f469..a0b709948c 100755
> --- a/tests/tcg/configure.sh
> +++ b/tests/tcg/configure.sh
> @@ -251,6 +251,12 @@ for target in $target_list; do
> echo "CROSS_CC_HAS_ARMV8_MTE=y" >> $config_target_mak
> fi
> ;;
> + ppc*)
> + if do_compiler "$target_compiler" $target_compiler_cflags \
> + -mpower8-vector -o $TMPE $TMPC; then
> + echo "CROSS_CC_HAS_POWER8_VECTOR=y" >> $config_target_mak
> + fi
> + ;;
> esac
>
> enabled_cross_compilers="$enabled_cross_compilers $target_compiler"
> diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
> new file mode 100644
> index 0000000000..0c6a4585fc
> --- /dev/null
> +++ b/tests/tcg/ppc64/Makefile.target
> @@ -0,0 +1,13 @@
> +# -*- Mode: makefile -*-
> +#
> +# ppc64 specific tweaks
> +
> +VPATH += $(SRC_PATH)/tests/tcg/ppc64
> +VPATH += $(SRC_PATH)/tests/tcg/ppc64le
> +
> +ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_POWER8_VECTOR),)
> +PPC64_TESTS=bcdsub
> +endif
> +bcdsub: CFLAGS += -mpower8-vector
> +
> +TESTS += $(PPC64_TESTS)
> diff --git a/tests/tcg/ppc64le/Makefile.target
> b/tests/tcg/ppc64le/Makefile.target
> new file mode 100644
> index 0000000000..1acfcff94a
> --- /dev/null
> +++ b/tests/tcg/ppc64le/Makefile.target
> @@ -0,0 +1,12 @@
> +# -*- Mode: makefile -*-
> +#
> +# ppc64le specific tweaks
> +
> +VPATH += $(SRC_PATH)/tests/tcg/ppc64le
> +
> +ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_POWER8_VECTOR),)
> +PPC64LE_TESTS=bcdsub
> +endif
> +bcdsub: CFLAGS += -mpower8-vector
> +
> +TESTS += $(PPC64LE_TESTS)
> diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
> new file mode 100644
> index 0000000000..8c188cae6d
> --- /dev/null
> +++ b/tests/tcg/ppc64le/bcdsub.c
> @@ -0,0 +1,130 @@
> +#include <assert.h>
> +#include <unistd.h>
> +#include <signal.h>
> +
> +#define CRF_LT (1 << 3)
> +#define CRF_GT (1 << 2)
> +#define CRF_EQ (1 << 1)
> +#define CRF_SO (1 << 0)
> +#define UNDEF 0
> +
> +#define BCDSUB(vra, vrb, ps) \
> + asm ("bcdsub. %1,%2,%3,%4;" \
> + "mfocrf %0,0b10;" \
> + : "=r" (cr), "=v" (vrt) \
> + : "v" (vra), "v" (vrb), "i" (ps) \
> + : );
> +
> +#define TEST(vra, vrb, ps, exp_res, exp_cr6) \
> + do { \
> + __int128 vrt = 0; \
> + int cr = 0; \
> + BCDSUB(vra, vrb, ps); \
> + if (exp_res) \
> + assert(vrt == exp_res); \
> + assert((cr >> 4) == exp_cr6); \
> + } while (0)
> +
> +
> +/*
> + * Unbounded result is equal to zero:
> + * sign = (PS) ? 0b1111 : 0b1100
> + * CR6 = 0b0010
> + */
> +void test_bcdsub_eq(void)
> +{
> + __int128 a, b;
> +
> + /* maximum positive BCD value */
> + a = b = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
> +
> + TEST(a, b, 0, 0xc, CRF_EQ);
> + TEST(a, b, 1, 0xf, CRF_EQ);
> +}
> +
> +/*
> + * Unbounded result is greater than zero:
> + * sign = (PS) ? 0b1111 : 0b1100
> + * CR6 = (overflow) ? 0b0101 : 0b0100
> + */
> +void test_bcdsub_gt(void)
> +{
> + __int128 a, b, c;
> +
> + /* maximum positive BCD value */
> + a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
> +
> + /* negative one BCD value */
> + b = (__int128) 0x1d;
> +
> + TEST(a, b, 0, 0xc, (CRF_GT | CRF_SO));
> + TEST(a, b, 1, 0xf, (CRF_GT | CRF_SO));
> +
> + c = (((__int128) 0x9999999999999999) << 64 | 0x999999999999998c);
> +
> + TEST(c, b, 0, a, CRF_GT);
> + TEST(c, b, 1, (a | 0x3), CRF_GT);
> +}
> +
> +/*
> + * Unbounded result is less than zero:
> + * sign = 0b1101
> + * CR6 = (overflow) ? 0b1001 : 0b1000
> + */
> +void test_bcdsub_lt(void)
> +{
> + __int128 a, b;
> +
> + /* positive zero BCD value */
> + a = (__int128) 0xc;
> +
> + /* positive one BCD value */
> + b = (__int128) 0x1c;
> +
> + TEST(a, b, 0, 0x1d, CRF_LT);
> + TEST(a, b, 1, 0x1d, CRF_LT);
> +
> + /* maximum negative BCD value */
> + a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999d);
> +
> + /* positive one BCD value */
> + b = (__int128) 0x1c;
> +
> + TEST(a, b, 0, 0xd, (CRF_LT | CRF_SO));
> + TEST(a, b, 1, 0xd, (CRF_LT | CRF_SO));
> +}
> +
> +void test_bcdsub_invalid(void)
> +{
> + __int128 a, b;
> +
> + /* positive one BCD value */
> + a = (__int128) 0x1c;
> + b = 0xf00;
> +
> + TEST(a, b, 0, UNDEF, CRF_SO);
> + TEST(a, b, 1, UNDEF, CRF_SO);
> +
> + TEST(b, a, 0, UNDEF, CRF_SO);
> + TEST(b, a, 1, UNDEF, CRF_SO);
> +
> + a = 0xbad;
> +
> + TEST(a, b, 0, UNDEF, CRF_SO);
> + TEST(a, b, 1, UNDEF, CRF_SO);
> +}
> +
> +int main(void)
> +{
> + struct sigaction action;
> +
> + action.sa_handler = _exit;
> + sigaction(SIGABRT, &action, NULL);
> +
> + test_bcdsub_eq();
> + test_bcdsub_gt();
> + test_bcdsub_lt();
> + test_bcdsub_invalid();
> +
> + return 0;
> +}
--
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
signature.asc
Description: PGP signature