|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 1/7] xen/arm: Introduce alternative runtime patching
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> new file mode 100644
> index 0000000..d00f98e
> --- /dev/null
> +++ b/xen/arch/arm/alternative.c
Hey!
I've some comments, most of them are light. What I am concerned most
is the difference between comments in the header vs the code.
Looking at Linux code it seems to have the same issue - and I know you don't
want to differ to much from Linux.
Perhaps go with the changes as is (With the comments being off) and the
then have some patches for Linux for the header mismatches and those
can be backported?
..snip..
> +static int __apply_alternatives(const struct alt_region *region)
> +{
> + const struct alt_instr *alt;
> + const u32 *origptr, *replptr;
> + u32 *writeptr, *writemap;
> + mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
> + unsigned int text_order = get_order_from_bytes(_end - _start);
> +
> + printk("Patching kernel code\n");
I see you use prefixed later with 'alternatives:' and also this is without
any prefix (DEBUG, or ERR). Would it make sense to have this one above
be dprintk while the one below with printk (KERN_ERR) ?
> +
> + /*
> + * The text section is read-only. So re-map Xen to be able to patch
> + * the code.
> + */
> + writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> + VMAP_DEFAULT);
> + if ( !writemap )
> + {
> + printk("alternatives: Unable to map the text section\n");
Perhaps include the size of the text section? That may help in figuring
out what went wrong or if one could increase the VMAP area?
> + return -ENOMEM;
> + }
> +
> + for ( alt = region->begin; alt < region->end; alt++ )
> + {
> + u32 insn;
> + int i, nr_inst;
> +
> + if ( !cpus_have_cap(alt->cpufeature) )
> + continue;
> +
> + BUG_ON(alt->alt_len != alt->orig_len);
The header file says:
+ u8 alt_len; /* size of new instruction(s), <= orig_len */
So this BUG_ON seems incorrect?
But hten later in the header it says:
"1. Be exactly the same length (in bytes) as the default code
+ * sequence."
So the BUG_ON is correct. Perhaps the 'alt_len' comment should be s/<=/==/ ?
> +
> + origptr = ALT_ORIG_PTR(alt);
> + writeptr = origptr - (u32 *)_start + writemap;
How about just using writeptr += ?
Ah wait. You are trying to preserve the Linux code!. Nevermind then.
> + replptr = ALT_REPL_PTR(alt);
> +
> + nr_inst = alt->alt_len / sizeof(insn);
> +
> + for ( i = 0; i < nr_inst; i++ )
> + {
> + insn = get_alt_insn(alt, origptr + i, replptr + i);
> + *(writeptr + i) = cpu_to_le32(insn);
> + }
> +
> + /* Ensure the new instructions reached the memory and nuke */
> + clean_and_invalidate_dcache_va_range(writeptr,
> + (sizeof (*writeptr) * nr_inst));
> + }
> +
> + /* Nuke the instruction cache */
> + invalidate_icache();
> +
> + vunmap(writemap);
> +
> + return 0;
> +}
> +
> +/*
> + * We might be patching the stop_machine state machine, so implement a
> + * really simple polling protocol here.
> + */
> +static int __apply_alternatives_multi_stop(void *unused)
> +{
> + static int patched = 0;
Shouldn't this be 'atomic_t' ?
> + struct alt_region region = {
Can this 'const' ?
> + .begin = __alt_instructions,
> + .end = __alt_instructions_end,
> + };
> +
> + /* We always have a CPU 0 at this point (__init) */
> + if ( smp_processor_id() )
> + {
> + while ( !read_atomic(&patched) )
> + cpu_relax();
> + isb();
> + }
> + else
> + {
> + int ret;
> +
> + BUG_ON(patched);
> + ret = __apply_alternatives(®ion);
> + /* The patching is not expected to fail during boot. */
> + BUG_ON(ret != 0);
> +
> + /* Barriers provided by the cache flushing */
> + write_atomic(&patched, 1);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * This function should only be called during boot and before CPU0 jump
> + * into the idle_loop.
So could you add an:
> + */
> +void __init apply_alternatives_all(void)
> +{
> + int ret;
> +
ASSERT(system_state != SYS_STATE_active);
?
> + /* better not try code patching on a live SMP system */
> + ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
> +
> + /* stop_machine_run should never fail at this stage of the boot */
> + BUG_ON(ret);
> +}
> +
> +int apply_alternatives(void *start, size_t length)
> +{
> + struct alt_region region = {
Could this be 'const'?
> + .begin = start,
> + .end = start + length,
> + };
> +
> + return __apply_alternatives(®ion);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 46cf6dd..97b3214 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -38,6 +38,7 @@
> #include <xen/vmap.h>
> #include <xen/libfdt/libfdt.h>
> #include <xen/acpi.h>
> +#include <asm/alternative.h>
> #include <asm/page.h>
> #include <asm/current.h>
> #include <asm/setup.h>
> @@ -835,6 +836,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>
> do_initcalls();
>
> + /*
> + * It needs to be called after do_initcalls to be able to use
> + * stop_machine (tasklets initialized via an initcall).
> + */
> + apply_alternatives_all();
> +
> /* Create initial domain 0. */
> /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index b18c9c2..b24e93b 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -151,6 +151,15 @@ SECTIONS
> *(.initcall1.init)
> __initcall_end = .;
>
> +#ifdef CONFIG_ALTERNATIVE
> + . = ALIGN(4);
> + __alt_instructions = .;
> + *(.altinstructions)
> + __alt_instructions_end = .;
> + . = ALIGN(4);
> + *(.altinstr_replacement)
> +#endif
> +
> *(.init.data)
> *(.init.data.rel)
> *(.init.data.rel.*)
> diff --git a/xen/include/asm-arm/alternative.h
> b/xen/include/asm-arm/alternative.h
> new file mode 100644
> index 0000000..4287bac
> --- /dev/null
> +++ b/xen/include/asm-arm/alternative.h
> @@ -0,0 +1,165 @@
> +#ifndef __ASM_ALTERNATIVE_H
> +#define __ASM_ALTERNATIVE_H
> +
> +#include <asm/cpufeature.h>
> +#include <xen/config.h>
> +#include <xen/kconfig.h>
> +
> +#ifdef CONFIG_ALTERNATIVE
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/init.h>
> +#include <xen/types.h>
> +#include <xen/stringify.h>
> +
> +struct alt_instr {
> + s32 orig_offset; /* offset to original instruction */
> + s32 alt_offset; /* offset to replacement instruction */
> + u16 cpufeature; /* cpufeature bit set for replacement */
> + u8 orig_len; /* size of original instruction(s) */
> + u8 alt_len; /* size of new instruction(s), <= orig_len */
Perhaps s/<=/==/?
> +};
> +
> +void __init apply_alternatives_all(void);
> +int apply_alternatives(void *start, size_t length);
> +
> +#define ALTINSTR_ENTRY(feature)
> \
> + " .word 661b - .\n" /* label */ \
> + " .word 663f - .\n" /* new instruction */ \
> + " .hword " __stringify(feature) "\n" /* feature bit */ \
> + " .byte 662b-661b\n" /* source len */ \
> + " .byte 664f-663f\n" /* replacement len */
> +
> +/*
> + * alternative assembly primitive:
> + *
> + * If any of these .org directive fail, it means that insn1 and insn2
> + * don't have the same length. This used to be written as
> + *
> + * .if ((664b-663b) != (662b-661b))
> + * .error "Alternatives instruction length mismatch"
> + * .endif
> + *
> + * but most assemblers die if insn1 or insn2 have a .inst. This should
> + * be fixed in a binutils release posterior to 2.25.51.0.2 (anything
> + * containing commit 4e4d08cf7399b606 or c1baaddf8861).
> + */
> +#define __ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg_enabled) \
> + ".if "__stringify(cfg_enabled)" == 1\n" \
> + "661:\n\t" \
> + oldinstr "\n" \
> + "662:\n" \
> + ".pushsection .altinstructions,\"a\"\n" \
> + ALTINSTR_ENTRY(feature) \
> + ".popsection\n" \
> + ".pushsection .altinstr_replacement, \"a\"\n" \
> + "663:\n\t" \
> + newinstr "\n" \
> + "664:\n\t" \
> + ".popsection\n\t" \
> + ".org . - (664b-663b) + (662b-661b)\n\t" \
> + ".org . - (662b-661b) + (664b-663b)\n" \
> + ".endif\n"
> +
> +#define _ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg, ...) \
> + __ALTERNATIVE_CFG(oldinstr, newinstr, feature, IS_ENABLED(cfg))
> +
> +#else
> +
> +#include <asm/asm_defns.h>
> +
> +.macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
> + .word \orig_offset - .
> + .word \alt_offset - .
> + .hword \feature
> + .byte \orig_len
> + .byte \alt_len
> +.endm
> +
> +.macro alternative_insn insn1, insn2, cap, enable = 1
> + .if \enable
> +661: \insn1
> +662: .pushsection .altinstructions, "a"
> + altinstruction_entry 661b, 663f, \cap, 662b-661b, 664f-663f
> + .popsection
> + .pushsection .altinstr_replacement, "ax"
> +663: \insn2
> +664: .popsection
> + .org . - (664b-663b) + (662b-661b)
> + .org . - (662b-661b) + (664b-663b)
> + .endif
> +.endm
> +
> +/*
> + * Begin an alternative code sequence.
> + *
> + * The code that follows this macro will be assembled and linked as
> + * normal. There are no restrictions on this code.
> + */
> +.macro alternative_if_not cap, enable = 1
> + .if \enable
> + .pushsection .altinstructions, "a"
> + altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
> + .popsection
> +661:
> + .endif
> +.endm
> +
> +/*
> + * Provide the alternative code sequence.
> + *
> + * The code that follows this macro is assembled into a special
> + * section to be used for dynamic patching. Code that follows this
> + * macro must:
> + *
> + * 1. Be exactly the same length (in bytes) as the default code
> + * sequence.
> + *
> + * 2. Not contain a branch target that is used outside of the
> + * alternative sequence it is defined in (branches into an
> + * alternative sequence are not fixed up).
Hm,? But get_alt_insn seems to take care of fixing this up:
/*
+ * If we're branching inside the alternate sequence,
+ * do not rewrite the instruction, as it is already
+ * correct. Otherwise, generate the new instruction.
+ */
?
> + */
> +.macro alternative_else
> +662: .pushsection .altinstr_replacement, "ax"
> +663:
> +.endm
> +
> +/*
> + * Complete an alternative code sequence.
> + */
> +.macro alternative_endif
> +664: .popsection
> + .org . - (664b-663b) + (662b-661b)
> + .org . - (662b-661b) + (664b-663b)
> +.endm
> +
> +#define _ALTERNATIVE_CFG(insn1, insn2, cap, cfg, ...) \
> + alternative_insn insn1, insn2, cap, IS_ENABLED(cfg)
> +
> +#endif /* __ASSEMBLY__ */
> +
> +/*
> + * Usage: asm(ALTERNATIVE(oldinstr, newinstr, feature));
> + *
> + * Usage: asm(ALTERNATIVE(oldinstr, newinstr, feature, CONFIG_FOO));
> + * N.B. If CONFIG_FOO is specified, but not selected, the whole block
> + * will be omitted, including oldinstr.
> + */
> +#define ALTERNATIVE(oldinstr, newinstr, ...) \
> + _ALTERNATIVE_CFG(oldinstr, newinstr, __VA_ARGS__, 1)
> +
> +#else /* !CONFIG_ALTERNATIVE */
> +
> +static inline void apply_alternatives_all(void)
> +{
> +}
> +
> +static inline int apply_alternatives(void *start, size_t lenght)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> +#endif /* __ASM_ALTERNATIVE_H */
This being a new file perhaps add:
*
* Local variables:
* mode: C
* c-file-style: "BSD"
* c-basic-offset: 4
* indent-tabs-mode: nil
* End:
*/
?
> diff --git a/xen/include/asm-arm/arm64/insn.h
> b/xen/include/asm-arm/arm64/insn.h
> index cfcdbe9..6ce37be 100644
> --- a/xen/include/asm-arm/arm64/insn.h
> +++ b/xen/include/asm-arm/arm64/insn.h
> @@ -61,6 +61,22 @@ u32 aarch64_insn_encode_immediate(enum
> aarch64_insn_imm_type type,
> s32 aarch64_get_branch_offset(u32 insn);
> u32 aarch64_set_branch_offset(u32 insn, s32 offset);
>
> +/* Wrapper for common code */
> +static inline bool insn_is_branch_imm(u32 insn)
> +{
> + return aarch64_insn_is_branch_imm(insn);
> +}
> +
> +static inline s32 insn_get_branch_offset(u32 insn)
> +{
> + return aarch64_get_branch_offset(insn);
> +}
> +
> +static inline u32 insn_set_branch_offset(u32 insn, s32 offset)
> +{
> + return aarch64_set_branch_offset(insn, offset);
> +}
> +
> #endif /* !__ARCH_ARM_ARM64_INSN */
> /*
> * Local variables:
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |