[v2,6/6] target-arm: dump-guest-memory: add fpregset notes

Also refactors note init code to avoid code duplication.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 22 deletions(-)

On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> Also refactors note init code to avoid code duplication.

Can you squash those parts down into the preceding patch?

>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 139 insertions(+), 22 deletions(-)
>
> diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
> index 5debe549d721d..8d74788411d79 100644
> --- a/target-arm/arch_dump.c
> +++ b/target-arm/arch_dump.c
> @@ -35,6 +35,16 @@ struct aarch64_user_regs {
>
>  QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
>
> +/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h */

The kernel struct uses __uint128_t, not an array of uint64_t; that's
worth commenting because it clarifies what the expected format is.

> +struct aarch64_user_vfp_state {
> +    uint64_t vregs[64];
> +    uint32_t fpsr;
> +    uint32_t fpcr;
> +    char pad[8];
> +} QEMU_PACKED;
> +
> +QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_vfp_state) != 528);
> +
>  /* struct elf_prstatus from include/uapi/linux/elfcore.h */
>  struct aarch64_elf_prstatus {
>      char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
> @@ -51,10 +61,77 @@ QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
>  struct aarch64_note {
>      Elf64_Nhdr hdr;
>      char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> -    struct aarch64_elf_prstatus prstatus;
> +    union {
> +        struct aarch64_elf_prstatus prstatus;
> +        struct aarch64_user_vfp_state vfp;
> +    };
>  } QEMU_PACKED;
>
> -QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
> +#define AARCH64_NOTE_HEADER_SIZE offsetof(struct aarch64_note, prstatus)
> +#define AARCH64_PRSTATUS_NOTE_SIZE \
> +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_elf_prstatus))
> +#define AARCH64_FPREGSET_NOTE_SIZE \
> +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
> +
> +static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
> +                              Elf64_Word type, Elf64_Word descsz)
> +{
> +    memset(note, 0, sizeof(*note));
> +
> +    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> +    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
> +    note->hdr.n_type = cpu_to_dump32(s, type);
> +
> +    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
> +}
> +
> +static void arm_write_vregs(uint64_t *vregs, int num_regs,
> +                            CPUARMState *env, DumpState *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < num_regs; ++i) {
> +        vregs[i] = cpu_to_dump64(s, env->vfp.regs[i]);
> +    }
> +
> +    if (s->dump_info.d_endian == ELFDATA2MSB) {
> +        /* We must always swap vfp.regs's 2n and 2n+1 entries when
> +         * generating BE notes, because even big endian hosts use
> +         * 2n+1 for the high half.
> +         */
> +        for (i = 0; i < num_regs/2; ++i) {
> +            uint64_t tmp = vregs[2*i];
> +            vregs[2*i] = vregs[2*i+1];
> +            vregs[2*i+1] = tmp;
> +        }

This swapping is correct for AArch64, where the core dump format
is an array of 128 bit Qn register values (which in QEMU
are stored in vfp.regs[2n+1]:vfp.regs[2n] as a pair of 64
bit values). However it's wrong for AArch32, where the core
dump format is an array of 64-bit Dn register values, which
in QEMU are just in vfp.regs[n].

(See the comment in target-arm/cpu.h about VFP/Neon register
state and different views of the register bank.)

> +static int
> +arm_write_elf32_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
> +                         int id, DumpState *s)
> +{
> +    struct arm_note note;
> +    int ret;
> +
> +    arm_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
> +
> +    arm_write_vregs(note.vfp.vregs, 32, env, s);
> +
> +    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));

Do consumers care if we write out a dump with FP register
state for a CPU which doesn't implement FP? (For AArch64
FP is always present, but for AArch32 it may not be.)

thanks
-- PMM

On Thu, Dec 03, 2015 at 12:27:34PM +0000, Peter Maydell wrote:
> On 25 November 2015 at 00:37, Andrew Jones <drjones@redhat.com> wrote:
> > Also refactors note init code to avoid code duplication.
> 
> Can you squash those parts down into the preceding patch?

Sure, but there wasn't any duplication of code in the first patch, only
the FP register notes introduce that possibility.

> 
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target-arm/arch_dump.c | 161 ++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 139 insertions(+), 22 deletions(-)
> >
> > diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
> > index 5debe549d721d..8d74788411d79 100644
> > --- a/target-arm/arch_dump.c
> > +++ b/target-arm/arch_dump.c
> > @@ -35,6 +35,16 @@ struct aarch64_user_regs {
> >
> >  QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_regs) != 272);
> >
> > +/* struct user_fpsimd_state from arch/arm64/include/uapi/asm/ptrace.h */
> 
> The kernel struct uses __uint128_t, not an array of uint64_t; that's
> worth commenting because it clarifies what the expected format is.

