|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/4] xen/riscv: add exception table support
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?
> --- 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |