[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.