On Tue Dec 19, 2023 at 11:57 PM AEST, Thomas Huth wrote:
> 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@xxxxxxxxx>
> > ---
> > 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?
Yeah, should be more consistent with those. I'll take a look.
>
> > +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?
Yeah you're right.
>
> > + 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?
It's an interrupt-specific bit so SRR1_xx, but yes it should be a
define.
>
> > +}
> > +
> > +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)?
Yeah that would improve things.
> > + 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?
Will do.
Thanks,
Nick
