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

[Jan Beulich <jbeulich@suse.com>]

On 13.03.2026 17:44, Oleksii Kurochko wrote:
> Introduce exception table handling for RISC-V so faults from selected
> instructions can be recovered via fixup handlers instead of being
> treated as fatal.
> 
> Add the RISC-V exception table format, sorting at boot to allow binary
> search used furthuer, and lookup from the trap handler. Update the
> linker script to emit the .ex_table section using introduced common
> EX_TABLE macro shared with other architectures.
> 
> Also, the __start___ext_table is aligned now by POINTER_ALIGN instead
> of just using hard-coded 8 as there is no too much sense to align
> __start___ext_table by 8 for 32-bit systems.

Nit: The identifier named here twice isn't correct (extra 't').

> This implementation is based on Linux 6.16.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Open question:
> 
> With some renaming the following could be generic, at least, between
> x86 and RISC-V:
>  - ASM_EXTABLE() definition
>  - All what is conencted with sort_extable().
>  - With some change of how x86 searchs an extension this cmp_ex_search()
>    could also go to common file.
> 
> Does it make sense to introduce xen/extable.h and common/extable.c?

Maybe, but not right here. Already the introduction of EX_TABLE for
linker script use might better have been broken out.

Seeing the names you suggest here, ...

> ---
>  xen/arch/riscv/Kconfig                |  1 +
>  xen/arch/riscv/Makefile               |  1 +
>  xen/arch/riscv/extables.c             | 85 +++++++++++++++++++++++++++
>  xen/arch/riscv/include/asm/extables.h | 72 +++++++++++++++++++++++
>  xen/arch/riscv/setup.c                |  3 +
>  xen/arch/riscv/traps.c                |  3 +
>  xen/arch/riscv/xen.lds.S              |  3 +
>  xen/arch/x86/xen.lds.S                |  6 +-
>  xen/include/xen/xen.lds.h             | 10 ++++
>  9 files changed, 179 insertions(+), 5 deletions(-)
>  create mode 100644 xen/arch/riscv/extables.c
>  create mode 100644 xen/arch/riscv/include/asm/extables.h

... is there a reason you use plural in the name here?

> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -3,6 +3,7 @@ obj-y += cpufeature.o
>  obj-y += domain.o
>  obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-y += entry.o
> +obj-$(CONFIG_HAS_EX_TABLE) += extables.o

Simply obj-y please as long as the select is unconditional.

> --- /dev/null
> +++ b/xen/arch/riscv/extables.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/sort.h>
> +#include <xen/virtual_region.h>
> +
> +#include <asm/extables.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;
> +    int delta = b - a;

Better play safe and use "long" (as we have it for x86)?

> +    tmp = *x;
> +    x->insn = y->insn + delta;
> +    y->insn = tmp.insn - delta;
> +
> +    x->fixup = y->fixup + delta;
> +    y->fixup = tmp.fixup - delta;
> +}
> +
> +static int __init cf_check cmp_ex_sort(const void *a, const void *b)
> +{
> +    const unsigned long l = ex_insn(a);
> +    const unsigned long r = ex_insn(b);
> +
> +    /* avoid overflow */
> +    return (l > r) - (l < r);
> +}
> +
> +void __init sort_extable(void)

Better account for live-patching right away (see corresponding x86 code)?

> +{
> +    sort(__start___ex_table,  __stop___ex_table - __start___ex_table,
> +         sizeof(struct exception_table_entry), cmp_ex_sort, swap_ex);
> +}
> +
> +static int cf_check cmp_ex_search(const void *key, const void *elt)
> +{
> +    const unsigned long k = *(const unsigned long *)key;

The deref here looks to be needed solely because you pass &pc into bsearch().
Generally I'd expect both search functions to be pretty similar (if already
distinct ones are needed, which indeed looks to make things easier here).

> +    const unsigned long insn = ex_insn(elt);
> +
> +    /* avoid overflow */
> +    return (k > insn) - (k < insn);
> +}
> +
> +static bool ex_handler_fixup(const struct exception_table_entry *ex,
> +			                 struct cpu_user_regs *regs)

Nit: Bad indentation.

> +{
> +	regs->sepc = ex_fixup(ex);
> +
> +	return true;

Nit: Bad use of hard tabs.

And then - why the boolean return type, when this can't fail anyway?

> +}
> +
> +bool fixup_exception(struct cpu_user_regs *regs)
> +{
> +    unsigned long pc = regs->sepc;
> +    const struct virtual_region *region = find_text_region(pc);
> +    const struct exception_table_entry *ex;
> +
> +    if ( !region || !region->ex )
> +        return false;
> +
> +    ex = bsearch(&pc, region->ex, region->ex_end - region->ex,
> +                 sizeof(struct exception_table_entry), cmp_ex_search);

Please prefer sizeof(<expression>) over sizeof(<type>) (also in the sort()
invocation further up, as I notice only now).

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

Nit: More uses of hard tabs. Maybe that alone is the reason for the mis-aligned
trailing backslashes.

> +    .popsection;
> +.endm

I can't spot the corresponding .macro. What's going on here?

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

More misaligned backslashes.

> +    ".popsection\n"
> +
> +/*
> + * The exception table consists of pairs of relative offsets: the first
> + * is the relative offset to an instruction that is allowed to fault,
> + * and the second is the relative offset at which the program should
> + * continue. No registers are modified, so it is entirely up to the
> + * continuation code to figure out what to do.

And the program counter is not a register?

> + * All the routines below use bits of fixup code that are out of line
> + * with the main instruction path.  This means when everything is well,
> + * we don't even have to jump over them.  Further, they do not intrude
> + * on our cache or tlb entries.

What is this paragraph about? There's nothing "below" which I can
associate this with.

> + */
> +struct exception_table_entry {
> +	int32_t insn, fixup;
> +};
> +
> +extern struct exception_table_entry __start___ex_table[];
> +extern struct exception_table_entry __stop___ex_table[];
> +
> +#ifdef CONFIG_HAS_EX_TABLE

Why, when this is a RISC-V specific header and HAS_EX_TABLE is selected
unconditionally?

> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -12,6 +12,7 @@
>  #include <xen/sched.h>
>  #include <xen/softirq.h>
>  
> +#include <asm/extables.h>
>  #include <asm/cpufeature.h>
>  #include <asm/intc.h>
>  #include <asm/processor.h>
> @@ -217,6 +218,8 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>  
>              break;
>          }
> +        else if ( fixup_exception(cpu_regs) )
> +            break;

Instead od the "else" better put a blank line ahead of the if(), to
visually separate the set of checks.

> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -219,4 +219,14 @@
>  #define VPCI_ARRAY
>  #endif
>  
> +#ifdef CONFIG_HAS_EX_TABLE

No real need for this?

> +#define EX_TABLE                  \
> +        . = ALIGN(POINTER_ALIGN); \

Strictly speaking the original 8 (in x86 code) as much as this is more
than we need - each element is a struct of 2 4-byte entities, after all.

Jan