Re: [Qemu-devel] [PATCH v3 2/2] target-arm: Add support for AArch32 ARMv8 CRC32 instructions

[Peter Maydell]

On 25 February 2014 09:32, Will Newton <address@hidden> wrote:
> Add support for AArch32 CRC32 and CRC32C instructions added in ARMv8.
> The CRC32-C implementation used is the built-in qemu implementation
> and The CRC-32 implementation is from zlib. This requires adding zlib
> to LIBS to ensure it is linked for the linux-user binary.
>
> Signed-off-by: Will Newton <address@hidden>
> ---
>  configure              |  2 +-
>  target-arm/helper.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  target-arm/helper.h    |  3 +++
>  target-arm/translate.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 91 insertions(+), 1 deletion(-)
>
> Changes in v3:
>  - Use extract32 to get register fields from instruction
>
> diff --git a/configure b/configure
> index 4648117..822842c 100755
> --- a/configure
> +++ b/configure
> @@ -1550,7 +1550,7 @@ EOF
>              "Make sure to have the zlib libs and headers installed."
>      fi
>  fi
> -libs_softmmu="$libs_softmmu -lz"
> +LIBS="$LIBS -lz"
>
>  ##########################################
>  # libseccomp check
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 5ae08c9..294cfaf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5,6 +5,8 @@
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/bitops.h"
> +#include "qemu/crc32c.h"
> +#include <zlib.h> /* For crc32 */
>
>  #ifndef CONFIG_USER_ONLY
>  static inline int get_phys_addr(CPUARMState *env, uint32_t address,
> @@ -4468,3 +4470,40 @@ int arm_rmode_to_sf(int rmode)
>      }
>      return rmode;
>  }
> +
> +static void crc_init_buffer(uint8_t *buf, uint32_t val, uint32_t bytes)
> +{
> +    memset(buf, 0, 4);
> +
> +    if (bytes == 1) {
> +        buf[0] = val & 0xff;
> +    } else if (bytes == 2) {
> +        buf[0] = val & 0xff;
> +        buf[1] = (val >> 8) & 0xff;
> +    } else {
> +        buf[0] = val & 0xff;
> +        buf[1] = (val >> 8) & 0xff;
> +        buf[2] = (val >> 16) & 0xff;
> +        buf[3] = (val >> 24) & 0xff;
> +    }
> +}
> +
> +uint32_t HELPER(crc32)(uint32_t acc, uint32_t val, uint32_t bytes)
> +{
> +    uint8_t buf[4];
> +
> +    crc_init_buffer(buf, val, bytes);
> +
> +    /* zlib crc32 converts the accumulator and output to one's complement.  
> */
> +    return crc32(acc ^ 0xffffffff, buf, bytes) ^ 0xffffffff;
> +}
> +
> +uint32_t HELPER(crc32c)(uint32_t acc, uint32_t val, uint32_t bytes)
> +{
> +    uint8_t buf[4];
> +
> +    crc_init_buffer(buf, val, bytes);
> +
> +    /* Linux crc32c converts the output to one's complement.  */
> +    return crc32c(acc, buf, bytes) ^ 0xffffffff;
> +}
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 951e6ad..fb92e53 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -494,6 +494,9 @@ DEF_HELPER_3(neon_qzip32, void, env, i32, i32)
>  DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32)
>  DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32)
>
> +DEF_HELPER_3(crc32, i32, i32, i32, i32)
> +DEF_HELPER_3(crc32c, i32, i32, i32, i32)

These helpers have no side effects and don't access TCG
globals, so we can set the flags appropriately:

DEF_HELPER_FLAGS_3(crc32, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)

and similarly for crc32c.

> +
>  #ifdef TARGET_AARCH64
>  #include "helper-a64.h"
>  #endif
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 782aab8..8e87869 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7541,6 +7541,32 @@ static void disas_arm_insn(CPUARMState * env, 
> DisasContext *s)
>              store_reg(s, 14, tmp2);
>              gen_bx(s, tmp);
>              break;
> +        case 0x4:
> +        {
> +            /* crc32/crc32c */
> +            uint32_t c = extract32(insn, 9, 1);
> +
> +            /* size == 64 is UNPREDICTABLE but handle as UNDEFINED.  */
> +            if (op1 == 0x3) {
> +                goto illegal_op;
> +            }

I think it would also be nice to UNDEF if the SBZ bits
11, 10 and 8 are not all zero. (This is a CONSTRAINED
UNPREDICTABLE case and "instruction operates as if the
bit had the /should-be/ value" is a valid implementation
choice, but as a quality-of-implementation issue I think
choosing to UNDEF is better.)

> +
> +            rn = extract32(insn, 16, 4);
> +            rd = extract32(insn, 12, 4);
> +
> +            tmp = load_reg(s, rn);
> +            tmp2 = load_reg(s, rm);
> +            tmp3 = tcg_const_i32(1 << op1);
> +            if (c) {
> +                gen_helper_crc32c(tmp, tmp, tmp2, tmp3);
> +            } else {
> +                gen_helper_crc32(tmp, tmp, tmp2, tmp3);
> +            }
> +            tcg_temp_free_i32(tmp2);
> +            tcg_temp_free_i32(tmp3);
> +            store_reg(s, rd, tmp);
> +            break;

This looks good, but it's enabling the CRC instructions
for all CPUs, and we only want to do that for CPUs which
actually implement them. (They're optional even for v8
CPUs). You need to add a new ARM_FEATURE_CRC flag to
the arm_features enum in cpu.h, set it in the cpu.c
arm_any_initfn(), and test for it here.

> +        }
>          case 0x5: /* saturating add/subtract */
>              ARCH(5TE);
>              rd = (insn >> 12) & 0xf;
> @@ -9125,6 +9151,28 @@ static int disas_thumb2_insn(CPUARMState *env, 
> DisasContext *s, uint16_t insn_hw
>                  case 0x18: /* clz */
>                      gen_helper_clz(tmp, tmp);
>                      break;
> +                case 0x20:
> +                case 0x21:
> +                case 0x22:
> +                case 0x28:
> +                case 0x29:
> +                case 0x2a:
> +                {
> +                    /* crc32/crc32c */
> +                    uint32_t sz = op & 0x3;
> +                    uint32_t c = op & 0x8;
> +
> +                    tmp2 = load_reg(s, rm);
> +                    tmp3 = tcg_const_i32(1 << sz);
> +                    if (c) {
> +                        gen_helper_crc32c(tmp, tmp, tmp2, tmp3);
> +                    } else {
> +                        gen_helper_crc32(tmp, tmp, tmp2, tmp3);
> +                    }
> +                    tcg_temp_free_i32(tmp2);
> +                    tcg_temp_free_i32(tmp3);
> +                    break;
> +                }

Similarly this needs to check ARM_FEATURE_CRC.

Patch looks good otherwise and passes my tests.

thanks
-- PMM