diff mbox

[v1,01/22] target-arm: A64: Add friendly logging of PSTATE A and I flags

Message ID 1399356506-5609-2-git-send-email-edgar.iglesias@gmail.com
State New
Headers show

Commit Message

Edgar E. Iglesias May 6, 2014, 6:08 a.m. UTC
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/translate-a64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Peter Crosthwaite May 7, 2014, 5:32 a.m. UTC | #1
On Tue, May 6, 2014 at 4:08 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/translate-a64.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index b62db4d..4f8246f 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -137,8 +137,10 @@ void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>              cpu_fprintf(f, " ");
>          }
>      }
> -    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c)\n",
> +    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c%c%c)\n",

Should delimit (just a space I think) between DAIF and NZCV
components. ARM ARM repeatedly refers to these two groups of four as
single item suggesting they are two logical groupings of 4 bits each.

>                  psr,
> +                psr & PSTATE_A ? 'A' : '-',
> +                psr & PSTATE_I ? 'I' : '-',

And should the full DAIF be added for completeness?

Regards,
Peter

>                  psr & PSTATE_N ? 'N' : '-',
>                  psr & PSTATE_Z ? 'Z' : '-',
>                  psr & PSTATE_C ? 'C' : '-',
> --
> 1.8.3.2
>
>
Peter Maydell May 7, 2014, 8:50 a.m. UTC | #2
On 6 May 2014 07:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/translate-a64.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index b62db4d..4f8246f 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -137,8 +137,10 @@ void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>              cpu_fprintf(f, " ");
>          }
>      }
> -    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c)\n",
> +    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c%c%c)\n",
>                  psr,
> +                psr & PSTATE_A ? 'A' : '-',
> +                psr & PSTATE_I ? 'I' : '-',
>                  psr & PSTATE_N ? 'N' : '-',
>                  psr & PSTATE_Z ? 'Z' : '-',
>                  psr & PSTATE_C ? 'C' : '-',

Why A and I ? In particular in QEMU the A bit is always zero
because we don't do System Errors (aka asynchronous
external aborts), and it's weird to show I but not F. The
idea of splitting out NZCV is really that (as with the A32/T32
state dump) they're the most useful bits for immediately
figuring out code flow); anything else you can fish out of
the hex value by hand if you really need it. I think you can
make a case for "decode only a small set of key bits" or
for "completely decode the whole register", but I'm not
sure adding only two more bits makes sense.

thanks
-- PMM
Edgar E. Iglesias May 8, 2014, 12:08 a.m. UTC | #3
On Wed, May 07, 2014 at 09:50:27AM +0100, Peter Maydell wrote:
> On 6 May 2014 07:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/translate-a64.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> > index b62db4d..4f8246f 100644
> > --- a/target-arm/translate-a64.c
> > +++ b/target-arm/translate-a64.c
> > @@ -137,8 +137,10 @@ void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
> >              cpu_fprintf(f, " ");
> >          }
> >      }
> > -    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c)\n",
> > +    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c%c%c)\n",
> >                  psr,
> > +                psr & PSTATE_A ? 'A' : '-',
> > +                psr & PSTATE_I ? 'I' : '-',
> >                  psr & PSTATE_N ? 'N' : '-',
> >                  psr & PSTATE_Z ? 'Z' : '-',
> >                  psr & PSTATE_C ? 'C' : '-',
> 
> Why A and I ? In particular in QEMU the A bit is always zero
> because we don't do System Errors (aka asynchronous
> external aborts), and it's weird to show I but not F. The
> idea of splitting out NZCV is really that (as with the A32/T32
> state dump) they're the most useful bits for immediately
> figuring out code flow); anything else you can fish out of
> the hex value by hand if you really need it. I think you can
> make a case for "decode only a small set of key bits" or
> for "completely decode the whole register", but I'm not
> sure adding only two more bits makes sense.

Hi,

TBH I didn't give this much thought. I used the I flag while
debugging some virq stuff and probably added A while stumbling
in the dark at some point..

For v2 I've added the DAIF flags with delimiter if thats not good
enough I suggest removing the patch, it's not very important.

Cheers,
Edgar
diff mbox

Patch

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index b62db4d..4f8246f 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -137,8 +137,10 @@  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
             cpu_fprintf(f, " ");
         }
     }
-    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c)\n",
+    cpu_fprintf(f, "PSTATE=%08x (flags %c%c%c%c%c%c)\n",
                 psr,
+                psr & PSTATE_A ? 'A' : '-',
+                psr & PSTATE_I ? 'I' : '-',
                 psr & PSTATE_N ? 'N' : '-',
                 psr & PSTATE_Z ? 'Z' : '-',
                 psr & PSTATE_C ? 'C' : '-',