Messages in this thread | | | Date | Wed, 15 Jan 2014 17:18:01 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v6 3/8] x86: add generic function to modify more calls using int3 framework |
| |
(2014/01/15 9:33), Steven Rostedt wrote: > On Tue, 10 Dec 2013 16:42:15 +0100 > Petr Mladek <pmladek@suse.cz> wrote: > >> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h >> index 586747f5f41d..82ffe7e1529c 100644 >> --- a/arch/x86/include/asm/alternative.h >> +++ b/arch/x86/include/asm/alternative.h >> @@ -232,4 +232,40 @@ extern int text_poke_bp(void *addr, const void *opcode, size_t len, >> extern void text_poke_bp_or_die(void *addr, const void *opcode, size_t len, >> void *handler); > > Small nit. If you can, place comments on the same line as the > structure field. > >> +struct text_poke_bp_iter { >> + /* information used to start iteration from the beginning */ >> + void *init; >> + /* length of the patched instruction */ >> + size_t len; >> + /* details about failure if any */ >> + int fail_count; >> + void *fail_addr; > > The above should have the comments on the same line as the field. > Something like this: > > void *init; /* information used to start > iteration from the beginning */ > > The comments for the function pointers below are fine. > >> + /* iteration over entries */ >> + void *(*start)(void *init); >> + void *(*next)(void *prev); >> + /* callback to get patched address */ >> + void *(*get_addr)(void *cur); >> + /* >> + * Callbacks to get the patched code. They could return NULL if no >> + * patching is needed; This is useful for example in ftrace. >> + */ >> + const void *(*get_opcode)(void *cur); >> + const void *(*get_old_opcode)(void *cur); >> + /* >> + * Optional function that is called when the patching of the given >> + * has finished. It might be NULL if no postprocess is needed. >> + */ >> + int (*finish)(void *cur); >> + /* >> + * Helper function for int3 handler. It decides whether the given IP >> + * is being patched or not. >> + * >> + * Try to implement it as fast as possible. It affects performance >> + * of the system when the patching is in progress. >> + */ >> + void *(*is_handled)(const unsigned long ip); >> +}; >> + >> +extern int text_poke_bp_list(struct text_poke_bp_iter *iter); >> + >> #endif /* _ASM_X86_ALTERNATIVE_H */ >> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c >> index 6436beec7b0c..8e57ac03a0e8 100644 >> --- a/arch/x86/kernel/alternative.c >> +++ b/arch/x86/kernel/alternative.c >> @@ -7,6 +7,7 @@ >> #include <linux/stringify.h> >> #include <linux/kprobes.h> >> #include <linux/mm.h> >> +#include <linux/uaccess.h> >> #include <linux/vmalloc.h> >> #include <linux/memory.h> >> #include <linux/stop_machine.h> >> @@ -610,8 +611,11 @@ static void run_sync(void) >> on_each_cpu(do_sync_core, NULL, 1); >> } >> >> +static char bp_int3; > > bp_int3 is not going to be anything but 0xcc. Let's change that to: > > static char bp_int3 = 0xcc; > > And remove the other initializations.
just a comment. If it is always 0xcc, it should be a const variable.
Thank you,
-- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com
| |