Re: [kvm-unit-tests PATCH v5 24/29] powerpc: interrupt tests

[Thomas Huth <thuth@redhat.com>]

On 16/12/2023 14.42, Nicholas Piggin wrote:
> Add basic testing of various kinds of interrupts, machine check,
> page fault, illegal, decrementer, trace, syscall, etc.
> 
> This has a known failure on QEMU TCG pseries machines where MSR[ME]
> can be incorrectly set to 0.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   lib/powerpc/asm/ppc_asm.h |  21 +-
>   powerpc/Makefile.common   |   3 +-
>   powerpc/interrupts.c      | 422 ++++++++++++++++++++++++++++++++++++++
>   powerpc/unittests.cfg     |   3 +
>   4 files changed, 445 insertions(+), 4 deletions(-)
>   create mode 100644 powerpc/interrupts.c
> 
> diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h
> index ef2d91dd..778e78ee 100644
> --- a/lib/powerpc/asm/ppc_asm.h
> +++ b/lib/powerpc/asm/ppc_asm.h
> @@ -35,17 +35,32 @@
>   
>   #endif /* __BYTE_ORDER__ */
>   
> +#define SPR_DSISR	0x012
> +#define SPR_DAR		0x013
> +#define SPR_DEC		0x016
> +#define SPR_SRR0	0x01A
> +#define SPR_SRR1	0x01B
> +#define SPR_FSCR	0x099
> +#define   FSCR_PREFIX	0x2000
> +#define SPR_HDEC	0x136
>   #define SPR_HSRR0	0x13A
>   #define SPR_HSRR1	0x13B
> +#define SPR_LPCR	0x13E
> +#define   LPCR_HDICE	0x1UL
> +#define SPR_HEIR	0x153
> +#define SPR_SIAR	0x31C
>   
>   /* Machine State Register definitions: */
>   #define MSR_LE_BIT	0
>   #define MSR_EE_BIT	15			/* External Interrupts Enable */
>   #define MSR_HV_BIT	60			/* Hypervisor mode */
>   #define MSR_SF_BIT	63			/* 64-bit mode */
> -#define MSR_ME		0x1000ULL
>   
> -#define SPR_HSRR0	0x13A
> -#define SPR_HSRR1	0x13B
> +#define MSR_DR		0x0010ULL
> +#define MSR_IR		0x0020ULL
> +#define MSR_BE		0x0200ULL		/* Branch Trace Enable */
> +#define MSR_SE		0x0400ULL		/* Single Step Enable */
> +#define MSR_EE		0x8000ULL
> +#define MSR_ME		0x1000ULL
>   
>   #endif /* _ASMPOWERPC_PPC_ASM_H */
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index a7af225b..b340a53b 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -11,7 +11,8 @@ tests-common = \
>   	$(TEST_DIR)/rtas.elf \
>   	$(TEST_DIR)/emulator.elf \
>   	$(TEST_DIR)/tm.elf \
> -	$(TEST_DIR)/sprs.elf
> +	$(TEST_DIR)/sprs.elf \
> +	$(TEST_DIR)/interrupts.elf
>   
>   tests-all = $(tests-common) $(tests)
>   all: directories $(TEST_DIR)/boot_rom.bin $(tests-all)
> diff --git a/powerpc/interrupts.c b/powerpc/interrupts.c
> new file mode 100644
> index 00000000..3217b15e
> --- /dev/null
> +++ b/powerpc/interrupts.c
> @@ -0,0 +1,422 @@
> +/*
> + * Test interrupts
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <util.h>
> +#include <migrate.h>
> +#include <alloc.h>
> +#include <asm/handlers.h>
> +#include <asm/hcall.h>
> +#include <asm/processor.h>
> +#include <asm/barrier.h>
> +
> +#define SPR_LPCR	0x13E
> +#define LPCR_HDICE	0x1UL
> +#define SPR_DEC		0x016
> +#define SPR_HDEC	0x136
> +
> +#define MSR_DR		0x0010ULL
> +#define MSR_IR		0x0020ULL
> +#define MSR_EE		0x8000ULL
> +#define MSR_ME		0x1000ULL

Why don't you use the definitions from ppc_asm.h above?

> +static bool cpu_has_heir(void)
> +{
> +	uint32_t pvr = mfspr(287);	/* Processor Version Register */
> +
> +	if (!machine_is_powernv())
> +		return false;
> +
> +	/* POWER6 has HEIR, but QEMU powernv support does not go that far */
> +	switch (pvr >> 16) {
> +	case 0x4b:			/* POWER8E */
> +	case 0x4c:			/* POWER8NVL */
> +	case 0x4d:			/* POWER8 */
> +	case 0x4e:			/* POWER9 */
> +	case 0x80:			/* POWER10 */

I'd suggest to introduce some #defines for those PVR values instead of using 
magic numbers all over the place?

> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool cpu_has_prefix(void)
> +{
> +	uint32_t pvr = mfspr(287);	/* Processor Version Register */
> +	switch (pvr >> 16) {
> +	case 0x80:			/* POWER10 */
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool cpu_has_lev_in_srr1(void)
> +{
> +	uint32_t pvr = mfspr(287);	/* Processor Version Register */
> +	switch (pvr >> 16) {
> +	case 0x80:			/* POWER10 */
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool regs_is_prefix(volatile struct pt_regs *regs)
> +{
> +	return (regs->msr >> (63-34)) & 1;

You introduced a bunch of new #define MSR_xx statements ... why not for this 
one, too?

> +}
> +
> +static void regs_advance_insn(struct pt_regs *regs)
> +{
> +	if (regs_is_prefix(regs))
> +		regs->nip += 8;
> +	else
> +		regs->nip += 4;
> +}
> +
> +static volatile bool got_interrupt;
> +static volatile struct pt_regs recorded_regs;
> +
> +static void mce_handler(struct pt_regs *regs, void *opaque)
> +{
> +	got_interrupt = true;
> +	memcpy((void *)&recorded_regs, regs, sizeof(struct pt_regs));
> +	regs_advance_insn(regs);
> +}
> +
> +static void test_mce(void)
> +{
> +	unsigned long addr = -4ULL;
> +	uint8_t tmp;
> +
> +	handle_exception(0x200, mce_handler, NULL);
> +
> +	if (machine_is_powernv()) {
> +		enable_mcheck();
> +	} else {
> +		report(mfmsr() & MSR_ME, "pseries machine has MSR[ME]=1");
> +		if (!(mfmsr() & MSR_ME)) { /* try to fix it */
> +			enable_mcheck();
> +		}
> +		if (mfmsr() & MSR_ME) {
> +			disable_mcheck();
> +			report(mfmsr() & MSR_ME, "pseries is unable to change MSR[ME]");
> +			if (!(mfmsr() & MSR_ME)) { /* try to fix it */
> +				enable_mcheck();
> +			}
> +		}
> +	}
> +
> +	asm volatile("lbz %0,0(%1)" : "=r"(tmp) : "r"(addr));
> +
> +	report(got_interrupt, "MCE on access to invalid real address");
> +	report(mfspr(SPR_DAR) == addr, "MCE sets DAR correctly");
> +	got_interrupt = false;
> +}
> +
> +static void dseg_handler(struct pt_regs *regs, void *data)
> +{
> +	got_interrupt = true;
> +	memcpy((void *)&recorded_regs, regs, sizeof(struct pt_regs));
> +	regs_advance_insn(regs);
> +	regs->msr &= ~MSR_DR;
> +}
> +
> +static void test_dseg(void)
> +{
> +	uint64_t msr, tmp;
> +
> +	report_prefix_push("data segment");
> +
> +	/* Some HV start in radix mode and need 0x300 */
> +	handle_exception(0x300, &dseg_handler, NULL);
> +	handle_exception(0x380, &dseg_handler, NULL);
> +
> +	asm volatile(
> +"		mfmsr	%0		\n \
> +		ori	%0,%0,%2	\n \
> +		mtmsrd	%0		\n \
> +		lbz	%1,0(0)		"
> +		: "=r"(msr), "=r"(tmp) : "i"(MSR_DR): "memory");
> +
> +	report(got_interrupt, "interrupt on NULL dereference");
> +	got_interrupt = false;
> +
> +	handle_exception(0x300, NULL, NULL);
> +	handle_exception(0x380, NULL, NULL);
> +
> +	report_prefix_pop();
> +}
> +
> +static void dec_handler(struct pt_regs *regs, void *data)
> +{
> +	got_interrupt = true;
> +	memcpy((void *)&recorded_regs, regs, sizeof(struct pt_regs));
> +	regs->msr &= ~MSR_EE;
> +}
> +
> +static void test_dec(void)
> +{
> +	uint64_t msr;
> +
> +	report_prefix_push("decrementer");
> +
> +	handle_exception(0x900, &dec_handler, NULL);
> +
> +	asm volatile(
> +"		mtdec	%1		\n \
> +		mfmsr	%0		\n \
> +		ori	%0,%0,%2	\n \
> +		mtmsrd	%0,1		"
> +		: "=r"(msr) : "r"(10000), "i"(MSR_EE): "memory");
> +
> +	while (!got_interrupt)
> +		;

Maybe add a timeout (in case the interrupt never fires)?

> +	report(got_interrupt, "interrupt on decrementer underflow");
> +	got_interrupt = false;
> +
> +	handle_exception(0x900, NULL, NULL);
> +
> +	if (!machine_is_powernv())
> +		goto done;
> +
> +	handle_exception(0x980, &dec_handler, NULL);
> +
> +	mtspr(SPR_LPCR, mfspr(SPR_LPCR) | LPCR_HDICE);
> +	asm volatile(
> +"		mtspr	0x136,%1	\n \
> +		mtdec	%3		\n \
> +		mfmsr	%0		\n \
> +		ori	%0,%0,%2	\n \
> +		mtmsrd	%0,1		"
> +		: "=r"(msr) : "r"(10000), "i"(MSR_EE), "r"(0x7fffffff): "memory");
> +
> +	while (!got_interrupt)
> +		;

dito?

> +	mtspr(SPR_LPCR, mfspr(SPR_LPCR) & ~LPCR_HDICE);
> +
> +	report(got_interrupt, "interrupt on hdecrementer underflow");
> +	got_interrupt = false;
> +
> +	handle_exception(0x980, NULL, NULL);
> +
> +done:
> +	report_prefix_pop();
> +}

  Thomas