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

[Julien Grall <julien.grall@xxxxxxx>]

On 16/07/18 04:29, Wei Chen wrote:
> Hi Julien,
>
> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月13日 18: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
> >
> >
> >
> > 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".
> I don't know where you get the conclusion uint32_t can only be used for
> registers. I hadn't heard it before you said. I just know, for some project,
> they don't allow to use uint32_t and unsigned int in a file.
If you noticed I wrote "should" and not "cannot". It just does not make 
sense to use uint*_t for index you don't know the size. Because this is 
very subjective and technically 8-bit would have been sufficient.
Anyway, this is not a big deal.

> > > > > +
> > > > > +       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.
> I totally don't understand your comments here. At first, your asked me
> why "both el and reason are unsigned", and then I replied to you, I am
> using "int32_t" for them. Are you asking me to use uint32_t?
Well, I made a typo s/unsigned/signed/ in my first comment... So yes I 
am asking to move to uint32_t (thought 32-bit does not make much sense 
there too...).
Cheers,

--
Julien Grall

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