Re: [PATCH v2] target/arm/arch_dump: Add SVE notes

[Richard Henderson]

On 10/16/19 2:13 AM, Andrew Jones wrote:
> On Thu, Oct 10, 2019 at 01:33:02PM -0400, Richard Henderson wrote:
>> Ah, I wonder if you changed things around with the ifdefs due to the pregs.
>> There's no trivial solution for those.  It'd be nice to share the bswapping
>> subroutine that you add in the SVE KVM patch set, and size the temporary 
>> array
>> using ARM_MAX_VQ.
> 
> How about something like this squashed in?

Looks good.


r~


> 
> Thanks,
> drew
> 
> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
> index b02e398430b9..7c7fd23f4d94 100644
> --- a/target/arm/arch_dump.c
> +++ b/target/arm/arch_dump.c
> @@ -182,6 +182,7 @@ static int aarch64_write_elf64_sve(WriteCoreDumpFunction 
> f,
>      struct aarch64_note *note;
>      ARMCPU *cpu = env_archcpu(env);
>      uint32_t vq = sve_current_vq(env);
> +    uint64_t tmp[ARM_MAX_VQ * 2], *r;
>      uint32_t fpr;
>      uint8_t *buf;
>      int ret, i;
> @@ -198,31 +199,14 @@ static int 
> aarch64_write_elf64_sve(WriteCoreDumpFunction f,
>      note->sve.flags = cpu_to_dump16(s, 1);
>  
>      for (i = 0; i < 32; ++i) {
> -#ifdef HOST_WORDS_BIGENDIAN
> -        uint64_t d[vq * 2];
> -        int j;
> -
> -        for (j = 0; j < vq * 2; ++j) {
> -            d[j] = bswap64(env->vfp.zregs[i].d[j]);
> -        }
> -#else
> -        uint64_t *d = &env->vfp.zregs[i].d[0];
> -#endif
> -        memcpy(&buf[sve_zreg_offset(vq, i)], &d[0], vq * 16);
> +        r = sve_bswap64(tmp, &env->vfp.zregs[i].d[0], vq * 2);
> +        memcpy(&buf[sve_zreg_offset(vq, i)], r, vq * 16);
>      }
>  
>      for (i = 0; i < 17; ++i) {
> -#ifdef HOST_WORDS_BIGENDIAN
> -        uint64_t d[DIV_ROUND_UP(vq * 2, 8)];
> -        int j;
> -
> -        for (j = 0; j < DIV_ROUND_UP(vq * 2, 8); ++j) {
> -            d[j] = bswap64(env->vfp.pregs[i].p[j]);
> -        }
> -#else
> -        uint64_t *d = &env->vfp.pregs[i].p[0];
> -#endif
> -        memcpy(&buf[sve_preg_offset(vq, i)], &d[0], vq * 16 / 8);
> +        r = sve_bswap64(tmp, r = &env->vfp.pregs[i].p[0],
> +                        DIV_ROUND_UP(vq * 2, 8));
> +        memcpy(&buf[sve_preg_offset(vq, i)], r, vq * 16 / 8);
>      }
>  
>      fpr = cpu_to_dump32(s, vfp_get_fpsr(env));
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5b9c3e4cd73d..b3092e5213e6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -975,6 +975,31 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned 
> vq);
>  void aarch64_sve_change_el(CPUARMState *env, int old_el,
>                             int new_el, bool el0_a64);
>  void aarch64_add_sve_properties(Object *obj);
> +
> +/*
> + * SVE registers are encoded in KVM's memory in an endianness-invariant 
> format.
> + * The byte at offset i from the start of the in-memory representation 
> contains
> + * the bits [(7 + 8 * i) : (8 * i)] of the register value. As this means the
> + * lowest offsets are stored in the lowest memory addresses, then that nearly
> + * matches QEMU's representation, which is to use an array of host-endian
> + * uint64_t's, where the lower offsets are at the lower indices. To complete
> + * the translation we just need to byte swap the uint64_t's on big-endian 
> hosts.
> + */
> +static inline uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
> +{
> +#ifdef HOST_WORDS_BIGENDIAN
> +    int i;
> +
> +    for (i = 0; i < nr; ++i) {
> +        dst[i] = bswap64(src[i]);
> +    }
> +
> +    return dst;
> +#else
> +    return src;
> +#endif
> +}
> +
>  #else
>  static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>  static inline void aarch64_sve_change_el(CPUARMState *env, int o,
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 876184b8fe4d..e2da756e65ed 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -876,30 +876,6 @@ static int kvm_arch_put_fpsimd(CPUState *cs)
>      return 0;
>  }
>  
> -/*
> - * SVE registers are encoded in KVM's memory in an endianness-invariant 
> format.
> - * The byte at offset i from the start of the in-memory representation 
> contains
> - * the bits [(7 + 8 * i) : (8 * i)] of the register value. As this means the
> - * lowest offsets are stored in the lowest memory addresses, then that nearly
> - * matches QEMU's representation, which is to use an array of host-endian
> - * uint64_t's, where the lower offsets are at the lower indices. To complete
> - * the translation we just need to byte swap the uint64_t's on big-endian 
> hosts.
> - */
> -static uint64_t *sve_bswap64(uint64_t *dst, uint64_t *src, int nr)
> -{
> -#ifdef HOST_WORDS_BIGENDIAN
> -    int i;
> -
> -    for (i = 0; i < nr; ++i) {
> -        dst[i] = bswap64(src[i]);
> -    }
> -
> -    return dst;
> -#else
> -    return src;
> -#endif
> -}
> -
>  /*
>   * KVM SVE registers come in slices where ZREGs have a slice size of 2048 
> bits
>   * and PREGS and the FFR have a slice size of 256 bits. However we simply 
> hard
>