Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.07.2024 13:23, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  xen/arch/riscv/Kconfig |  1 +
>  xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++
>  xen/common/bug.c       |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> index b4b354a778..74ad019fe7 100644
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -5,6 +5,7 @@ config RISCV
>  config RISCV_64
>       def_bool y
>       select 64BIT
> +     select GENERIC_BUG_FRAME

Any particular reason to put this here, and not higher up with RISCV?

> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct 
> cpu_user_regs *regs)
>      die();
>  }
>  
> +static bool is_valid_bug_insn(uint32_t insn)
> +{
> +    return insn == BUG_INSN_32 ||
> +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> +}
> +
> +/* Should be used only on Xen code */
> +static uint32_t read_instr(unsigned long pc)
> +{
> +    uint16_t instr16 = *(uint16_t *)pc;
> +
> +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
> +
> +    if ( GET_INSN_LENGTH(instr16) == 2 )
> +        return instr16;
> +
> +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
> +
> +    return *(uint32_t *)pc;
> +}

Related to the point made further down: If either of these assertions fails,
won't we come back again right here? If either of the is_kernel_*text()
wasn't working quite right, wouldn't we be at risk of entering an infinite
loop (presumably not quite infinite because of the stack overflowing at some
point)?

>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> +    register_t pc = cpu_regs->sepc;
> +    uint32_t instr = read_instr(pc);
> +
> +    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, pc) >= 0 ) 
> )

No consideration of the kind of exception? I'd expect it is one very
specific one which the BUG insn would raise, and then there's no point
fetching the insn when it's a different kind of exception.

Further, nit: Certainly no need for the parentheses on the lhs of the &&.
Having them on the rhs is a matter of taste, so okay, but then the
blanks immediately inside will want dropping.


> --- a/xen/common/bug.c
> +++ b/xen/common/bug.c
> @@ -1,6 +1,7 @@
>  #include <xen/bug.h>
>  #include <xen/errno.h>
>  #include <xen/kernel.h>
> +#include <xen/lib.h>
>  #include <xen/livepatch.h>
>  #include <xen/string.h>
>  #include <xen/types.h>

Unrelated change? Or did you simply forget to mention in the description
why it's needed?

Jan