Re: [Minios-devel] [UNIKRAFT PATCHv4 26/43] plat/kvm: Add trap handler to dump registers

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Wei,

On 06/07/18 10:03, Wei Chen wrote:
> Somtimes, for debug purpose, we would like to dump the
s/Somtimes/Sometimes/

> registers' value while exception happned. This patch add
s/happned/happened/

> a function to dump registers. Currently, we haven't enable
> the interrupt controller, so any exception is not expected.
> So any exception will cause registers dump.
>
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> ---
>   plat/common/arm/traps.c | 72 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
>   create mode 100644 plat/common/arm/traps.c
>
> diff --git a/plat/common/arm/traps.c b/plat/common/arm/traps.c
> new file mode 100644
> index 0000000..49c6813
> --- /dev/null
> +++ b/plat/common/arm/traps.c
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: ISC */
Same remark as before for SPDX.

> +/*
> + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> + *
> + * Copyright (c) 2018 Arm Ltd.
> + *
> + * Permission to use, copy, modify, and/or distribute this software
> + * for any purpose with or without fee is hereby granted, provided
> + * that the above copyright notice and this permission notice appear
> + * in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
> + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
> + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <stdint.h>
> +#include <string.h>
> +#include <uk/print.h>
> +#include <uk/assert.h>
> +
> +static const char *exception_modes[]= {
> +       "Synchronous Abort",
> +       "IRQ",
> +       "FIQ",
> +       "Error"
> +};
> +
> +static void dump_registers(struct __regs *regs, uint64_t far)
> +{
> +       uint32_t idx;
Does this need to be 32-bit? Couldn't it just be unsigned int?

> +
> +       uk_printd(DLVL_ERR, "Unikraft: Dump registers:\n");
> +       uk_printd(DLVL_ERR, "\t SP       : 0x%016lx\n", regs->sp);
> +       uk_printd(DLVL_ERR, "\t ESR_EL1  : 0x%016lx\n", regs->esr_el1);
> +       uk_printd(DLVL_ERR, "\t ELR_EL1  : 0x%016lx\n", regs->elr_el1);
> +       uk_printd(DLVL_ERR, "\t LR (x30) : 0x%016lx\n", regs->lr);
> +       uk_printd(DLVL_ERR, "\t PSTATE   : 0x%016lx\n", regs->spsr_el1);
> +       uk_printd(DLVL_ERR, "\t FAR_EL1  : 0x%016lx\n", far);
> +
> +       for (idx = 0; idx < 28; idx+=4)
s/idx+=4/idx += 4/

It would be nice to define the 28 using a macro.

> +               uk_printd(DLVL_ERR,
> +                       "\t x%02d ~ x%02d: 0x%016lx 0x%016lx 0x%016lx 
> 0x%016lx\n",
> +                       idx, idx + 3, regs->x[idx], regs->x[idx + 1],
> +                       regs->x[idx + 2], regs->x[idx + 3]);
> +
> +       uk_printd(DLVL_ERR, "\t x28 ~ x29: 0x%016lx 0x%016lx\n",
> +                               regs->x[28], regs->x[29]);
> +}
> +
> +void invalid_trap_handler(struct __regs *regs, int32_t el,
> +                               int32_t reason, uint64_t far)
I am not sure to understand why both el and reason are unsigned. They 
should never be negative.
> +{
> +       uk_printd(DLVL_ERR,  "Unikraft: EL%d invalid %s trap caught\n",
> +                               el, exception_modes[reason]);
> +       dump_registers(regs, far);
> +       UK_CRASH("PANIC\n");
> +}
> +
> +void trap_handler(struct __regs *regs, uint64_t far)
> +{ > +       uk_printd(DLVL_ERR,  "Unikraft: EL1 sync trap caught\n");
It feels like you want to name the function "trap_el1_sync" to make 
clear what the use of the function.
> +
> +       dump_registers(regs, far);
> +
> +       UK_CRASH("EXIT\n");
> +}
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel