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月16日 18:57
> 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 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.
> 

Ok, that makes sense.

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

Ok.

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