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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/2/26 8:24 AM, Jan Beulich wrote:
> 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.
Originally, I thought to imeplement this function something like:
  return insn_a - insn_b;

It'd get integer overflow when insn_a is a very small number and insn_b 
is very large.
It could drop the comment to avoid confusion.


> > --- /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?
It looked visually better to me but I am okay to drop them.

> 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.
Agree, we could use .word instead of .long.

> Preferably with the adjustments:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks a lot.

> Happy to carry out while committing, provided you agree.
I would be happy with that.

~ Oleksii