Re: [PATCH] riscv: xip: support runtime trap patching

[Vitaly Wool]

On Mon, May 31, 2021 at 5:17 PM Alex Ghiti <alex@xxxxxxxx> wrote:
>
> Hi Vitaly,
>
> Le 31/05/2021 à 10:53, Vitaly Wool a écrit :
> > RISCV_ERRATA_ALTERNATIVE patches text at runtime which is currently
> > not possible when the kernel is executed from the flash in XIP mode.
> > Since runtime patching concerns only traps at the moment, let's just
> > have all the traps reside in RAM anyway if RISCV_ERRATA_ALTERNATIVE
> > is set. Thus, these functions will be patch-able even when the .text
> > section is in flash.
> >
>
> This sounds like a good fix for sifive platforms to work with XIP kernel
> in 5.13: did you test that it actually works on HW?

I don't have any SiFive HW so strictly speaking, not quite. However, I
created a test configuration for Polarfire with a basically no-op
runtime patching and it apparently worked. Traps end up in RAM and are
changeable.

> > Signed-off-by: Vitaly Wool <vitaly.wool@xxxxxxxxxxxx>
> > ---
> >   arch/riscv/kernel/traps.c           | 13 +++++++++----
> >   arch/riscv/kernel/vmlinux-xip.lds.S | 15 ++++++++++++++-
> >   2 files changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> > index 0721b9798595..7bc88d8aab97 100644
> > --- a/arch/riscv/kernel/traps.c
> > +++ b/arch/riscv/kernel/traps.c
> > @@ -86,8 +86,13 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
> >       }
> >   }
> >
> > +#if defined (CONFIG_XIP_KERNEL) && defined (CONFIG_RISCV_ERRATA_ALTERNATIVE)
> > +#define __trap_section               __section(".xip.traps")
> > +#else
> > +#define __trap_section
> > +#endif
>
> Maybe we could do something more generic. At the moment, only traps are
> subject to alternatives but that will likely expand: what about rather
> defining a section called __alternative_section?
>
> >   #define DO_ERROR_INFO(name, signo, code, str)                               \
> > -asmlinkage __visible void name(struct pt_regs *regs)                 \
> > +asmlinkage __visible __trap_section void name(struct pt_regs *regs)  \
> >   {                                                                   \
> >       do_trap_error(regs, signo, code, regs->epc, "Oops - " str);     \
> >   }
> > @@ -111,7 +116,7 @@ DO_ERROR_INFO(do_trap_store_misaligned,
> >   int handle_misaligned_load(struct pt_regs *regs);
> >   int handle_misaligned_store(struct pt_regs *regs);
> >
> > -asmlinkage void do_trap_load_misaligned(struct pt_regs *regs)
> > +asmlinkage void __trap_section do_trap_load_misaligned(struct pt_regs *regs)
> >   {
> >       if (!handle_misaligned_load(regs))
> >               return;
> > @@ -119,7 +124,7 @@ asmlinkage void do_trap_load_misaligned(struct pt_regs *regs)
> >                     "Oops - load address misaligned");
> >   }
> >
> > -asmlinkage void do_trap_store_misaligned(struct pt_regs *regs)
> > +asmlinkage void __trap_section do_trap_store_misaligned(struct pt_regs *regs)
> >   {
> >       if (!handle_misaligned_store(regs))
> >               return;
> > @@ -146,7 +151,7 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
> >       return GET_INSN_LENGTH(insn);
> >   }
> >
> > -asmlinkage __visible void do_trap_break(struct pt_regs *regs)
> > +asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
> >   {
> >   #ifdef CONFIG_KPROBES
> >       if (kprobe_single_step_handler(regs))
> > diff --git a/arch/riscv/kernel/vmlinux-xip.lds.S b/arch/riscv/kernel/vmlinux-xip.lds.S
> > index 4b29b9917f99..a3ff09c4c3f9 100644
> > --- a/arch/riscv/kernel/vmlinux-xip.lds.S
> > +++ b/arch/riscv/kernel/vmlinux-xip.lds.S
> > @@ -99,9 +99,22 @@ SECTIONS
> >       }
> >       PERCPU_SECTION(L1_CACHE_BYTES)
> >
> > -     . = ALIGN(PAGE_SIZE);
> > +     . = ALIGN(8);
> > +     .alternative : {
> > +             __alt_start = .;
> > +             *(.alternative)
> > +             __alt_end = .;
> > +     }
> >       __init_end = .;
> >
> > +     . = ALIGN(16);
>
> Why 16 here?

Honestly, I don't remember. I believe it should have been 8 but didn't
bother to check, thought it was not a big deal anyway :)

Best regards,
   Vitaly

> > +     .xip.traps : {
> > +             __xip_traps_start = .;
> > +             *(.xip.traps)
> > +             __xip_traps_end = .;
> > +     }
> > +
> > +     . = ALIGN(PAGE_SIZE);
> >       .sdata : {
> >               __global_pointer$ = . + 0x800;
> >               *(.sdata*)
> >
>
> Thanks,
>
> Alex