* [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff
2023-02-24 11:35 [PATCH v4 0/5] RISCV basic exception handling implementation Oleksii Kurochko
@ 2023-02-24 11:35 ` Oleksii Kurochko
2023-02-27 12:48 ` Jan Beulich
2023-02-24 11:35 ` [PATCH v4 2/5] xen/riscv: introduce trap_init() Oleksii Kurochko
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:35 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bob Eshleman, Alistair Francis,
Connor Davis
The patch introduces stuff needed to decode a reason of an
exception.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
- fix string in decode_reserved_interrupt_cause()
---
Changes in V3:
- Nothing changed
---
Changes in V2:
- Make decode_trap_cause() more optimization friendly.
- Merge the pathc which introduces do_unexpected_trap() to the current one.
---
xen/arch/riscv/traps.c | 87 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 86 insertions(+), 1 deletion(-)
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ccd3593f5a..29b1a0dfae 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -4,10 +4,95 @@
*
* RISC-V Trap handlers
*/
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+
+#include <asm/csr.h>
+#include <asm/early_printk.h>
#include <asm/processor.h>
#include <asm/traps.h>
-void do_trap(struct cpu_user_regs *cpu_regs)
+static const char *decode_trap_cause(unsigned long cause)
+{
+ static const char *const trap_causes[] = {
+ [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned",
+ [CAUSE_FETCH_ACCESS] = "Instruction Access Fault",
+ [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction",
+ [CAUSE_BREAKPOINT] = "Breakpoint",
+ [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned",
+ [CAUSE_LOAD_ACCESS] = "Load Access Fault",
+ [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned",
+ [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault",
+ [CAUSE_USER_ECALL] = "Environment Call from U-Mode",
+ [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode",
+ [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode",
+ [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault",
+ [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault",
+ [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault",
+ [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault",
+ [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault",
+ [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault",
+ [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
+ };
+
+ if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
+ return trap_causes[cause];
+ return "UNKNOWN";
+}
+
+const char *decode_reserved_interrupt_cause(unsigned long irq_cause)
+{
+ switch ( irq_cause )
+ {
+ case IRQ_M_SOFT:
+ return "M-mode Software Interrupt";
+ case IRQ_M_TIMER:
+ return "M-mode TIMER Interrupt";
+ case IRQ_M_EXT:
+ return "M-mode External Interrupt";
+ default:
+ return "UNKNOWN IRQ type";
+ }
+}
+
+const char *decode_interrupt_cause(unsigned long cause)
+{
+ unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG;
+
+ switch ( irq_cause )
+ {
+ case IRQ_S_SOFT:
+ return "Supervisor Software Interrupt";
+ case IRQ_S_TIMER:
+ return "Supervisor Timer Interrupt";
+ case IRQ_S_EXT:
+ return "Supervisor External Interrupt";
+ default:
+ return decode_reserved_interrupt_cause(irq_cause);
+ }
+}
+
+const char *decode_cause(unsigned long cause)
+{
+ if ( cause & CAUSE_IRQ_FLAG )
+ return decode_interrupt_cause(cause);
+
+ return decode_trap_cause(cause);
+}
+
+static void do_unexpected_trap(const struct cpu_user_regs *regs)
{
+ unsigned long cause = csr_read(CSR_SCAUSE);
+
+ early_printk("Unhandled exception: ");
+ early_printk(decode_cause(cause));
+ early_printk("\n");
+
die();
}
+
+void do_trap(struct cpu_user_regs *cpu_regs)
+{
+ do_unexpected_trap(cpu_regs);
+}
--
2.39.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff
2023-02-24 11:35 ` [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff Oleksii Kurochko
@ 2023-02-27 12:48 ` Jan Beulich
2023-03-01 9:07 ` Oleksii
2023-03-01 9:41 ` Oleksii
0 siblings, 2 replies; 16+ messages in thread
From: Jan Beulich @ 2023-02-27 12:48 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On 24.02.2023 12:35, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -4,10 +4,95 @@
> *
> * RISC-V Trap handlers
> */
> +
> +#include <xen/errno.h>
I couldn't spot anything in the file which would require this inclusion.
> +#include <xen/lib.h>
> +
> +#include <asm/csr.h>
> +#include <asm/early_printk.h>
> #include <asm/processor.h>
> #include <asm/traps.h>
>
> -void do_trap(struct cpu_user_regs *cpu_regs)
> +static const char *decode_trap_cause(unsigned long cause)
> +{
> + static const char *const trap_causes[] = {
> + [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned",
> + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault",
> + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction",
> + [CAUSE_BREAKPOINT] = "Breakpoint",
> + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned",
> + [CAUSE_LOAD_ACCESS] = "Load Access Fault",
> + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned",
> + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault",
> + [CAUSE_USER_ECALL] = "Environment Call from U-Mode",
> + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode",
> + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode",
> + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault",
> + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault",
> + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault",
> + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault",
> + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault",
> + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault",
> + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
> + };
> +
> + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
> + return trap_causes[cause];
> + return "UNKNOWN";
> +}
> +
> +const char *decode_reserved_interrupt_cause(unsigned long irq_cause)
For any non-static function that you add you will need a declaration
in a header, which the defining C file then includes. I understand
that during initial bringup functions without (external) callers may
want to (temporarily) exist, but briefly clarifying what the future
expectation regarding external uses might help. Any function that's
not expected to gain external callers should be static.
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff
2023-02-27 12:48 ` Jan Beulich
@ 2023-03-01 9:07 ` Oleksii
2023-03-01 9:41 ` Oleksii
1 sibling, 0 replies; 16+ messages in thread
From: Oleksii @ 2023-03-01 9:07 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -4,10 +4,95 @@
> > *
> > * RISC-V Trap handlers
> > */
> > +
> > +#include <xen/errno.h>
>
> I couldn't spot anything in the file which would require this
> inclusion.
Sure. It can be removed.
>
> > +#include <xen/lib.h>
> > +
> > +#include <asm/csr.h>
> > +#include <asm/early_printk.h>
> > #include <asm/processor.h>
> > #include <asm/traps.h>
> >
> > -void do_trap(struct cpu_user_regs *cpu_regs)
> > +static const char *decode_trap_cause(unsigned long cause)
> > +{
> > + static const char *const trap_causes[] = {
> > + [CAUSE_MISALIGNED_FETCH] = "Instruction Address
> > Misaligned",
> > + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault",
> > + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction",
> > + [CAUSE_BREAKPOINT] = "Breakpoint",
> > + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned",
> > + [CAUSE_LOAD_ACCESS] = "Load Access Fault",
> > + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned",
> > + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault",
> > + [CAUSE_USER_ECALL] = "Environment Call from U-Mode",
> > + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode",
> > + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode",
> > + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault",
> > + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault",
> > + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault",
> > + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page
> > Fault",
> > + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault",
> > + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction
> > Fault",
> > + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page
> > Fault",
> > + };
> > +
> > + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
> > + return trap_causes[cause];
> > + return "UNKNOWN";
> > +}
> > +
> > +const char *decode_reserved_interrupt_cause(unsigned long
> > irq_cause)
>
> For any non-static function that you add you will need a declaration
> in a header, which the defining C file then includes. I understand
> that during initial bringup functions without (external) callers may
> want to (temporarily) exist, but briefly clarifying what the future
> expectation regarding external uses might help. Any function that's
> not expected to gain external callers should be static.
Thanks. For explanation. I'll make them static.
~ Oleksii
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff
2023-02-27 12:48 ` Jan Beulich
2023-03-01 9:07 ` Oleksii
@ 2023-03-01 9:41 ` Oleksii
2023-03-01 9:45 ` Oleksii
1 sibling, 1 reply; 16+ messages in thread
From: Oleksii @ 2023-03-01 9:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -4,10 +4,95 @@
> > *
> > * RISC-V Trap handlers
> > */
> > +
> > +#include <xen/errno.h>
>
> I couldn't spot anything in the file which would require this
> inclusion.
-EINVAL, -ENOENT are used in do_bug_frame() I missed that when wrote
previous answer on e-mail.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff
2023-03-01 9:41 ` Oleksii
@ 2023-03-01 9:45 ` Oleksii
0 siblings, 0 replies; 16+ messages in thread
From: Oleksii @ 2023-03-01 9:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On Wed, 2023-03-01 at 11:41 +0200, Oleksii wrote:
> On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> > On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > > --- a/xen/arch/riscv/traps.c
> > > +++ b/xen/arch/riscv/traps.c
> > > @@ -4,10 +4,95 @@
> > > *
> > > * RISC-V Trap handlers
> > > */
> > > +
> > > +#include <xen/errno.h>
> >
> > I couldn't spot anything in the file which would require this
> > inclusion.
> -EINVAL, -ENOENT are used in do_bug_frame() I missed that when wrote
> previous answer on e-mail.
Sorry for confusion. I resolved merge conflicts after rebase so
<xen/errno.h> should be remove as anything is used in traps.c from it
~ Oleksii
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/5] xen/riscv: introduce trap_init()
2023-02-24 11:35 [PATCH v4 0/5] RISCV basic exception handling implementation Oleksii Kurochko
2023-02-24 11:35 ` [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff Oleksii Kurochko
@ 2023-02-24 11:35 ` Oleksii Kurochko
2023-02-27 12:50 ` Jan Beulich
2023-02-24 11:35 ` [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h> Oleksii Kurochko
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:35 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bob Eshleman, Alistair Francis,
Connor Davis
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V4:
- Nothing changed
---
Changes in V3:
- Nothing changed
---
Changes in V2:
- Rename setup_trap_handler() to trap_init().
- Add Reviewed-by to the commit message.
---
xen/arch/riscv/include/asm/traps.h | 1 +
xen/arch/riscv/setup.c | 4 ++++
xen/arch/riscv/traps.c | 7 +++++++
3 files changed, 12 insertions(+)
diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index f3fb6b25d1..f1879294ef 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -7,6 +7,7 @@
void do_trap(struct cpu_user_regs *cpu_regs);
void handle_trap(void);
+void trap_init(void);
#endif /* __ASSEMBLY__ */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index d09ffe1454..c8513ca4f8 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,7 +1,9 @@
#include <xen/compile.h>
#include <xen/init.h>
+#include <asm/csr.h>
#include <asm/early_printk.h>
+#include <asm/traps.h>
/* Xen stack for bringing up the first CPU. */
unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
@@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
{
early_printk("Hello from C env\n");
+ trap_init();
+
for ( ;; )
asm volatile ("wfi");
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 29b1a0dfae..ad7311f269 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -13,6 +13,13 @@
#include <asm/processor.h>
#include <asm/traps.h>
+void trap_init(void)
+{
+ unsigned long addr = (unsigned long)&handle_trap;
+
+ csr_write(CSR_STVEC, addr);
+}
+
static const char *decode_trap_cause(unsigned long cause)
{
static const char *const trap_causes[] = {
--
2.39.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()
2023-02-24 11:35 ` [PATCH v4 2/5] xen/riscv: introduce trap_init() Oleksii Kurochko
@ 2023-02-27 12:50 ` Jan Beulich
2023-03-01 10:34 ` Oleksii
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2023-02-27 12:50 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On 24.02.2023 12:35, Oleksii Kurochko wrote:
> @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
> {
> early_printk("Hello from C env\n");
>
> + trap_init();
> +
> for ( ;; )
> asm volatile ("wfi");
Along the lines of what Andrew has said there - it's hard to see
how this can come (both code flow and patch ordering wise) ahead
of clearing .bss.
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()
2023-02-27 12:50 ` Jan Beulich
@ 2023-03-01 10:34 ` Oleksii
2023-03-01 10:44 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Oleksii @ 2023-03-01 10:34 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On Mon, 2023-02-27 at 13:50 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
> > {
> > early_printk("Hello from C env\n");
> >
> > + trap_init();
> > +
> > for ( ;; )
> > asm volatile ("wfi");
>
> Along the lines of what Andrew has said there - it's hard to see
> how this can come (both code flow and patch ordering wise) ahead
> of clearing .bss.
So should I add the patch with initializatin of bss as a part of this
patch series?
~ Oleksii
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()
2023-03-01 10:34 ` Oleksii
@ 2023-03-01 10:44 ` Jan Beulich
0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2023-03-01 10:44 UTC (permalink / raw)
To: Oleksii
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On 01.03.2023 11:34, Oleksii wrote:
> On Mon, 2023-02-27 at 13:50 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:35, Oleksii Kurochko wrote:
>>> @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
>>> {
>>> early_printk("Hello from C env\n");
>>>
>>> + trap_init();
>>> +
>>> for ( ;; )
>>> asm volatile ("wfi");
>>
>> Along the lines of what Andrew has said there - it's hard to see
>> how this can come (both code flow and patch ordering wise) ahead
>> of clearing .bss.
> So should I add the patch with initializatin of bss as a part of this
> patch series?
That or make clear in the cover letter that there is a dependency.
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>
2023-02-24 11:35 [PATCH v4 0/5] RISCV basic exception handling implementation Oleksii Kurochko
2023-02-24 11:35 ` [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff Oleksii Kurochko
2023-02-24 11:35 ` [PATCH v4 2/5] xen/riscv: introduce trap_init() Oleksii Kurochko
@ 2023-02-24 11:35 ` Oleksii Kurochko
2023-02-27 12:59 ` Jan Beulich
2023-02-24 11:35 ` [PATCH v4 4/5] xen/riscv: test basic handling stuff Oleksii Kurochko
2023-02-24 11:35 ` [PATCH v4 5/5] automation: modify RISC-V smoke test Oleksii Kurochko
4 siblings, 1 reply; 16+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:35 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bob Eshleman, Alistair Francis,
Connor Davis
The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.
The implementation uses "ebreak" instruction in combination with
diffrent bug frame tables (for each type) which contains useful
information.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
- Updates in RISC-V's <asm/bug.h>:
* Add explanatory comment about why there is only defined for 32-bits length
instructions and 16/32-bits BUG_INSN_{16,32}.
* Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
* Update declaration of is_valid_bugaddr(): switch return type from int to bool
and the argument from 'unsigned int' to 'vaddr'.
- Updates in RISC-V's traps.c:
* replace /xen and /asm includes
* update definition of is_valid_bugaddr():switch return type from int to bool
and the argument from 'unsigned int' to 'vaddr'. Code style inside function
was updated too.
* do_bug_frame() refactoring:
* local variables start and bug became 'const struct bug_frame'
* bug_frames[] array became 'static const struct bug_frame[] = ...'
* remove all casts
* remove unneeded comments and add an explanatory comment that the do_bug_frame()
will be switched to a generic one.
* do_trap() refactoring:
* read 16-bits value instead of 32-bits as compressed instruction can
be used and it might happen than only 16-bits may be accessible.
* code style updates
* re-use instr variable instead of re-reading instruction.
- Updates in setup.c:
* add blank line between xen/ and asm/ includes.
---
Changes in V3:
- Rebase the patch "xen/riscv: introduce an implementation of macros
from <asm/bug.h>" on top of patch series [introduce generic implementation
of macros from bug.h]
---
Changes in V2:
- Remove __ in define namings
- Update run_in_exception_handler() with
register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
- Remove bug_instr_t type and change it's usage to uint32_t
---
xen/arch/riscv/include/asm/bug.h | 48 ++++++++++
xen/arch/riscv/include/asm/processor.h | 2 +
xen/arch/riscv/setup.c | 1 +
xen/arch/riscv/traps.c | 125 +++++++++++++++++++++++++
xen/arch/riscv/xen.lds.S | 10 ++
5 files changed, 186 insertions(+)
create mode 100644 xen/arch/riscv/include/asm/bug.h
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
new file mode 100644
index 0000000000..67ade6f895
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2021-2023 Vates
+ *
+ */
+#ifndef _ASM_RISCV_BUG_H
+#define _ASM_RISCV_BUG_H
+
+#include <xen/types.h>
+
+#ifndef __ASSEMBLY__
+
+#define BUG_INSTR "ebreak"
+
+/*
+ * The base instruction set has a fixed length of 32-bit naturally aligned
+ * instructions.
+ *
+ * There are extensions of variable length ( where each instruction can be
+ * any number of 16-bit parcels in length ) but they aren't used in Xen
+ * and Linux kernel ( where these definitions were taken from ).
+ *
+ * Compressed ISA is used now where the instruction length is 16 bit and
+ * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
+ * depending on if compressed ISA is used or not )
+ */
+#define INSN_LENGTH_MASK _UL(0x3)
+#define INSN_LENGTH_32 _UL(0x3)
+
+#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
+#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
+#define COMPRESSED_INSN_MASK _UL(0xffff)
+
+#define GET_INSN_LENGTH(insn) \
+({ \
+ unsigned int len; \
+ len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \
+ 4UL : 2UL; \
+ len; \
+})
+
+/* These are defined by the architecture */
+bool is_valid_bugaddr(vaddr_t addr);
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index a71448e02e..ef23d9589e 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -69,6 +69,8 @@ static inline void die(void)
wfi();
}
+void show_execution_state(const struct cpu_user_regs *regs);
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_RISCV_PROCESSOR_H */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index c8513ca4f8..73b9a82883 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,3 +1,4 @@
+#include <xen/bug.h>
#include <xen/compile.h>
#include <xen/init.h>
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ad7311f269..d87a9cfd2c 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,7 @@
* RISC-V Trap handlers
*/
+#include <xen/bug.h>
#include <xen/errno.h>
#include <xen/lib.h>
@@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
die();
}
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+ early_printk("implement show_execution_state(regs)\n");
+}
+
+/*
+ * TODO: change early_printk's function to early_printk with format
+ * when s(n)printf() will be added.
+ *
+ * Probably the TODO won't be needed as generic do_bug_frame() (at
+ * least, for ARM and RISC-V) has been introduced and current
+ * implementation will be replaced with generic one when panic(),
+ * printk() and find_text_region() (virtual memory?) will be
+ * ready/merged
+ */
+int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
+{
+ const struct bug_frame *start, *end;
+ const struct bug_frame *bug = NULL;
+ unsigned int id = 0;
+ const char *filename, *predicate;
+ int lineno;
+
+ static const struct bug_frame* bug_frames[] = {
+ &__start_bug_frames[0],
+ &__stop_bug_frames_0[0],
+ &__stop_bug_frames_1[0],
+ &__stop_bug_frames_2[0],
+ &__stop_bug_frames_3[0],
+ };
+
+ for ( id = 0; id < BUGFRAME_NR; id++ )
+ {
+ start = bug_frames[id];
+ end = bug_frames[id + 1];
+
+ while ( start != end )
+ {
+ if ( (vaddr_t)bug_loc(start) == pc )
+ {
+ bug = start;
+ goto found;
+ }
+
+ start++;
+ }
+ }
+
+ found:
+ if ( bug == NULL )
+ return -ENOENT;
+
+ if ( id == BUGFRAME_run_fn )
+ {
+ void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+
+ fn(regs);
+
+ goto end;
+ }
+
+ /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+ filename = bug_ptr(bug);
+ lineno = bug_line(bug);
+
+ switch ( id )
+ {
+ case BUGFRAME_warn:
+ early_printk("Xen WARN at ");
+ early_printk(filename);
+ early_printk("\n");
+
+ show_execution_state(regs);
+
+ goto end;
+
+ case BUGFRAME_bug:
+ early_printk("Xen BUG at ");
+ early_printk(filename);
+ early_printk("\n");
+
+ show_execution_state(regs);
+ early_printk("change wait_for_interrupt to panic() when common is available\n");
+ die();
+
+ case BUGFRAME_assert:
+ /* ASSERT: decode the predicate string pointer. */
+ predicate = bug_msg(bug);
+
+ early_printk("Assertion \'");
+ early_printk(predicate);
+ early_printk("\' failed at ");
+ early_printk(filename);
+ early_printk("\n");
+
+ show_execution_state(regs);
+ early_printk("change wait_for_interrupt to panic() when common is available\n");
+ die();
+ }
+
+ return -EINVAL;
+
+ end:
+ return 0;
+}
+
+bool is_valid_bugaddr(vaddr_t insn)
+{
+ if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
+ return ( insn == BUG_INSN_32 );
+ else
+ return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
+}
+
void do_trap(struct cpu_user_regs *cpu_regs)
{
+ register_t pc = cpu_regs->sepc;
+ uint16_t instr = *(uint16_t *)pc;
+
+ if ( is_valid_bugaddr(instr) ) {
+ if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
+ cpu_regs->sepc += GET_INSN_LENGTH(instr);
+ return;
+ }
+ }
+
do_unexpected_trap(cpu_regs);
}
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index ca57cce75c..139e2d04cb 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -39,6 +39,16 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
.rodata : {
_srodata = .; /* Read-only data */
+ /* Bug frames table */
+ __start_bug_frames = .;
+ *(.bug_frames.0)
+ __stop_bug_frames_0 = .;
+ *(.bug_frames.1)
+ __stop_bug_frames_1 = .;
+ *(.bug_frames.2)
+ __stop_bug_frames_2 = .;
+ *(.bug_frames.3)
+ __stop_bug_frames_3 = .;
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
--
2.39.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>
2023-02-24 11:35 ` [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h> Oleksii Kurochko
@ 2023-02-27 12:59 ` Jan Beulich
2023-03-01 11:51 ` Oleksii
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2023-02-27 12:59 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On 24.02.2023 12:35, Oleksii Kurochko wrote:
> The patch introduces macros: BUG(), WARN(), run_in_exception(),
> assert_failed.
>
> The implementation uses "ebreak" instruction in combination with
> diffrent bug frame tables (for each type) which contains useful
> information.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V4:
> - Updates in RISC-V's <asm/bug.h>:
> * Add explanatory comment about why there is only defined for 32-bits length
> instructions and 16/32-bits BUG_INSN_{16,32}.
> * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
> * Update declaration of is_valid_bugaddr(): switch return type from int to bool
> and the argument from 'unsigned int' to 'vaddr'.
> - Updates in RISC-V's traps.c:
> * replace /xen and /asm includes
> * update definition of is_valid_bugaddr():switch return type from int to bool
> and the argument from 'unsigned int' to 'vaddr'. Code style inside function
> was updated too.
> * do_bug_frame() refactoring:
> * local variables start and bug became 'const struct bug_frame'
> * bug_frames[] array became 'static const struct bug_frame[] = ...'
> * remove all casts
> * remove unneeded comments and add an explanatory comment that the do_bug_frame()
> will be switched to a generic one.
> * do_trap() refactoring:
> * read 16-bits value instead of 32-bits as compressed instruction can
> be used and it might happen than only 16-bits may be accessible.
> * code style updates
> * re-use instr variable instead of re-reading instruction.
> - Updates in setup.c:
> * add blank line between xen/ and asm/ includes.
> ---
> Changes in V3:
> - Rebase the patch "xen/riscv: introduce an implementation of macros
> from <asm/bug.h>" on top of patch series [introduce generic implementation
> of macros from bug.h]
> ---
> Changes in V2:
> - Remove __ in define namings
> - Update run_in_exception_handler() with
> register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
> - Remove bug_instr_t type and change it's usage to uint32_t
> ---
> xen/arch/riscv/include/asm/bug.h | 48 ++++++++++
> xen/arch/riscv/include/asm/processor.h | 2 +
> xen/arch/riscv/setup.c | 1 +
> xen/arch/riscv/traps.c | 125 +++++++++++++++++++++++++
> xen/arch/riscv/xen.lds.S | 10 ++
> 5 files changed, 186 insertions(+)
> create mode 100644 xen/arch/riscv/include/asm/bug.h
>
> diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
> new file mode 100644
> index 0000000000..67ade6f895
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates
> + *
> + */
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#include <xen/types.h>
> +
> +#ifndef __ASSEMBLY__
This #ifndef contradicts the xen/types.h inclusion. Either the #include
moves down below here, or the #ifndef goes away as pointless. This is
because xen/types.h cannot be include in assembly files.
> +#define BUG_INSTR "ebreak"
> +
> +/*
> + * The base instruction set has a fixed length of 32-bit naturally aligned
> + * instructions.
> + *
> + * There are extensions of variable length ( where each instruction can be
> + * any number of 16-bit parcels in length ) but they aren't used in Xen
> + * and Linux kernel ( where these definitions were taken from ).
> + *
> + * Compressed ISA is used now where the instruction length is 16 bit and
> + * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> + * depending on if compressed ISA is used or not )
> + */
> +#define INSN_LENGTH_MASK _UL(0x3)
> +#define INSN_LENGTH_32 _UL(0x3)
> +
> +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
> +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
> +#define COMPRESSED_INSN_MASK _UL(0xffff)
> +
> +#define GET_INSN_LENGTH(insn) \
> +({ \
> + unsigned int len; \
> + len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \
> + 4UL : 2UL; \
Why the UL suffixes?
> + len; \
> +})
And anyway - can't the whole macro be written without using any
extension:
#define GET_INSN_LENGTH(insn) \
(((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
? (Note also that uses of macro parameters should be properly
parenthesized.)
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,3 +1,4 @@
> +#include <xen/bug.h>
> #include <xen/compile.h>
> #include <xen/init.h>
Why? Eventually this will be needed, I agree, but right now?
> @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
> die();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> + early_printk("implement show_execution_state(regs)\n");
> +}
> +
> +/*
> + * TODO: change early_printk's function to early_printk with format
> + * when s(n)printf() will be added.
> + *
> + * Probably the TODO won't be needed as generic do_bug_frame() (at
> + * least, for ARM and RISC-V) has been introduced and current
> + * implementation will be replaced with generic one when panic(),
> + * printk() and find_text_region() (virtual memory?) will be
> + * ready/merged
> + */
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> + const struct bug_frame *start, *end;
> + const struct bug_frame *bug = NULL;
> + unsigned int id = 0;
> + const char *filename, *predicate;
> + int lineno;
> +
> + static const struct bug_frame* bug_frames[] = {
> + &__start_bug_frames[0],
> + &__stop_bug_frames_0[0],
> + &__stop_bug_frames_1[0],
> + &__stop_bug_frames_2[0],
> + &__stop_bug_frames_3[0],
> + };
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + start = bug_frames[id];
> + end = bug_frames[id + 1];
> +
> + while ( start != end )
> + {
> + if ( (vaddr_t)bug_loc(start) == pc )
> + {
> + bug = start;
> + goto found;
> + }
> +
> + start++;
> + }
> + }
> +
> + found:
> + if ( bug == NULL )
> + return -ENOENT;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> + fn(regs);
> +
> + goto end;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_ptr(bug);
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + early_printk("Xen WARN at ");
> + early_printk(filename);
> + early_printk("\n");
> +
> + show_execution_state(regs);
> +
> + goto end;
> +
> + case BUGFRAME_bug:
> + early_printk("Xen BUG at ");
> + early_printk(filename);
> + early_printk("\n");
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is available\n");
> + die();
> +
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> +
> + early_printk("Assertion \'");
> + early_printk(predicate);
> + early_printk("\' failed at ");
> + early_printk(filename);
> + early_printk("\n");
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is available\n");
> + die();
> + }
> +
> + return -EINVAL;
> +
> + end:
> + return 0;
> +}
Wasn't the goal to use the generic do_bug_frame()?
> +bool is_valid_bugaddr(vaddr_t insn)
Why is insn of type vaddr_t?
> +{
> + if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> + return ( insn == BUG_INSN_32 );
> + else
> + return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
Nit (style): The kind-of-operand to return is an expression. Hence
you have stray blanks there immediately inside the parentheses.
(This is unlike e.g. if(), where you've formatted things correctly.)
> +}
> +
> void do_trap(struct cpu_user_regs *cpu_regs)
> {
> + register_t pc = cpu_regs->sepc;
> + uint16_t instr = *(uint16_t *)pc;
> +
> + if ( is_valid_bugaddr(instr) ) {
> + if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
Nit: Brace placement (twice).
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>
2023-02-27 12:59 ` Jan Beulich
@ 2023-03-01 11:51 ` Oleksii
2023-03-01 12:42 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Oleksii @ 2023-03-01 11:51 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > The patch introduces macros: BUG(), WARN(), run_in_exception(),
> > assert_failed.
> >
> > The implementation uses "ebreak" instruction in combination with
> > diffrent bug frame tables (for each type) which contains useful
> > information.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V4:
> > - Updates in RISC-V's <asm/bug.h>:
> > * Add explanatory comment about why there is only defined for
> > 32-bits length
> > instructions and 16/32-bits BUG_INSN_{16,32}.
> > * Change 'unsigned long' to 'unsigned int' inside
> > GET_INSN_LENGTH().
> > * Update declaration of is_valid_bugaddr(): switch return type
> > from int to bool
> > and the argument from 'unsigned int' to 'vaddr'.
> > - Updates in RISC-V's traps.c:
> > * replace /xen and /asm includes
> > * update definition of is_valid_bugaddr():switch return type
> > from int to bool
> > and the argument from 'unsigned int' to 'vaddr'. Code style
> > inside function
> > was updated too.
> > * do_bug_frame() refactoring:
> > * local variables start and bug became 'const struct
> > bug_frame'
> > * bug_frames[] array became 'static const struct bug_frame[]
> > = ...'
> > * remove all casts
> > * remove unneeded comments and add an explanatory comment
> > that the do_bug_frame()
> > will be switched to a generic one.
> > * do_trap() refactoring:
> > * read 16-bits value instead of 32-bits as compressed
> > instruction can
> > be used and it might happen than only 16-bits may be
> > accessible.
> > * code style updates
> > * re-use instr variable instead of re-reading instruction.
> > - Updates in setup.c:
> > * add blank line between xen/ and asm/ includes.
> > ---
> > Changes in V3:
> > - Rebase the patch "xen/riscv: introduce an implementation of
> > macros
> > from <asm/bug.h>" on top of patch series [introduce generic
> > implementation
> > of macros from bug.h]
> > ---
> > Changes in V2:
> > - Remove __ in define namings
> > - Update run_in_exception_handler() with
> > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
> > - Remove bug_instr_t type and change it's usage to uint32_t
> > ---
> > xen/arch/riscv/include/asm/bug.h | 48 ++++++++++
> > xen/arch/riscv/include/asm/processor.h | 2 +
> > xen/arch/riscv/setup.c | 1 +
> > xen/arch/riscv/traps.c | 125
> > +++++++++++++++++++++++++
> > xen/arch/riscv/xen.lds.S | 10 ++
> > 5 files changed, 186 insertions(+)
> > create mode 100644 xen/arch/riscv/include/asm/bug.h
> >
> > diff --git a/xen/arch/riscv/include/asm/bug.h
> > b/xen/arch/riscv/include/asm/bug.h
> > new file mode 100644
> > index 0000000000..67ade6f895
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2021-2023 Vates
> > + *
> > + */
> > +#ifndef _ASM_RISCV_BUG_H
> > +#define _ASM_RISCV_BUG_H
> > +
> > +#include <xen/types.h>
> > +
> > +#ifndef __ASSEMBLY__
>
> This #ifndef contradicts the xen/types.h inclusion. Either the
> #include
> moves down below here, or the #ifndef goes away as pointless. This is
> because xen/types.h cannot be include in assembly files.
I will remove it. It looks like it leave when as generic implementation
was taken ARM.
>
> > +#define BUG_INSTR "ebreak"
> > +
> > +/*
> > + * The base instruction set has a fixed length of 32-bit naturally
> > aligned
> > + * instructions.
> > + *
> > + * There are extensions of variable length ( where each
> > instruction can be
> > + * any number of 16-bit parcels in length ) but they aren't used
> > in Xen
> > + * and Linux kernel ( where these definitions were taken from ).
> > + *
> > + * Compressed ISA is used now where the instruction length is 16
> > bit and
> > + * 'ebreak' instruction, in this case, can be either 16 or 32 bit
> > (
> > + * depending on if compressed ISA is used or not )
> > + */
> > +#define INSN_LENGTH_MASK _UL(0x3)
> > +#define INSN_LENGTH_32 _UL(0x3)
> > +
> > +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
> > +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
> > +#define COMPRESSED_INSN_MASK _UL(0xffff)
> > +
> > +#define GET_INSN_LENGTH(insn) \
> > +({ \
> > + unsigned int len; \
> > + len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \
> > + 4UL : 2UL; \
>
> Why the UL suffixes?
>
> > + len; \
> > +})
>
> And anyway - can't the whole macro be written without using any
> extension:
>
> #define GET_INSN_LENGTH(insn) \
> (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
>
> ? (Note also that uses of macro parameters should be properly
> parenthesized.)
It can. I'll update the macros.
>
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,3 +1,4 @@
> > +#include <xen/bug.h>
> > #include <xen/compile.h>
> > #include <xen/init.h>
>
> Why? Eventually this will be needed, I agree, but right now?
Agree. <xen/bug.h> should be removed fro the current patch.
Probably merge error during rebase.
>
> > @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> > die();
> > }
> >
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > + early_printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: change early_printk's function to early_printk with
> > format
> > + * when s(n)printf() will be added.
> > + *
> > + * Probably the TODO won't be needed as generic do_bug_frame() (at
> > + * least, for ARM and RISC-V) has been introduced and current
> > + * implementation will be replaced with generic one when panic(),
> > + * printk() and find_text_region() (virtual memory?) will be
> > + * ready/merged
> > + */
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> > +{
> > + const struct bug_frame *start, *end;
> > + const struct bug_frame *bug = NULL;
> > + unsigned int id = 0;
> > + const char *filename, *predicate;
> > + int lineno;
> > +
> > + static const struct bug_frame* bug_frames[] = {
> > + &__start_bug_frames[0],
> > + &__stop_bug_frames_0[0],
> > + &__stop_bug_frames_1[0],
> > + &__stop_bug_frames_2[0],
> > + &__stop_bug_frames_3[0],
> > + };
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + start = bug_frames[id];
> > + end = bug_frames[id + 1];
> > +
> > + while ( start != end )
> > + {
> > + if ( (vaddr_t)bug_loc(start) == pc )
> > + {
> > + bug = start;
> > + goto found;
> > + }
> > +
> > + start++;
> > + }
> > + }
> > +
> > + found:
> > + if ( bug == NULL )
> > + return -ENOENT;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > + fn(regs);
> > +
> > + goto end;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + early_printk("Xen WARN at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > +
> > + goto end;
> > +
> > + case BUGFRAME_bug:
> > + early_printk("Xen BUG at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > + early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > + die();
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > +
> > + early_printk("Assertion \'");
> > + early_printk(predicate);
> > + early_printk("\' failed at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > + early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > + die();
> > + }
> > +
> > + return -EINVAL;
> > +
> > + end:
> > + return 0;
> > +}
>
> Wasn't the goal to use the generic do_bug_frame()?
It's the goal but at the current stage we can't switch to
do_bug_frame() because it is based on function from xen/common which we
don't have now for RISC-V. So it will be switched when xen/common will
be available.
>
> > +bool is_valid_bugaddr(vaddr_t insn)
>
> Why is insn of type vaddr_t?
Initially address of insn was passed here. But now we should change
vaddr_t to 'uint32_t' as the instruction length can be either 16 or 32
and update instr variable inside do_trap():
static uint32_t read_instr(unsigned long pc)
{
uint16_t instr16 = *(uint16_t *)pc;
if ( GET_INSN_LENGTH(instr16) == 2 )
return (uint32_t)instr16;
else
return *(uint32_t *)pc;
}
void do_trap(struct cpu_user_regs *cpu_regs)
{
register_t pc = cpu_regs->sepc;
uint32_t instr = read_instr(pc);
...
>
> > +{
> > + if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> > + return ( insn == BUG_INSN_32 );
> > + else
> > + return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
>
> Nit (style): The kind-of-operand to return is an expression. Hence
> you have stray blanks there immediately inside the parentheses.
> (This is unlike e.g. if(), where you've formatted things correctly.)
To be 100% sure, should it be:
return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );
> > +}
> > +
> > void do_trap(struct cpu_user_regs *cpu_regs)
> > {
> > + register_t pc = cpu_regs->sepc;
> > + uint16_t instr = *(uint16_t *)pc;
> > +
> > + if ( is_valid_bugaddr(instr) ) {
> > + if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
>
> Nit: Brace placement (twice).
Thanks. I'll update then.
>
> Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>
2023-03-01 11:51 ` Oleksii
@ 2023-03-01 12:42 ` Jan Beulich
0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2023-03-01 12:42 UTC (permalink / raw)
To: Oleksii
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bob Eshleman, Alistair Francis, Connor Davis, xen-devel
On 01.03.2023 12:51, Oleksii wrote:
> On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:35, Oleksii Kurochko wrote:
>>> +{
>>> + if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
>>> + return ( insn == BUG_INSN_32 );
>>> + else
>>> + return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
>>
>> Nit (style): The kind-of-operand to return is an expression. Hence
>> you have stray blanks there immediately inside the parentheses.
>> (This is unlike e.g. if(), where you've formatted things correctly.)
> To be 100% sure, should it be:
> return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );
No, that's yet more spaces instead of fewer ones.
if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
return insn == BUG_INSN_32;
else
return (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
or, if you really want the extra parentheses:
if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
return (insn == BUG_INSN_32);
else
return ((insn & COMPRESSED_INSN_MASK) == BUG_INSN_16);
(Personally I'd also omit the "else":
if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
return insn == BUG_INSN_32;
return (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
. Plus I don't think you really need to mask as much, i.e.
return insn == BUG_INSN_32 ||
(insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
would do as well afaict.)
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/5] xen/riscv: test basic handling stuff
2023-02-24 11:35 [PATCH v4 0/5] RISCV basic exception handling implementation Oleksii Kurochko
` (2 preceding siblings ...)
2023-02-24 11:35 ` [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h> Oleksii Kurochko
@ 2023-02-24 11:35 ` Oleksii Kurochko
2023-02-24 11:35 ` [PATCH v4 5/5] automation: modify RISC-V smoke test Oleksii Kurochko
4 siblings, 0 replies; 16+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:35 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bob Eshleman, Alistair Francis,
Connor Davis
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V4:
- Add Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V3:
- Nothing changed
---
Changes in V2:
- Nothing changed
---
xen/arch/riscv/setup.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 73b9a82883..b3f8b10f71 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -10,12 +10,28 @@
unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
__aligned(STACK_SIZE);
+static void test_run_in_exception(struct cpu_user_regs *regs)
+{
+ early_printk("If you see this message, ");
+ early_printk("run_in_exception_handler is most likely working\n");
+}
+
+static void test_macros_from_bug_h(void)
+{
+ run_in_exception_handler(test_run_in_exception);
+ WARN();
+ early_printk("If you see this message, ");
+ early_printk("WARN is most likely working\n");
+}
+
void __init noreturn start_xen(void)
{
early_printk("Hello from C env\n");
trap_init();
+ test_macros_from_bug_h();
+
for ( ;; )
asm volatile ("wfi");
--
2.39.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 5/5] automation: modify RISC-V smoke test
2023-02-24 11:35 [PATCH v4 0/5] RISCV basic exception handling implementation Oleksii Kurochko
` (3 preceding siblings ...)
2023-02-24 11:35 ` [PATCH v4 4/5] xen/riscv: test basic handling stuff Oleksii Kurochko
@ 2023-02-24 11:35 ` Oleksii Kurochko
4 siblings, 0 replies; 16+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:35 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Doug Goldstein,
Alistair Francis
The patch modifies the grep pattern to reflect the usage of WARN.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in V4:
- Add Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
and Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in V3:
- Update commit message
---
Changes in V2:
- Leave only the latest "grep ..."
---
automation/scripts/qemu-smoke-riscv64.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index e0f06360bc..02fc66be03 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -16,5 +16,5 @@ qemu-system-riscv64 \
|& tee smoke.serial
set -e
-(grep -q "Hello from C env" smoke.serial) || exit 1
+(grep -q "WARN is most likely working" smoke.serial) || exit 1
exit 0
--
2.39.0
^ permalink raw reply related [flat|nested] 16+ messages in thread