Re: [kvm-unit-tests PATCH 2/5] powerpc: add test to check invalid instruction trap

[Thomas Huth <thuth@redhat.com>]

On 16.03.2016 16:13, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  powerpc/Makefile.common |  5 ++++-
>  powerpc/emulator.c      | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |  3 +++
>  3 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100644 powerpc/emulator.c
> 
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index ab2caf6..257e3fb 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -7,7 +7,8 @@
>  tests-common = \
>  	$(TEST_DIR)/selftest.elf \
>  	$(TEST_DIR)/spapr_hcall.elf \
> -	$(TEST_DIR)/rtas.elf
> +	$(TEST_DIR)/rtas.elf \
> +	$(TEST_DIR)/emulator.elf
>  
>  all: $(TEST_DIR)/boot_rom.bin test_cases
>  
> @@ -70,3 +71,5 @@ $(TEST_DIR)/selftest.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/selftest.o
>  $(TEST_DIR)/spapr_hcall.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/spapr_hcall.o
>  
>  $(TEST_DIR)/rtas.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/rtas.o
> +
> +$(TEST_DIR)/emulator.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/emulator.o
> diff --git a/powerpc/emulator.c b/powerpc/emulator.c
> new file mode 100644
> index 0000000..1215c4f
> --- /dev/null
> +++ b/powerpc/emulator.c
> @@ -0,0 +1,46 @@
> +/*
> + * Test some powerpc instructions
> + */
> +
> +#include <libcflat.h>
> +#include <asm/processor.h>
> +
> +static int volatile is_invalid;
> +
> +static void program_check_handler(struct pt_regs *regs, void *opaque)
> +{
> +	int *data = opaque;
> +
> +	printf("Detected invalid instruction 0x%016lx: %08x\n",
> +	       regs->nip, *(uint32_t*)regs->nip);

Should the test always print this, or is this rather confusing for the
users? Then it should maybe be commented out by default, I think.

> +	*data = 1;
> +
> +	regs->nip += 4;
> +}
> +
> +static void test_illegal(void)
> +{
> +	report_prefix_push("invalid");
> +
> +	is_invalid = 0;
> +
> +	asm volatile (".long 0");
> +
> +	report("exception", is_invalid);
> +
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	handle_exception(0x700, program_check_handler, (void *)&is_invalid);
> +
> +	report_prefix_push("emulator");
> +
> +	test_illegal();
> +
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index 02b21c7..ed4fdbe 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -47,3 +47,6 @@ file = rtas.elf
>  extra_params = -append "set-time-of-day"
>  timeout = 5
>  groups = rtas
> +
> +[emulator]
> +file = emulator.elf

Reviewed-by: Thomas Huth <thuth@redhat.com>

BTW: I think you could optionally also check the contents of SRR1 for
sane values in test_illegal(), in case you want to improve the test a
little bit.

 Thomas