OK

> 
> > +struct aarch64_user_vfp_state {
> > +    uint64_t vregs[64];
> > +    uint32_t fpsr;
> > +    uint32_t fpcr;
> > +    char pad[8];
> > +} QEMU_PACKED;
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(struct aarch64_user_vfp_state) != 528);
> > +
> >  /* struct elf_prstatus from include/uapi/linux/elfcore.h */
> >  struct aarch64_elf_prstatus {
> >      char pad1[32]; /* 32 == offsetof(struct elf_prstatus, pr_pid) */
> > @@ -51,10 +61,77 @@ QEMU_BUILD_BUG_ON(sizeof(struct aarch64_elf_prstatus) != 392);
> >  struct aarch64_note {
> >      Elf64_Nhdr hdr;
> >      char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
> > -    struct aarch64_elf_prstatus prstatus;
> > +    union {
> > +        struct aarch64_elf_prstatus prstatus;
> > +        struct aarch64_user_vfp_state vfp;
> > +    };
> >  } QEMU_PACKED;
> >
> > -QEMU_BUILD_BUG_ON(sizeof(struct aarch64_note) != 412);
> > +#define AARCH64_NOTE_HEADER_SIZE offsetof(struct aarch64_note, prstatus)
> > +#define AARCH64_PRSTATUS_NOTE_SIZE \
> > +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_elf_prstatus))
> > +#define AARCH64_FPREGSET_NOTE_SIZE \
> > +            (AARCH64_NOTE_HEADER_SIZE + sizeof(struct aarch64_user_vfp_state))
> > +
> > +static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
> > +                              Elf64_Word type, Elf64_Word descsz)
> > +{
> > +    memset(note, 0, sizeof(*note));
> > +
> > +    note->hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
> > +    note->hdr.n_descsz = cpu_to_dump32(s, descsz);
> > +    note->hdr.n_type = cpu_to_dump32(s, type);
> > +
> > +    memcpy(note->name, NOTE_NAME, NOTE_NAMESZ);
> > +}
> > +
> > +static void arm_write_vregs(uint64_t *vregs, int num_regs,
> > +                            CPUARMState *env, DumpState *s)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < num_regs; ++i) {
> > +        vregs[i] = cpu_to_dump64(s, env->vfp.regs[i]);
> > +    }
> > +
> > +    if (s->dump_info.d_endian == ELFDATA2MSB) {
> > +        /* We must always swap vfp.regs's 2n and 2n+1 entries when
> > +         * generating BE notes, because even big endian hosts use
> > +         * 2n+1 for the high half.
> > +         */
> > +        for (i = 0; i < num_regs/2; ++i) {
> > +            uint64_t tmp = vregs[2*i];
> > +            vregs[2*i] = vregs[2*i+1];
> > +            vregs[2*i+1] = tmp;
> > +        }
> 
> This swapping is correct for AArch64, where the core dump format
> is an array of 128 bit Qn register values (which in QEMU
> are stored in vfp.regs[2n+1]:vfp.regs[2n] as a pair of 64
> bit values). However it's wrong for AArch32, where the core
> dump format is an array of 64-bit Dn register values, which
> in QEMU are just in vfp.regs[n].
> 
> (See the comment in target-arm/cpu.h about VFP/Neon register
> state and different views of the register bank.)

OK, I'll fix that. I thought I tested this already, but maybe not, as
I didn't check FP registers for every test case in the matrix in the
cover letter.

> 
> > +static int
> > +arm_write_elf32_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
> > +                         int id, DumpState *s)
> > +{
> > +    struct arm_note note;
> > +    int ret;
> > +
> > +    arm_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
> > +
> > +    arm_write_vregs(note.vfp.vregs, 32, env, s);
> > +
> > +    note.vfp.fpscr = cpu_to_dump32(s, vfp_get_fpscr(env));
> 
> Do consumers care if we write out a dump with FP register
> state for a CPU which doesn't implement FP? (For AArch64
> FP is always present, but for AArch32 it may not be.)

I don't know, but it should be easy to avoid writing them when FP
isn't present, based on some status, so I'll look into doing that.

Thanks,
drew

On 3 December 2015 at 19:00, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Dec 03, 2015 at 12:27:34PM +0000, Peter Maydell wrote:
>> Do consumers care if we write out a dump with FP register
>> state for a CPU which doesn't implement FP? (For AArch64
>> FP is always present, but for AArch32 it may not be.)
>
> I don't know, but it should be easy to avoid writing them when FP
> isn't present, based on some status, so I'll look into doing that.

if (arm_feature(env, ARM_FEATURE_VFP)) is probably what you want.

thanks
-- PMM