[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 5/9] target/arm: Add aa32_vfp_dreg/aa64_vfp_qreg h
From: |
Richard Henderson |
Subject: |
Re: [Qemu-arm] [PATCH 5/9] target/arm: Add aa32_vfp_dreg/aa64_vfp_qreg helpers |
Date: |
Fri, 12 Jan 2018 10:44:12 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
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(¬e, 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~