Re: [PATCH v5] cpukit/aarch64: Add ESR register decoding
On 23/03/2021 18:42, Joel Sherrill wrote:
> > + mask = 1 << (num_bits - 1);
> > +
> > + while ( mask != 0 ) {
> > + _IO_Printf( _CPU_Put_char, &sctx, "%d", (value & mask ? 1 : 0) );
> > + mask >>= 1;
> > + }
The _IO_Vprintf() is a port from the FreeBSD kvprintf(). I removed the
support for this:
/*
* Scaled down version of printf(3).
*
* Two additional formats:
*
* The format %b is supported to decode error registers.
* Its usage is:
*
* printf("reg=%b\n", regval, "*");
*
* where is the output base expressed as a control character, e.g.
* \10 gives octal; \20 gives hex. Each arg is a sequence of characters,
* the first of which gives the bit number to be inspected (origin 1), and
* the next characters (up to a control character, i.e. a character <= 32),
* give the name of the register. Thus:
*
* kvprintf("reg=%b\n", 3, "\10\2BITTWO\1BITONE");
We could also a a special %B specifier to produce 0b0100 stuff.
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v5] cpukit/aarch64: Add ESR register decoding
On Tue, Mar 23, 2021, 12:26 PM Alex White wrote:
> On Tue, Mar 23, 2021 at 11:46 AM Gedare Bloom wrote:
> >
> > On Tue, Mar 23, 2021 at 10:31 AM Alex White
> wrote:
> > >
> > > ---
> > > .../aarch64/aarch64-exception-frame-print.c | 133 --
> > > 1 file changed, 123 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > index 59b5d06032..a8c492b1da 100644
> > > --- a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > +++ b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > @@ -45,10 +45,111 @@
> > > #include
> > >
> > > #include
> > > +#include
> > > #include
> > >
> > > +typedef struct {
> > > + char *s;
> > > + size_t n;
> > > +} String_Context;
> > > +
> > > +static void _CPU_Put_char( int c, void *arg )
> > > +{
> > > + String_Context *sctx = arg;
> > > + size_t n = sctx->n;
> > > +
> > > + if (n > 0) {
> > > +char *s = sctx->s;
> > > +*s = (char) c;
> > > +sctx->s = s + 1;
> > > +sctx->n = n - 1;
> > > + }
> > > +}
> > > +
> > > +static void _CPU_Binary_sprintf(
> > > + char *s,
> > > + size_t maxlen,
> > > + uint32_t num_bits,
> > > + uint32_t value
> > > +)
> > > +{
> > > + String_Context sctx;
> > > + uint32_t mask;
> > > +
> > > + sctx.s = s;
> > > + sctx.n = maxlen;
> > > +
> > > + mask = 1 << (num_bits - 1);
> > > +
> > > + while ( mask != 0 ) {
> > > +_IO_Printf( _CPU_Put_char, &sctx, "%d", (value & mask ? 1 : 0) );
> > > +mask >>= 1;
> > > + }
> >
> > should this function default to NUL-terminate s?
> >
> > I'm not totally familiar with the usage, but usually that is the
> > expectation of an sprintf() kind of function.
>
> I decided that the function should not NULL-terminate in this case
> because it is only used in two places, and I zero-initialize the
> character buffers before both calls.
>
> I realize that NULL-termination would make the function more generally
> useful, but I only wrote it as a quick static helper for this file.
>
> Is it worth NULL-terminating to avoid a possible bug being introduced
> in the future if someone assumes that it is NULL-terminating?
>
Yeah. I've been around long enough where not null terminating it will
actually cause someone a problem. And you should never have to zero
something out if you fill it out properly. :)
> >
> > > +}
> > > +
> > > +static const char* _CPU_Exception_class_to_string( uint16_t
> exception_class )
> > > +{
> > > + switch ( exception_class ) {
> > > +case 0x01:
> > > + return "Trapped WFI or WFE instruction";
> > > +case 0x03:
> > > + return "Trapped MCR or MRC access with (coproc==0b)";
> > > +case 0x04:
> > > + return "Trapped MCRR or MRRC access with (coproc==0b)";
> > > +case 0x05:
> > > + return "Trapped MCR or MRC access with (coproc==0b1110)";
> > > +case 0x06:
> > > + return "Trapped LDC or STC access";
> > > +case 0x0c:
> > > + return "Trapped MRRC access with (coproc==0b1110)";
> > > +case 0x0e:
> > > + return "Illegal Execution state";
> > > +case 0x18:
> > > + return "Trapped MSR, MRS, or System instruction";
> > > +case 0x20:
> > > + return "Instruction Abort from a lower Exception level";
> > > +case 0x21:
> > > + return "Instruction Abort taken without a change in Exception
> level";
> > > +case 0x22:
> > > + return "PC alignment fault";
> > > +case 0x24:
> > > + return "Data Abort from a lower Exception level";
> > > +case 0x25:
> > > + return "Data Abort taken without a change in Exception level";
> > > +case 0x26:
> > > + return "SP alignment fault";
> > > +case 0x30:
> > > + return "Breakpoint exception from a lower Exception level";
> > > +case 0x31:
> > > + return "Breakpoint exception taken without a change in
> Exception level";
> > > +case 0x32:
> > > + return "Software Step exception from a lower Exception level";
> > > +case 0x33:
> > > + return "Software Step exception taken without a change in
> Exception "
> > > + "level";
> >
> > This is a case where I would say allowing the string to spill over the
> > 80c is better. I think that having a long string (say 40+ characters)
> > can justify violating this 80c limit. It would be best for someone to
> > agree with me though :)
>
> I agree that it would look better if I allowed this line to exceed the
> 80 character limit. Those darn coding conventions...
>
> Hopefully others agree to an exception here.
>
I agree. Put a comment above the switch that it is deliberately violated.
Seeing the values in makes it clear that there are categories to these
values based on the first nibble. I don't know that there's any value to
taking advantage of that but the pattern is there. Perhaps if more the
message can be common but only if the first nibble is used
RE: [PATCH v5] cpukit/aarch64: Add ESR register decoding
On Tue, Mar 23, 2021 at 11:46 AM Gedare Bloom wrote:
>
> On Tue, Mar 23, 2021 at 10:31 AM Alex White wrote:
> >
> > ---
> > .../aarch64/aarch64-exception-frame-print.c | 133 --
> > 1 file changed, 123 insertions(+), 10 deletions(-)
> >
> > diff --git a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > index 59b5d06032..a8c492b1da 100644
> > --- a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > +++ b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > @@ -45,10 +45,111 @@
> > #include
> >
> > #include
> > +#include
> > #include
> >
> > +typedef struct {
> > + char *s;
> > + size_t n;
> > +} String_Context;
> > +
> > +static void _CPU_Put_char( int c, void *arg )
> > +{
> > + String_Context *sctx = arg;
> > + size_t n = sctx->n;
> > +
> > + if (n > 0) {
> > + char *s = sctx->s;
> > + *s = (char) c;
> > + sctx->s = s + 1;
> > + sctx->n = n - 1;
> > + }
> > +}
> > +
> > +static void _CPU_Binary_sprintf(
> > + char *s,
> > + size_t maxlen,
> > + uint32_t num_bits,
> > + uint32_t value
> > +)
> > +{
> > + String_Context sctx;
> > + uint32_t mask;
> > +
> > + sctx.s = s;
> > + sctx.n = maxlen;
> > +
> > + mask = 1 << (num_bits - 1);
> > +
> > + while ( mask != 0 ) {
> > + _IO_Printf( _CPU_Put_char, &sctx, "%d", (value & mask ? 1 : 0) );
> > + mask >>= 1;
> > + }
>
> should this function default to NUL-terminate s?
>
> I'm not totally familiar with the usage, but usually that is the
> expectation of an sprintf() kind of function.
I decided that the function should not NULL-terminate in this case
because it is only used in two places, and I zero-initialize the
character buffers before both calls.
I realize that NULL-termination would make the function more generally
useful, but I only wrote it as a quick static helper for this file.
Is it worth NULL-terminating to avoid a possible bug being introduced
in the future if someone assumes that it is NULL-terminating?
>
> > +}
> > +
> > +static const char* _CPU_Exception_class_to_string( uint16_t
> > exception_class )
> > +{
> > + switch ( exception_class ) {
> > + case 0x01:
> > + return "Trapped WFI or WFE instruction";
> > + case 0x03:
> > + return "Trapped MCR or MRC access with (coproc==0b)";
> > + case 0x04:
> > + return "Trapped MCRR or MRRC access with (coproc==0b)";
> > + case 0x05:
> > + return "Trapped MCR or MRC access with (coproc==0b1110)";
> > + case 0x06:
> > + return "Trapped LDC or STC access";
> > + case 0x0c:
> > + return "Trapped MRRC access with (coproc==0b1110)";
> > + case 0x0e:
> > + return "Illegal Execution state";
> > + case 0x18:
> > + return "Trapped MSR, MRS, or System instruction";
> > + case 0x20:
> > + return "Instruction Abort from a lower Exception level";
> > + case 0x21:
> > + return "Instruction Abort taken without a change in Exception level";
> > + case 0x22:
> > + return "PC alignment fault";
> > + case 0x24:
> > + return "Data Abort from a lower Exception level";
> > + case 0x25:
> > + return "Data Abort taken without a change in Exception level";
> > + case 0x26:
> > + return "SP alignment fault";
> > + case 0x30:
> > + return "Breakpoint exception from a lower Exception level";
> > + case 0x31:
> > + return "Breakpoint exception taken without a change in Exception
> > level";
> > + case 0x32:
> > + return "Software Step exception from a lower Exception level";
> > + case 0x33:
> > + return "Software Step exception taken without a change in Exception "
> > + "level";
>
> This is a case where I would say allowing the string to spill over the
> 80c is better. I think that having a long string (say 40+ characters)
> can justify violating this 80c limit. It would be best for someone to
> agree with me though :)
I agree that it would look better if I allowed this line to exceed the
80 character limit. Those darn coding conventions...
Hopefully others agree to an exception here.
>
> Address the NUL termination, and decide how you want to proceed with
> the line length for string breaks. It's a gray area in the rules.
>
> > + case 0x34:
> > + return "Watchpoint exception from a lower Exception level";
> > + case 0x35:
> > + return "Watchpoint exception taken without a change in Exception
> > level";
> > + case 0x38:
> > + return "BKPT instruction execution in AArch32 state";
> > + case 0x3c:
> > + return "BRK instruction execution in AArch64 state";
> > + default:
> > + return "Unknown";
> > + }
> > +}
> > +
> > void _CPU_Exception_frame_print( const CPU_Exception_frame *frame )
> > {
> > + uint32_t ec;
> > + uint32_t il;
> > + uint32_t iss;
> > + char ec_str[7] = { 0 };
> > + char iss_str[26] = { 0 };
> > + int i;
> > + const uint12
Re: [PATCH v5] cpukit/aarch64: Add ESR register decoding
On Tue, Mar 23, 2021 at 10:31 AM Alex White wrote:
>
> ---
> .../aarch64/aarch64-exception-frame-print.c | 133 --
> 1 file changed, 123 insertions(+), 10 deletions(-)
>
> diff --git a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> index 59b5d06032..a8c492b1da 100644
> --- a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> +++ b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> @@ -45,10 +45,111 @@
> #include
>
> #include
> +#include
> #include
>
> +typedef struct {
> + char *s;
> + size_t n;
> +} String_Context;
> +
> +static void _CPU_Put_char( int c, void *arg )
> +{
> + String_Context *sctx = arg;
> + size_t n = sctx->n;
> +
> + if (n > 0) {
> +char *s = sctx->s;
> +*s = (char) c;
> +sctx->s = s + 1;
> +sctx->n = n - 1;
> + }
> +}
> +
> +static void _CPU_Binary_sprintf(
> + char *s,
> + size_t maxlen,
> + uint32_t num_bits,
> + uint32_t value
> +)
> +{
> + String_Context sctx;
> + uint32_t mask;
> +
> + sctx.s = s;
> + sctx.n = maxlen;
> +
> + mask = 1 << (num_bits - 1);
> +
> + while ( mask != 0 ) {
> +_IO_Printf( _CPU_Put_char, &sctx, "%d", (value & mask ? 1 : 0) );
> +mask >>= 1;
> + }
should this function default to NUL-terminate s?
I'm not totally familiar with the usage, but usually that is the
expectation of an sprintf() kind of function.
> +}
> +
> +static const char* _CPU_Exception_class_to_string( uint16_t exception_class )
> +{
> + switch ( exception_class ) {
> +case 0x01:
> + return "Trapped WFI or WFE instruction";
> +case 0x03:
> + return "Trapped MCR or MRC access with (coproc==0b)";
> +case 0x04:
> + return "Trapped MCRR or MRRC access with (coproc==0b)";
> +case 0x05:
> + return "Trapped MCR or MRC access with (coproc==0b1110)";
> +case 0x06:
> + return "Trapped LDC or STC access";
> +case 0x0c:
> + return "Trapped MRRC access with (coproc==0b1110)";
> +case 0x0e:
> + return "Illegal Execution state";
> +case 0x18:
> + return "Trapped MSR, MRS, or System instruction";
> +case 0x20:
> + return "Instruction Abort from a lower Exception level";
> +case 0x21:
> + return "Instruction Abort taken without a change in Exception level";
> +case 0x22:
> + return "PC alignment fault";
> +case 0x24:
> + return "Data Abort from a lower Exception level";
> +case 0x25:
> + return "Data Abort taken without a change in Exception level";
> +case 0x26:
> + return "SP alignment fault";
> +case 0x30:
> + return "Breakpoint exception from a lower Exception level";
> +case 0x31:
> + return "Breakpoint exception taken without a change in Exception
> level";
> +case 0x32:
> + return "Software Step exception from a lower Exception level";
> +case 0x33:
> + return "Software Step exception taken without a change in Exception "
> + "level";
This is a case where I would say allowing the string to spill over the
80c is better. I think that having a long string (say 40+ characters)
can justify violating this 80c limit. It would be best for someone to
agree with me though :)
Address the NUL termination, and decide how you want to proceed with
the line length for string breaks. It's a gray area in the rules.
> +case 0x34:
> + return "Watchpoint exception from a lower Exception level";
> +case 0x35:
> + return "Watchpoint exception taken without a change in Exception
> level";
> +case 0x38:
> + return "BKPT instruction execution in AArch32 state";
> +case 0x3c:
> + return "BRK instruction execution in AArch64 state";
> +default:
> + return "Unknown";
> + }
> +}
> +
> void _CPU_Exception_frame_print( const CPU_Exception_frame *frame )
> {
> + uint32_t ec;
> + uint32_t il;
> + uint32_t iss;
> + char ec_str[7] = { 0 };
> + char iss_str[26] = { 0 };
> + int i;
> + const uint128_t *qx;
> +
>printk(
> "\n"
> "X0 = 0x%016" PRIx64 " X17 = 0x%016" PRIx64 "\n"
> @@ -68,8 +169,7 @@ void _CPU_Exception_frame_print( const CPU_Exception_frame
> *frame )
> "X14 = 0x%016" PRIx64 " SP = 0x%016" PRIxPTR "\n"
> "X15 = 0x%016" PRIx64 " PC = 0x%016" PRIxPTR "\n"
> "X16 = 0x%016" PRIx64 " DAIF = 0x%016" PRIx64 "\n"
> -"VEC = 0x%016" PRIxPTR " CPSR = 0x%016" PRIx64 "\n"
> -"ESR = 0x%016" PRIx64 " FAR = 0x%016" PRIx64 "\n",
> +"VEC = 0x%016" PRIxPTR " CPSR = 0x%016" PRIx64 "\n",
> frame->register_x0, frame->register_x17,
> frame->register_x1, frame->register_x18,
> frame->register_x2, frame->register_x19,
> @@ -83,23 +183,36 @@ void _CPU_Exception_frame_print( const
> CPU_Exception_frame *frame )
> frame->register_x10, frame->register_x27,
> frame->register_x11, frame->register_x28,
> frame->register_x12, frame->register_fp,
> -
