Re: [Qemu-devel] [PATCH 5/9] target/arm: Add aa32_vfp_dreg/aa64_vfp_qreg helpers

[Richard Henderson]

On 01/12/2018 10:24 AM, Peter Maydell wrote:
>> +/**
>> + * aa32_vfp_dreg:
>> + * Return a pointer to the Dn register within env in 32-bit mode.
>> + */
>> +static inline uint64_t *aa32_vfp_dreg(CPUARMState *env, unsigned regno)
>> +{
>> +    return &env->vfp.regs[regno];
>> +}
>> +
>> +/**
>> + * aa64_vfp_dreg:
>> + * Return a pointer to the Qn register within env in 64-bit mode.
>> + */
>> +static inline uint64_t *aa64_vfp_qreg(CPUARMState *env, unsigned regno)
> 
> Comment and code disagree about the name of the function...

Oops.

>> @@ -99,8 +99,10 @@ static int 
>> aarch64_write_elf64_prfpreg(WriteCoreDumpFunction f,
>>
>>      aarch64_note_init(&note, s, "CORE", 5, NT_PRFPREG, sizeof(note.vfp));
>>
>> -    for (i = 0; i < 64; ++i) {
>> -        note.vfp.vregs[i] = cpu_to_dump64(s, float64_val(env->vfp.regs[i]));
>> +    for (i = 0; i < 32; ++i) {
>> +        uint64_t *q = aa64_vfp_qreg(env, i);
>> +        note.vfp.vregs[2*i + 0] = cpu_to_dump64(s, q[0]);
>> +        note.vfp.vregs[2*i + 1] = cpu_to_dump64(s, q[1]);
> 
> This doesn't change the behaviour but I suspect it's wrong for big-endian...

Perhaps.  Since this doesn't change behaviour I'll leave this patch as-is.
That said, how does one go about testing aarch64-big-endian?  I don't think
I've seen a distro for that...

>> -            int elt = (rn * 2 + (index >> 3)) % 64;
>> -            int bitidx = (index & 7) * 8;
>> -            uint64_t val = extract64(env->vfp.regs[elt], bitidx, 8);
>> +            unsigned elt = (rn * 2 + (index >> 3)) % 64;
>> +            unsigned bitidx = (index & 7) * 8;
>> +            uint64_t *q = aa64_vfp_qreg(env, elt >> 1);
>> +            uint64_t val = extract64(q[elt & 1], bitidx, 8);
> 
> 
> Any particular reason for changing all these ints to unsigned ?

*shrug* unsigned division by power-of-two is simpler than signed.  And of
course we know these values must be positive.  I can drop that change if you 
want.

>>  vfp_reg_offset (int dp, int reg)
>>  {
>> -    if (dp)
>> +    if (dp) {
>>          return offsetof(CPUARMState, vfp.regs[reg]);
>> -    else if (reg & 1) {
>> -        return offsetof(CPUARMState, vfp.regs[reg >> 1])
>> -          + offsetof(CPU_DoubleU, l.upper);
>>      } else {
>> -        return offsetof(CPUARMState, vfp.regs[reg >> 1])
>> -          + offsetof(CPU_DoubleU, l.lower);
>> +        long ofs = offsetof(CPUARMState, vfp.regs[reg >> 1]);
>> +        if (reg & 1) {
>> +            ofs += offsetof(CPU_DoubleU, l.upper);
>> +        } else {
>> +            ofs += offsetof(CPU_DoubleU, l.lower);
>> +        }
>> +        return ofs;
>>      }
> 
> This hunk seems to just be rearranging the if-logic without
> doing anything else, right?

There is a name change in there.  But otherwise it's a re-factor to avoid
over-long lines and reduce duplication.


r~