Re: [PATCH v2 1/4] xen/riscv: add exception table support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2026 21:04, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/extable.c
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/init.h>
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch.h>
> +#include <xen/sort.h>
> +#include <xen/virtual_region.h>
> +
> +#include <asm/extable.h>
> +#include <asm/processor.h>
> +
> +#define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
> +
> +static inline unsigned long ex_insn(const struct exception_table_entry *ex)
> +{
> +    return EX_FIELD(ex, insn);
> +}
> +
> +static inline unsigned long ex_fixup(const struct exception_table_entry *ex)
> +{
> +    return EX_FIELD(ex, fixup);
> +}
> +
> +static void __init cf_check swap_ex(void *a, void *b)
> +{
> +    struct exception_table_entry *x = a, *y = b, tmp;
> +    long delta = b - a;
> +
> +    tmp = *x;
> +    x->insn = y->insn + delta;
> +    y->insn = tmp.insn - delta;
> +
> +    x->fixup = y->fixup + delta;
> +    y->fixup = tmp.fixup - delta;
> +}
> +
> +static int cf_check cmp_ex(const void *a, const void *b)
> +{
> +    const unsigned long insn_a = ex_insn(a);
> +    const unsigned long insn_b = ex_insn(b);
> +
> +    /* avoid overflow */
> +    return (insn_a > insn_b) - (insn_a < insn_b);

What is the (slightly malformed) comment about? I don't see anything close
to possibly causing overflow here.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/extable.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef ASM__RISCV__ASM_EXTABLE_H
> +#define ASM__RISCV__ASM_EXTABLE_H
> +
> +#ifdef __ASSEMBLER__
> +
> +#define ASM_EXTABLE(insn, fixup)    \
> +    .pushsection .ex_table, "a";    \
> +    .balign     4;                  \
> +    .long       ((insn) - .);       \
> +    .long       ((fixup) - .);      \

For readability's sake I'm generally advocating for having enough, but
not more parentheses than necessary. What's the purpose of the outer pair
here and ...

> +    .popsection;
> +
> +.macro asm_extable, insn, fixup
> +    ASM_EXTABLE(\insn, \fixup)
> +.endm
> +
> +#else /* __ASSEMBLER__ */
> +
> +#include <xen/stringify.h>
> +#include <xen/types.h>
> +
> +struct cpu_user_regs;
> +
> +#define ASM_EXTABLE(insn, fixup)        \
> +    ".pushsection .ex_table, \"a\"\n"   \
> +    ".balign    4\n"                    \
> +    ".long      ((" #insn ") - .)\n"    \
> +    ".long      ((" #fixup ") - .)\n"   \

... here?

I'm also uncertain about the use of .long (generally in RISC-V code, and
really also in some other architectures). Imo, considering suffixes used
in the instruction set (e.g. load/store insns or OP-32 ones in RV64) .word
may be the more expressive directive.

Preferably with the adjustments:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Happy to carry out while committing, provided you agree.

Jan