Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] riscv: xip: support runtime trap patching
To:     Vitaly Wool <vitaly.wool@konsulko.com>,
        linux-riscv@lists.infradead.org
Cc:     linux-kernel@vger.kernel.org,
        Palmer Dabbelt <palmerdabbelt@google.com>,
        Nicolas Pitre <nico@fluxnic.net>
References: <20210531085342.17494-1-vitaly.wool@konsulko.com>
From:   Alex Ghiti <alex@ghiti.fr>
Message-ID: <62961217-1733-eaa3-b072-cbb252620c9a@ghiti.fr>
Date:   Mon, 31 May 2021 17:17:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <20210531085342.17494-1-vitaly.wool@konsulko.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?

> Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.com>
> ---
>   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?

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

Thanks,

Alex