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

[Julien Grall <julien.grall@xxxxxxx>]

On 13/07/18 11:15, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----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.
Then I could ask, why do you specifically use 32-bit and not 8-bit... 
*-bit should only be used to describe registers. The rest could deal 
with "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.
> 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 : )
How come the EL can be negative? The EL will be 0, 1, 2, 3. We don't 
care about the last 2.
Same question for negative.

Cheers,

--
Julien Grall

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