On 29.05.2023 14:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/bug.h
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -7,4 +7,32 @@
> #ifndef _ASM_RISCV_BUG_H
> #define _ASM_RISCV_BUG_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 ).
This, at least to some degree, looks to contradict ...
> + * 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 )
... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed insns
can very well be used in Xen.
> @@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct
> cpu_user_regs *regs)
> die();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> + 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.
What is this comment about? I don't think I understand what it says
needs doing.
> + * Probably the TODO won't be needed as generic do_bug_frame()
> + * 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)
While it's going to be the maintainers to judge, I continue to be
unconvinced that introducing copies of common functions (also in
patch 1) is a good idea.
> +{
> + 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[] = {
Nit: * and blank want to swap places. I would also expect another
"const".
> +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;
> +}
As long as this function is only used on Xen code, it's kind of okay.
There you/we control whether code can change behind our backs. But as
soon as you might use this on guest code, the double read is going to
be a problem (I think; I wonder how hardware is supposed to deal with
the situation: Maybe they indeed fetch in 16-bit quantities?).
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -40,6 +40,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)
Nit: There looks to be an off-by-one in how you indent your addition
(except for the comment).
Jan
