Re: [Qemu-arm] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Richard Henderson]

On 5/13/19 5:31 AM, Dave Martin wrote:
> On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
>> These are the SVE equivalents to kvm_arch_get/put_fpsimd.
>>
>> Signed-off-by: Andrew Jones <address@hidden>
>> ---
>>  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 123 insertions(+), 4 deletions(-)
> 
> [...]
> 
>> +static int kvm_arch_put_sve(CPUState *cs)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_one_reg reg;
>> +    int n, ret;
>> +
>> +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
>> +        uint64_t *q = aa64_vfp_qreg(env, n);
>> +#ifdef HOST_WORDS_BIGENDIAN
>> +        uint64_t d[ARM_MAX_VQ * 2];
>> +        int i;
>> +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
>> +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
>> +        }
> 
> Out of interest, why do all this swabbing?  It seems expensive.

Indeed, to me this seems to be the wrong kind of swabbing here.  Exactly what
format is KVM expecting?  Surely it should be the one used by the unpredicated
LDR/STR instructions.  Anything else would seem to be working against the
architecture.

If so, the format is, architecturally, a stream of bytes in index order, which
corresponds to a little-endian stream of words.  So the loop I'd expect to see
here is

    for (i = 0, n = cpu->sve_max_vq; i < n; ++i) {
        d[i] = bswap64(q[i]);
    }


r~