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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@suse.com>

Thanks a lot.

> Happy to carry out while committing, provided you agree.

I would be happy with that.

~ Oleksii