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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxxxxx>
> Sent: 2018年7月12日 19:52
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 26/43] plat/kvm: Add trap
> handler to dump registers
> 
> 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?
> 

What's the different? In my option, I want to use uniform
type format in one source file.

> > +
> > +   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.

That makes sense.

> 
> > +           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.
> 

They are int32_t : )

> > +{
> > +   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.
> 

Ok, currently, Yes. We only use it in el1_sync.

> > +
> > +   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