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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/24/26 3:04 PM, Jan Beulich wrote:
> 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@xxxxxxxxx>
> > ---
> > 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?
No, I'll use singular form.

I called it tables as potentially I will need a different types of 
exception table, so I counted different types as different exception tables.
> > --- 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.
I see your point and at the moment there is no also other options how
to handle case(s) for which exception table is introduced now. But if 
potentially another mechanism will be introduced what will be the point 
to have extable.o code in the final binary?
> > --- /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)?
It makes sense. Lets switch to "long".

> > +    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)?
I will introduce then void init_or_livepatch sort_exception_table(...) 
and re-use it inside sort_extable() with renaming it to 
sort_exception_tables() to take into account live-patching which 
requires sort_exception_table().
> > +{
> > +    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).
The stuff is easier with such implementation.

We could really drop cmp_ex_search() if to pass struct 
exception_table_entry instead of (unsigned long *) so the following will 
allow to drop cmp_ex_search():
@@ -78,12 +78,15 @@ 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;
+    struct exception_table_entry key;

     if ( !region || !region->ex )
         return false;

-    ex = bsearch(&pc, region->ex, region->ex_end - region->ex,
-                 sizeof(struct exception_table_entry), cmp_ex_search);
+    key.insn = pc - (unsigned long)&key.insn;
+
+    ex = bsearch(&key, region->ex, region->ex_end - region->ex,
+                 sizeof(struct exception_table_entry), cmp_ex_sort 
/*cmp_ex_search*/);
Also, then I will rename l and r variable inside cmp_ex_sort() to
insn_a and insn_b.

> > +    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?
As potentially we could have other handlers which might return not only 
true, so it will be easier to handle return type inside fixup_exception().
But if you think there is no any sense to have for handlers the same 
signature then I am also return void instead of bool for 
ex_handler_fixup().
> > --- /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?
Good question... It was:

.macro asm_extable, insn, fixup
    ASM_EXTABLE(\insn, \fixup)
.endm

> > +
> > +/*
> > + * 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?
It is. "No register" meant no general purpose register. I will reprase 
this part to:
No general-purpose registers are modified by the exception handling 
mechanism itself, so it is up to the fixup code to handle any necessary 
state cleanup.
Or it could be just dropped.

> > + * 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.
It is orphaned from Linux (generally it is about that some functions 
from uaccess.h are using ASM_EXTABLE, the similar for Xen has in 
x86/uaccess.h). I'll rephrase it to:
 * The exception table and fixup code live 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.

> > + */
> > +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?
To handle the potential in future case that CONFIG_HAS_EX_TABLE will 
become conditional.
I thought that it makes sense to be in sync with common/virtual_region.c 
also uses ifdef around exception table related information.
> > --- 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?
Here I can agree that there is not reason as if CONFIG_HAS_EX_TABLE is n
then no one is expected to use exception table so the section is empty 
and don't occupy any extra space in binary (except potentially some 
space because of alignment).

> > +#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.
For the  current struct - yes, we can do . = ALIGN(4) but if the 
architecture will add uint64_t inside (or unsigned long) shouldn't we 
then have ALIGN(POINTER_ALIGN)?
Thanks.

~ Oleksii