Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

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