Re: [kvm-unit-tests PATCH 12/32] powerpc: Fix emulator illegal instruction test for powernv

[Thomas Huth <thuth@xxxxxxxxxx>]

On 26/02/2024 11.11, Nicholas Piggin wrote:
> Illegal instructions cause 0xe40 (HEAI) interrupts rather
> than program interrupts.
>
> Acked-by: Thomas Huth <thuth@xxxxxxxxxx>
> Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
> ---
>   lib/powerpc/asm/processor.h |  1 +
>   lib/powerpc/setup.c         | 13 +++++++++++++
>   powerpc/emulator.c          | 21 ++++++++++++++++++++-
>   3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> index 9d8061962..cf1b9d8ff 100644
> --- a/lib/powerpc/asm/processor.h
> +++ b/lib/powerpc/asm/processor.h
> @@ -11,6 +11,7 @@ void do_handle_exception(struct pt_regs *regs);
>   #endif /* __ASSEMBLY__ */
>   
>   extern bool cpu_has_hv;
> +extern bool cpu_has_heai;
>   
>   static inline uint64_t mfspr(int nr)
>   {
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index 89e5157f2..3c81aee9e 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -87,6 +87,7 @@ static void cpu_set(int fdtnode, u64 regval, void *info)
>   }
>   
>   bool cpu_has_hv;
> +bool cpu_has_heai;
>   
>   static void cpu_init(void)
>   {
> @@ -108,6 +109,18 @@ static void cpu_init(void)
>   		hcall(H_SET_MODE, 0, 4, 0, 0);
>   #endif
>   	}
> +
> +	switch (mfspr(SPR_PVR) & PVR_VERSION_MASK) {
> +	case PVR_VER_POWER10:
> +	case PVR_VER_POWER9:
> +	case PVR_VER_POWER8E:
> +	case PVR_VER_POWER8NVL:
> +	case PVR_VER_POWER8:
> +		cpu_has_heai = true;
> +		break;
> +	default:
> +		break;
> +	}
>   }
>   
>   static void mem_init(phys_addr_t freemem_start)
> diff --git a/powerpc/emulator.c b/powerpc/emulator.c
> index 39dd59645..c9b17f742 100644
> --- a/powerpc/emulator.c
> +++ b/powerpc/emulator.c
> @@ -31,6 +31,20 @@ static void program_check_handler(struct pt_regs *regs, void *opaque)
>   	regs->nip += 4;
>   }
>   
> +static void heai_handler(struct pt_regs *regs, void *opaque)
> +{
> +	int *data = opaque;
> +
> +	if (verbose) {
> +		printf("Detected invalid instruction %#018lx: %08x\n",
> +		       regs->nip, *(uint32_t*)regs->nip);
> +	}
> +
> +	*data = 8; /* Illegal instruction */
> +
> +	regs->nip += 4;
> +}
> +
>   static void alignment_handler(struct pt_regs *regs, void *opaque)
>   {
>   	int *data = opaque;
> @@ -362,7 +376,12 @@ int main(int argc, char **argv)
>   {
>   	int i;
>   
> -	handle_exception(0x700, program_check_handler, (void *)&is_invalid);
> +	if (cpu_has_heai) {
> +		handle_exception(0xe40, heai_handler, (void *)&is_invalid);
> +		handle_exception(0x700, program_check_handler, (void *)&is_invalid);
> +	} else {
> +		handle_exception(0x700, program_check_handler, (void *)&is_invalid);

The 0x700 line looks identical to the other part of the if-statement ... I'd 
suggest to leave it outside of the if-statement, drop the else-part and just 
set 0xe40 if cpu_has_heai.

 Thomas

> +	}
>   	handle_exception(0x600, alignment_handler, (void *)&alignment);
>   
>   	for (i = 1; i < argc; i++) {