Stafford Horne <sho...@gmail.com> 于2025年8月9日周六 16:25写道：
>
> On Wed, Aug 06, 2025 at 02:05:03AM +0000, ChenMiao wrote:
> > From: chenmiao <chenmiao...@gmail.com>
> >
> > We need a text patching mechanism to ensure that in the subsequent
> > implementation of jump_label, the code can be modified to the correct
> > location. Therefore, FIX_TEXT_POKE0 has been added as a mapping area.
> >
> > And, I create a new file named insn-def.h to define the or1k insn macro
> > size and more define in the future.
> >
> > Among these changes, we implement patch_map and support the
> > patch_insn_write API for single instruction writing.
> >
> > - V2: We modify the patch_insn_write(void *addr, const void *insn) API to
> > patch_insn_write(void *addr, u32 insn), derectly support a single u32
> > instruction write to map memory.
> >
> > Link: https://lore.kernel.org/openrisc/aJIC8o1WmVHol9RY@antec/T/#t
> >
> > Signed-off-by: chenmiao <chenmiao...@gmail.com>
> > ---
>
> The v2, notes should go in this part of the patch (after ---), this way
> it does not show up when I queue the patches.
well, I'll be care of the next version
>
> > arch/openrisc/include/asm/Kbuild | 1 -
> > arch/openrisc/include/asm/fixmap.h | 1 +
> > arch/openrisc/include/asm/insn-def.h | 12 ++++
> > arch/openrisc/include/asm/text-patching.h | 13 ++++
> > arch/openrisc/kernel/Makefile | 1 +
> > arch/openrisc/kernel/patching.c | 79 +++++++++++++++++++++++
> > arch/openrisc/mm/init.c | 2 +-
> > 7 files changed, 107 insertions(+), 2 deletions(-)
> > create mode 100644 arch/openrisc/include/asm/insn-def.h
> > create mode 100644 arch/openrisc/include/asm/text-patching.h
> > create mode 100644 arch/openrisc/kernel/patching.c
> >
> > diff --git a/arch/openrisc/include/asm/Kbuild
> > b/arch/openrisc/include/asm/Kbuild
> > index 2b1a6b00cdac..cef49d60d74c 100644
> > --- a/arch/openrisc/include/asm/Kbuild
> > +++ b/arch/openrisc/include/asm/Kbuild
> > @@ -9,4 +9,3 @@ generic-y += spinlock.h
> > generic-y += qrwlock_types.h
> > generic-y += qrwlock.h
> > generic-y += user.h
> > -generic-y += text-patching.h
> > diff --git a/arch/openrisc/include/asm/fixmap.h
> > b/arch/openrisc/include/asm/fixmap.h
> > index aaa6a26a3e92..74000215064d 100644
> > --- a/arch/openrisc/include/asm/fixmap.h
> > +++ b/arch/openrisc/include/asm/fixmap.h
> > @@ -28,6 +28,7 @@
> >
> > enum fixed_addresses {
> > FIX_EARLYCON_MEM_BASE,
> > + FIX_TEXT_POKE0,
> > __end_of_fixed_addresses
> > };
> >
> > diff --git a/arch/openrisc/include/asm/insn-def.h
> > b/arch/openrisc/include/asm/insn-def.h
> > new file mode 100644
> > index 000000000000..dc8d16db1579
> > --- /dev/null
> > +++ b/arch/openrisc/include/asm/insn-def.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2025 Chen Miao
> > + */
> > +
> > +#ifndef __ASM_INSN_DEF_H
> > +#define __ASM_INSN_DEF_H
> > +
> > +/* or1k instructions are always 32 bits. */
> > +#define OPENRISC_INSN_SIZE 4
>
> Is this used outside of the usage below? Why do we need a header for it?
I think we need the header, although it seems redundant. Although we
only added one line here, I think as development progresses, we might
need more macro definitions related to the instruction set, possibly
including jump_label, ftrace, and potentially bpf.
>
> > +#endif /* __ASM_INSN_DEF_H */
> > diff --git a/arch/openrisc/include/asm/text-patching.h
> > b/arch/openrisc/include/asm/text-patching.h
> > new file mode 100644
> > index 000000000000..bffe828288c3
> > --- /dev/null
> > +++ b/arch/openrisc/include/asm/text-patching.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2025 Chen Miao
> > + */
> > +
> > +#ifndef _ASM_PATCHING_H_
> > +#define _ASM_PATCHING_H_
> > +
> > +#include <linux/types.h>
> > +
> > +int patch_insn_write(void *addr, u32 insn);
> > +
> > +#endif /* _ASM_PATCHING_H_ */
> > diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
> > index 58e6a1b525b7..f0957ce16d6b 100644
> > --- a/arch/openrisc/kernel/Makefile
> > +++ b/arch/openrisc/kernel/Makefile
> > @@ -13,5 +13,6 @@ obj-$(CONFIG_SMP) += smp.o sync-timer.o
> > obj-$(CONFIG_STACKTRACE) += stacktrace.o
> > obj-$(CONFIG_MODULES) += module.o
> > obj-$(CONFIG_OF) += prom.o
> > +obj-y += patching.o
> >
> > clean:
> > diff --git a/arch/openrisc/kernel/patching.c
> > b/arch/openrisc/kernel/patching.c
> > new file mode 100644
> > index 000000000000..c9a30f0d1193
> > --- /dev/null
> > +++ b/arch/openrisc/kernel/patching.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (C) 2020 SiFive
> > + * Copyright (C) 2025 Chen Miao
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/uaccess.h>
> > +
> > +#include <asm/insn-def.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/page.h>
> > +#include <asm/fixmap.h>
> > +#include <asm/text-patching.h>
> > +#include <asm/sections.h>
> > +
> > +static DEFINE_RAW_SPINLOCK(patch_lock);
> > +
> > +static inline bool is_exit_text(uintptr_t addr)
> > +{
> > + /* Now Have NO Mechanism to do */
> > + return true;
> > +}
>
> Is this needed? OpenRISC doesn't seem to have any sections to support __exit
> annotated cleanup functions.
>
> Ref: https://kernelnewbies.org/FAQ/InitExitMacros
>
> Returning true here means that all of the patch_map calls will be treated as
> kernel pages below though, which is wrong, please remove this.
>
I'll remove it.
> > +
> > +static __always_inline void *patch_map(void *addr, int fixmap)
> > +{
> > + uintptr_t uaddr = (uintptr_t) addr;
> > + phys_addr_t phys;
> > +
> > + if (core_kernel_text(uaddr) || is_exit_text(uaddr)) {
> > + phys = __pa_symbol(addr);
> > + } else {
> > + struct page *page = vmalloc_to_page(addr);
> > + BUG_ON(!page);
> > + phys = page_to_phys(page) + offset_in_page(addr);
> > + }
> > +
> > + return (void *)set_fixmap_offset(fixmap, phys);
> > +}
> > +
> > +static void patch_unmap(int fixmap)
> > +{
> > + clear_fixmap(fixmap);
> > +}
> > +
> > +static int __patch_insn_write(void *addr, u32 insn)
> > +{
> > + void *waddr = addr;
> > + unsigned long flags = 0;
> > + int ret;
> > +
> > + raw_spin_lock_irqsave(&patch_lock, flags);
> > +
> > + waddr = patch_map(addr, FIX_TEXT_POKE0);
> > +
> > + ret = copy_to_kernel_nofault(waddr, &insn, OPENRISC_INSN_SIZE);
> > + local_icache_range_inv((unsigned long)waddr,
> > + (unsigned long)waddr + OPENRISC_INSN_SIZE);
>
> Instead of OPENRISC_INSN_SIZE, can you use sizeof(insn)?
I think not using sizeof(insn)here would be better. In fact, using the
OPENRISC_INSN_SIZE macro might be more appropriate.
Here we are modifying a single instruction, so it must be 4 bytes—the
length of one OPENRISC INSN.
>
> > + patch_unmap(FIX_TEXT_POKE0);
> > +
> > + raw_spin_unlock_irqrestore(&patch_lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +int patch_insn_write(void *addr, u32 insn)
> > +{
> > + u32 *tp = addr;
> > + int ret;
> > +
> > + if ((uintptr_t) tp & 0x3)
> > + return -EINVAL;
> > +
> > + ret = __patch_insn_write(tp, insn);
> > +
> > + return ret;
> > +}
> > diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
> > index e4904ca6f0a0..b5925710f954 100644
> > --- a/arch/openrisc/mm/init.c
> > +++ b/arch/openrisc/mm/init.c
> > @@ -226,7 +226,7 @@ static int __init map_page(unsigned long va,
> > phys_addr_t pa, pgprot_t prot)
> > return 0;
> > }
> >
> > -void __init __set_fixmap(enum fixed_addresses idx,
> > +void __set_fixmap(enum fixed_addresses idx,
>
> Can you put a comment about this bit in the commit description?
I'll do the next version
>
> > phys_addr_t phys, pgprot_t prot)
> > {
> > unsigned long address = __fix_to_virt(idx);
> > --
> > 2.45.2
>
> -Stafford
>
