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

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

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel