Sam Ravnborg wrote:
> On Thu, Jul 16, 2009 at 11:57:06AM -0400, Masami Hiramatsu wrote:
>> Add a user-space selftest of x86 instruction decoder at kernel build time.
>> When CONFIG_X86_DECODER_SELFTEST=y, Kbuild builds a test harness of x86
>> instruction decoder and performs it after building vmlinux.
>> The test compares the results of objdump and x86 instruction decoder
>> code and check there are no differences.
>
> Long overdue review from my side...
>
>> arch/x86/scripts/Makefile | 19 +++++++
>> arch/x86/scripts/distill.awk | 42 +++++++++++++++++
>> arch/x86/scripts/test_get_len.c | 99 +++++++++++++++++++++++++++++++++++++++
>> arch/x86/scripts/user_include.h | 49 +++++++++++++++++++
>
> Hmmm, we have two architectures that uses scripts/ and three that
> uses tools/.
> I prefer the latter name as what we have ere is beyound what
> I generally recognize as a script.
>
> we have scripts/ in top-level and we do not rename this
> as we have this hardcoded too many places - but no reason to
> use the wrong name here.
>
>> diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
>> index 01e079a..9090665 100644
>> --- a/arch/x86/include/asm/inat.h
>> +++ b/arch/x86/include/asm/inat.h
>> @@ -20,7 +20,9 @@
>> * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
>> *
>> */
>> +#ifdef __KERNEL__
>> #include <linux/types.h>
>> +#endif
>>
>> /* Instruction attributes */
>> typedef u32 insn_attr_t;
>
> Why this?
> If you need this to use this file from userspace then could we do some
> other trick to make this OK?
>
> I see it repeated several times below.
> [If this has already been discussed I have missed it - sorry].
>
>
>> diff --git a/arch/x86/scripts/Makefile b/arch/x86/scripts/Makefile
>> new file mode 100644
>> index 0000000..f08859e
>> --- /dev/null
>> +++ b/arch/x86/scripts/Makefile
>> @@ -0,0 +1,19 @@
>> +PHONY += posttest
>> +quiet_cmd_posttest = TEST $@
>> + cmd_posttest = objdump -d $(objtree)/vmlinux | awk -f $(srctree)/arch/x86/scripts/distill.awk | $(obj)/test_get_len
>> +
>
> You are using the native objdump here.
> But I assume this fails miserably when you build x86 on a powerpc host.
> In other words - you broke an allyesconfig build for -next...
> We have $(OBJDUMP) for this.
Ah, I see... Would you know actual name of x86-objdump on the powerpc
(or any other crosscompiling host)? I just set "OBJDUMP=objdump" is OK?
I'm not so sure about cross-compiling kernel...
>> +posttest: $(obj)/test_get_len vmlinux
>> + $(call cmd,posttest)
>> +
>> +test_get_len_SRC = $(srctree)/arch/x86/scripts/test_get_len.c $(srctree)/arch/x86/lib/insn.c $(srctree)/arch/x86/lib/inat.c
>> +test_get_len_INC = $(srctree)/arch/x86/include/asm/inat.h $(srctree)/arch/x86/include/asm/insn.h $(objtree)/arch/x86/lib/inat-tables.c
>> +
>> +quiet_cmd_test_get_len = CC $@
>> + cmd_test_get_len = $(CC) -Wall $(test_get_len_SRC) -I$(objtree)/arch/x86/lib/ -I$(srctree)/arch/x86/include -include $(srctree)/arch/x86/scripts/user_include.h -o $@
>
> Is there a specific reason why you cannot use the standard hostprogs-y for this?
> It will take care of dependency tracking etc.
> What you have above is a hopeless incomplete list of dependencies.
>
> You need to use HOST_EXTRACFLAGS to set additional -I options and the -include.
Thank you, I'll try to use hostprogs-y.
>> +
>> +static void usage()
>> +{
>> + fprintf(stderr, "usage: %s < distilled_disassembly\n", prog);
>> + exit(1);
>> +}
>
> It would be nice to tell the user what the program is supposed to do.
> I know this is a bit unusual but no reason to copy bad practice.
>
Sure, maybe copying usage line in distill.awk is more helpful for user...
Thank you,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division
e-mail: mhiramat@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
