diff mbox

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

Message ID 1448411877-22019-7-git-send-email-drjones@redhat.com
State New
Headers show

Commit Message

Andrew Jones Nov. 25, 2015, 12:37 a.m. UTC 
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(-)

-- 
2.4.3

Comments

Peter Maydell Dec. 3, 2015, 12:27 p.m. UTC | #1 
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
Andrew Jones Dec. 3, 2015, 7 p.m. UTC | #2 
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
Peter Maydell Dec. 3, 2015, 7:55 p.m. UTC | #3 
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
diff mbox

Patch

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 */
+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;
+        }
+    }
+}
+
+static int
+aarch64_write_elf64_fpregset(WriteCoreDumpFunction f, CPUARMState *env,
+                             int id, DumpState *s)
+{
+    struct aarch64_note note;
+    int ret;
+
+    aarch64_note_init(&note, s, NT_FPREGSET, sizeof(note.vfp));
+
+    if (is_a64(env)) {
+        arm_write_vregs(note.vfp.vregs, 64, env, s);
+    } else {
+        arm_write_vregs(note.vfp.vregs, 32, env, s);
+    }
+
+    note.vfp.fpsr = cpu_to_dump32(s, vfp_get_fpsr(env));
+    note.vfp.fpcr = cpu_to_dump32(s, vfp_get_fpcr(env));
+
+    ret = f(&note, AARCH64_FPREGSET_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
 
 static int
 aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
@@ -63,13 +140,8 @@  aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
     struct aarch64_note note;
     int ret, i;
 
-    memset(&note, 0, sizeof(note));
-
-    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
-    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
-    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+    aarch64_note_init(&note, s, NT_PRSTATUS, sizeof(note.prstatus));
 
-    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
     note.prstatus.pr_pid = cpu_to_dump32(s, id);
 
     if (is_a64(env)) {
@@ -89,12 +161,12 @@  aarch64_write_elf64_note(WriteCoreDumpFunction f, CPUARMState *env,
         note.prstatus.pr_reg.pstate = cpu_to_dump64(s, cpsr_read(env));
     }
 
-    ret = f(&note, sizeof(note), s);
+    ret = f(&note, AARCH64_PRSTATUS_NOTE_SIZE, s);
     if (ret < 0) {
         return -1;
     }
 
-    return 0;
+    return aarch64_write_elf64_fpregset(f, env, id, s);
 }
 
 /* struct pt_regs from arch/arm/include/asm/ptrace.h */
@@ -105,6 +177,14 @@  struct arm_user_regs {
 
 QEMU_BUILD_BUG_ON(sizeof(struct arm_user_regs) != 72);
 
+/* struct user_vfp from arch/arm/include/asm/user.h */
+struct arm_user_vfp_state {
+    uint64_t vregs[32];
+    uint32_t fpscr;
+} QEMU_PACKED;
+
+QEMU_BUILD_BUG_ON(sizeof(struct arm_user_vfp_state) != 260);
+
 /* struct elf_prstatus from include/uapi/linux/elfcore.h */
 struct arm_elf_prstatus {
     char pad1[24]; /* 24 == offsetof(struct elf_prstatus, pr_pid) */
@@ -120,10 +200,50 @@  QEMU_BUILD_BUG_ON(sizeof(struct arm_elf_prstatus) != 148);
 struct arm_note {
     Elf32_Nhdr hdr;
     char name[QEMU_ALIGN_UP(NOTE_NAMESZ, 4)];
-    struct arm_elf_prstatus prstatus;
+    union {
+        struct arm_elf_prstatus prstatus;
+        struct arm_user_vfp_state vfp;
+    };
 } QEMU_PACKED;
 
-QEMU_BUILD_BUG_ON(sizeof(struct arm_note) != 168);
+#define ARM_NOTE_HEADER_SIZE offsetof(struct arm_note, prstatus)
+#define ARM_PRSTATUS_NOTE_SIZE \
+            (ARM_NOTE_HEADER_SIZE + sizeof(struct arm_elf_prstatus))
+#define ARM_FPREGSET_NOTE_SIZE \
+            (ARM_NOTE_HEADER_SIZE + sizeof(struct arm_user_vfp_state))
+
+static void arm_note_init(struct arm_note *note, DumpState *s,
+                          Elf32_Word type, Elf32_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 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));
+
+    ret = f(&note, ARM_FPREGSET_NOTE_SIZE, s);
+    if (ret < 0) {
+        return -1;
+    }
+
+    return 0;
+}
 
 static int
 arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
@@ -132,13 +252,8 @@  arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
     struct arm_note note;
     int ret, i;
 
-    memset(&note, 0, sizeof(note));
-
-    note.hdr.n_namesz = cpu_to_dump32(s, NOTE_NAMESZ);
-    note.hdr.n_descsz = cpu_to_dump32(s, sizeof(note.prstatus));
-    note.hdr.n_type = cpu_to_dump32(s, NT_PRSTATUS);
+    arm_note_init(&note, s, NT_PRSTATUS, sizeof(note.prstatus));
 
-    memcpy(note.name, NOTE_NAME, NOTE_NAMESZ);
     note.prstatus.pr_pid = cpu_to_dump32(s, id);
 
     for (i = 0; i < 16; ++i) {
@@ -146,12 +261,12 @@  arm_write_elf32_note(WriteCoreDumpFunction f, CPUARMState *env,
     }
     note.prstatus.pr_reg.regs[16] = cpu_to_dump32(s, cpsr_read(env));
 
-    ret = f(&note, sizeof(note), s);
+    ret = f(&note, ARM_PRSTATUS_NOTE_SIZE, s);
     if (ret < 0) {
         return -1;
     }
 
-    return 0;
+    return arm_write_elf32_fpregset(f, env, id, s);
 }
 
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
@@ -221,9 +336,11 @@  ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
     size_t note_size;
 
     if (class == ELFCLASS64) {
-        note_size = sizeof(struct aarch64_note);
+        note_size = AARCH64_PRSTATUS_NOTE_SIZE;
+        note_size += AARCH64_FPREGSET_NOTE_SIZE;
     } else {
-        note_size = sizeof(struct arm_note);
+        note_size = ARM_PRSTATUS_NOTE_SIZE;
+        note_size += ARM_FPREGSET_NOTE_SIZE;
     }
 
     return note_size * nr_cpus;