* [PATCH v2 0/8] FWNMI fixes / changes
@ 2020-03-16 14:26 Nicholas Piggin
2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin
` (7 more replies)
0 siblings, 8 replies; 38+ messages in thread
From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw)
To: qemu-ppc
Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel,
Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson
Hi,
Since v1, I fixed the intermediate compile error spotted by Greg, and
rediffed the series on top of ppc-for-5.0, plus Alexey's patch
("spapr/rtas: Reserve space for RTAS blob and log").
The first 6 patches are otherwise unchanged since last posting.
Patch 7 implements fwnim sreset interrupts now in a way that's
compatible with existing Linux guests (which doesn't necessarily quite
match PAPR, but does match PowerVM behaviour).
Patch 8 isn't required but it papers over Linux warning messages caused
by another quirk.
Thanks,
Nick
Nicholas Piggin (8):
ppc/spapr: Fix FWNMI machine check failure handling
ppc/spapr: Change FWNMI names
ppc/spapr: Add FWNMI System Reset state
ppc/spapr: Fix FWNMI machine check interrupt delivery
ppc/spapr: Allow FWNMI on TCG
target/ppc: Allow ppc_cpu_do_system_reset to take an alternate vector
ppc/spapr: Implement FWNMI System Reset delivery
ppc/spapr: Ignore common "ibm,nmi-interlock" Linux bug
hw/ppc/spapr.c | 76 ++++++++++++++++++++++-------
hw/ppc/spapr_caps.c | 19 ++++----
hw/ppc/spapr_events.c | 38 ++++-----------
hw/ppc/spapr_rtas.c | 43 +++++++++++++----
include/hw/ppc/spapr.h | 28 +++++++----
target/ppc/cpu.h | 3 +-
target/ppc/excp_helper.c | 79 ++++++++++++++++++++++---------
tests/qtest/libqos/libqos-spapr.h | 2 +-
8 files changed, 188 insertions(+), 100 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 15:27 ` Greg Kurz 2020-03-16 22:46 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin ` (6 subsequent siblings) 7 siblings, 2 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson ppc_cpu_do_system_reset delivers a system rreset interrupt to the guest, which is certainly not what is intended here. Panic the guest like other failure cases here do. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_events.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 2afd1844e4..11303258d4 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -785,7 +785,6 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered, static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) { SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); - CPUState *cs = CPU(cpu); uint64_t rtas_addr; CPUPPCState *env = &cpu->env; PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); @@ -823,8 +822,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) /* get rtas addr from fdt */ rtas_addr = spapr_get_rtas_addr(); if (!rtas_addr) { - /* Unable to fetch rtas_addr. Hence reset the guest */ - ppc_cpu_do_system_reset(cs); + qemu_system_guest_panicked(NULL); g_free(ext_elog); return; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling 2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin @ 2020-03-16 15:27 ` Greg Kurz 2020-03-16 22:46 ` David Gibson 1 sibling, 0 replies; 38+ messages in thread From: Greg Kurz @ 2020-03-16 15:27 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Ganesh Goudar, qemu-ppc, David Gibson On Tue, 17 Mar 2020 00:26:06 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > ppc_cpu_do_system_reset delivers a system rreset interrupt to the guest, > which is certainly not what is intended here. Panic the guest like other > failure cases here do. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Makes sense. Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_events.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 2afd1844e4..11303258d4 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -785,7 +785,6 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered, > static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > { > SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > - CPUState *cs = CPU(cpu); > uint64_t rtas_addr; > CPUPPCState *env = &cpu->env; > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > @@ -823,8 +822,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > /* get rtas addr from fdt */ > rtas_addr = spapr_get_rtas_addr(); > if (!rtas_addr) { > - /* Unable to fetch rtas_addr. Hence reset the guest */ > - ppc_cpu_do_system_reset(cs); > + qemu_system_guest_panicked(NULL); > g_free(ext_elog); > return; > } ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling 2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin 2020-03-16 15:27 ` Greg Kurz @ 2020-03-16 22:46 ` David Gibson 1 sibling, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 22:46 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 1610 bytes --] On Tue, Mar 17, 2020 at 12:26:06AM +1000, Nicholas Piggin wrote: > ppc_cpu_do_system_reset delivers a system rreset interrupt to the guest, > which is certainly not what is intended here. Panic the guest like other > failure cases here do. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied to ppc-for-5.0, thanks. > --- > hw/ppc/spapr_events.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 2afd1844e4..11303258d4 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -785,7 +785,6 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered, > static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > { > SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > - CPUState *cs = CPU(cpu); > uint64_t rtas_addr; > CPUPPCState *env = &cpu->env; > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > @@ -823,8 +822,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > /* get rtas addr from fdt */ > rtas_addr = spapr_get_rtas_addr(); > if (!rtas_addr) { > - /* Unable to fetch rtas_addr. Hence reset the guest */ > - ppc_cpu_do_system_reset(cs); > + qemu_system_guest_panicked(NULL); > g_free(ext_elog); > return; > } -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin 2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 17:15 ` Greg Kurz ` (3 more replies) 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin ` (5 subsequent siblings) 7 siblings, 4 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson The option is called "FWNMI", and it involves more than just machine checks, also machine checks can be delivered without the FWNMI option, so re-name various things to reflect that. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr.c | 28 ++++++++++++++-------------- hw/ppc/spapr_caps.c | 14 +++++++------- hw/ppc/spapr_events.c | 14 +++++++------- hw/ppc/spapr_rtas.c | 17 +++++++++-------- include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- tests/qtest/libqos/libqos-spapr.h | 2 +- 6 files changed, 55 insertions(+), 47 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index d3db3ec56e..b03b26370d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1704,11 +1704,11 @@ static void spapr_machine_reset(MachineState *machine) spapr->cas_reboot = false; - spapr->mc_status = -1; - spapr->guest_machine_check_addr = -1; + spapr->fwnmi_machine_check_addr = -1; + spapr->fwnmi_machine_check_interlock = -1; /* Signal all vCPUs waiting on this condition */ - qemu_cond_broadcast(&spapr->mc_delivery_cond); + qemu_cond_broadcast(&spapr->fwnmi_machine_check_interlock_cond); migrate_del_blocker(spapr->fwnmi_migration_blocker); } @@ -1997,7 +1997,7 @@ static bool spapr_fwnmi_needed(void *opaque) { SpaprMachineState *spapr = (SpaprMachineState *)opaque; - return spapr->guest_machine_check_addr != -1; + return spapr->fwnmi_machine_check_addr != -1; } static int spapr_fwnmi_pre_save(void *opaque) @@ -2008,7 +2008,7 @@ static int spapr_fwnmi_pre_save(void *opaque) * Check if machine check handling is in progress and print a * warning message. */ - if (spapr->mc_status != -1) { + if (spapr->fwnmi_machine_check_interlock != -1) { warn_report("A machine check is being handled during migration. The" "handler may run and log hardware error on the destination"); } @@ -2016,15 +2016,15 @@ static int spapr_fwnmi_pre_save(void *opaque) return 0; } -static const VMStateDescription vmstate_spapr_machine_check = { - .name = "spapr_machine_check", +static const VMStateDescription vmstate_spapr_fwnmi = { + .name = "spapr_fwnmi", .version_id = 1, .minimum_version_id = 1, .needed = spapr_fwnmi_needed, .pre_save = spapr_fwnmi_pre_save, .fields = (VMStateField[]) { - VMSTATE_UINT64(guest_machine_check_addr, SpaprMachineState), - VMSTATE_INT32(mc_status, SpaprMachineState), + VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), + VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), VMSTATE_END_OF_LIST() }, }; @@ -2063,7 +2063,7 @@ static const VMStateDescription vmstate_spapr = { &vmstate_spapr_cap_large_decr, &vmstate_spapr_cap_ccf_assist, &vmstate_spapr_cap_fwnmi, - &vmstate_spapr_machine_check, + &vmstate_spapr_fwnmi, NULL } }; @@ -2884,7 +2884,7 @@ static void spapr_machine_init(MachineState *machine) spapr_create_lmb_dr_connectors(spapr); } - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) { + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_ON) { /* Create the error string for live migration blocker */ error_setg(&spapr->fwnmi_migration_blocker, "A machine check is being handled during migration. The handler" @@ -3053,7 +3053,7 @@ static void spapr_machine_init(MachineState *machine) kvmppc_spapr_enable_inkernel_multitce(); } - qemu_cond_init(&spapr->mc_delivery_cond); + qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); } static int spapr_kvm_type(MachineState *machine, const char *vm_type) @@ -4534,7 +4534,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF; smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON; + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; spapr_caps_add_properties(smc, &error_abort); smc->irq = &spapr_irq_dual; smc->dr_phb_enabled = true; @@ -4612,7 +4612,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc) spapr_machine_5_0_class_options(mc); compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_OFF; smc->rma_limit = 16 * GiB; mc->nvdimm_supported = false; } diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 8b27d3ac09..f626d769a0 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -509,7 +509,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, } } -static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val, +static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { if (!val) { @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { .type = "bool", .apply = cap_ccf_assist_apply, }, - [SPAPR_CAP_FWNMI_MCE] = { - .name = "fwnmi-mce", - .description = "Handle fwnmi machine check exceptions", - .index = SPAPR_CAP_FWNMI_MCE, + [SPAPR_CAP_FWNMI] = { + .name = "fwnmi", + .description = "Implements PAPR FWNMI option", + .index = SPAPR_CAP_FWNMI, .get = spapr_cap_get_bool, .set = spapr_cap_set_bool, .type = "bool", - .apply = cap_fwnmi_mce_apply, + .apply = cap_fwnmi_apply, }, }; @@ -774,7 +774,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE); SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); -SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE); +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); void spapr_caps_init(SpaprMachineState *spapr) { diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 11303258d4..27ba8a2c19 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -837,7 +837,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; env->msr = msr; - env->nip = spapr->guest_machine_check_addr; + env->nip = spapr->fwnmi_machine_check_addr; g_free(ext_elog); } @@ -849,7 +849,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) int ret; Error *local_err = NULL; - if (spapr->guest_machine_check_addr == -1) { + if (spapr->fwnmi_machine_check_addr == -1) { /* * This implies that we have hit a machine check either when the * guest has not registered FWNMI (i.e., "ibm,nmi-register" not @@ -861,19 +861,19 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) return; } - while (spapr->mc_status != -1) { + while (spapr->fwnmi_machine_check_interlock != -1) { /* * Check whether the same CPU got machine check error * while still handling the mc error (i.e., before * that CPU called "ibm,nmi-interlock") */ - if (spapr->mc_status == cpu->vcpu_id) { + if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) { qemu_system_guest_panicked(NULL); return; } - qemu_cond_wait_iothread(&spapr->mc_delivery_cond); + qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); /* Meanwhile if the system is reset, then just return */ - if (spapr->guest_machine_check_addr == -1) { + if (spapr->fwnmi_machine_check_addr == -1) { return; } } @@ -889,7 +889,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) warn_report("Received a fwnmi while migration was in progress"); } - spapr->mc_status = cpu->vcpu_id; + spapr->fwnmi_machine_check_interlock = cpu->vcpu_id; spapr_mce_dispatch_elog(cpu, recovered); } diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index fe83b50c66..0b8c481593 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -415,7 +415,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, { hwaddr rtas_addr; - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); return; } @@ -426,7 +426,8 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, return; } - spapr->guest_machine_check_addr = rtas_ld(args, 1); + spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); + rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -436,18 +437,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, target_ulong args, uint32_t nret, target_ulong rets) { - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); return; } - if (spapr->guest_machine_check_addr == -1) { + if (spapr->fwnmi_machine_check_addr == -1) { /* NMI register not called */ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); return; } - if (spapr->mc_status != cpu->vcpu_id) { + if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); return; @@ -455,10 +456,10 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, /* * vCPU issuing "ibm,nmi-interlock" is done with NMI handling, - * hence unset mc_status. + * hence unset fwnmi_machine_check_interlock. */ - spapr->mc_status = -1; - qemu_cond_signal(&spapr->mc_delivery_cond); + spapr->fwnmi_machine_check_interlock = -1; + qemu_cond_signal(&spapr->fwnmi_machine_check_interlock_cond); rtas_st(rets, 0, RTAS_OUT_SUCCESS); migrate_del_blocker(spapr->fwnmi_migration_blocker); } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 35b489a549..64b83402cb 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -79,10 +79,10 @@ typedef enum { #define SPAPR_CAP_LARGE_DECREMENTER 0x08 /* Count Cache Flush Assist HW Instruction */ #define SPAPR_CAP_CCF_ASSIST 0x09 -/* FWNMI machine check handling */ -#define SPAPR_CAP_FWNMI_MCE 0x0A +/* Implements PAPR FWNMI option */ +#define SPAPR_CAP_FWNMI 0x0A /* Num Caps */ -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1) +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) /* * Capability Values @@ -192,14 +192,21 @@ struct SpaprMachineState { * occurs during the unplug process. */ QTAILQ_HEAD(, SpaprDimmState) pending_dimm_unplugs; - /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */ - target_ulong guest_machine_check_addr; - /* - * mc_status is set to -1 if mc is not in progress, else is set to the CPU - * handling the mc. + /* State related to FWNMI option */ + + /* Machine Check Notification Routine address + * registered by "ibm,nmi-register" RTAS call. + */ + target_ulong fwnmi_machine_check_addr; + + /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is + * set to -1 if a FWNMI machine check is not in progress, else is set to + * the CPU that was delivered the machine check, and is set back to -1 + * when that CPU makes an "ibm,nmi-interlock" RTAS call. The cond is used + * to synchronize other CPUs. */ - int mc_status; - QemuCond mc_delivery_cond; + int fwnmi_machine_check_interlock; + QemuCond fwnmi_machine_check_interlock_cond; /*< public >*/ char *kvm_type; diff --git a/tests/qtest/libqos/libqos-spapr.h b/tests/qtest/libqos/libqos-spapr.h index d9c4c22343..16174dbada 100644 --- a/tests/qtest/libqos/libqos-spapr.h +++ b/tests/qtest/libqos/libqos-spapr.h @@ -13,6 +13,6 @@ void qtest_spapr_shutdown(QOSState *qs); "cap-sbbc=broken," \ "cap-ibs=broken," \ "cap-ccf-assist=off," \ - "cap-fwnmi-mce=off" + "cap-fwnmi=off" #endif -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin @ 2020-03-16 17:15 ` Greg Kurz 2020-03-16 17:18 ` Mahesh J Salgaonkar ` (2 subsequent siblings) 3 siblings, 0 replies; 38+ messages in thread From: Greg Kurz @ 2020-03-16 17:15 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Ganesh Goudar, qemu-ppc, David Gibson On Tue, 17 Mar 2020 00:26:07 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > The option is called "FWNMI", and it involves more than just machine > checks, also machine checks can be delivered without the FWNMI option, > so re-name various things to reflect that. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr.c | 28 ++++++++++++++-------------- > hw/ppc/spapr_caps.c | 14 +++++++------- > hw/ppc/spapr_events.c | 14 +++++++------- > hw/ppc/spapr_rtas.c | 17 +++++++++-------- > include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- > tests/qtest/libqos/libqos-spapr.h | 2 +- > 6 files changed, 55 insertions(+), 47 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d3db3ec56e..b03b26370d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,11 +1704,11 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > - spapr->mc_status = -1; > - spapr->guest_machine_check_addr = -1; > + spapr->fwnmi_machine_check_addr = -1; > + spapr->fwnmi_machine_check_interlock = -1; > > /* Signal all vCPUs waiting on this condition */ > - qemu_cond_broadcast(&spapr->mc_delivery_cond); > + qemu_cond_broadcast(&spapr->fwnmi_machine_check_interlock_cond); > > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > @@ -1997,7 +1997,7 @@ static bool spapr_fwnmi_needed(void *opaque) > { > SpaprMachineState *spapr = (SpaprMachineState *)opaque; > > - return spapr->guest_machine_check_addr != -1; > + return spapr->fwnmi_machine_check_addr != -1; > } > > static int spapr_fwnmi_pre_save(void *opaque) > @@ -2008,7 +2008,7 @@ static int spapr_fwnmi_pre_save(void *opaque) > * Check if machine check handling is in progress and print a > * warning message. > */ > - if (spapr->mc_status != -1) { > + if (spapr->fwnmi_machine_check_interlock != -1) { > warn_report("A machine check is being handled during migration. The" > "handler may run and log hardware error on the destination"); > } > @@ -2016,15 +2016,15 @@ static int spapr_fwnmi_pre_save(void *opaque) > return 0; > } > > -static const VMStateDescription vmstate_spapr_machine_check = { > - .name = "spapr_machine_check", > +static const VMStateDescription vmstate_spapr_fwnmi = { > + .name = "spapr_fwnmi", > .version_id = 1, > .minimum_version_id = 1, > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > - VMSTATE_UINT64(guest_machine_check_addr, SpaprMachineState), > - VMSTATE_INT32(mc_status, SpaprMachineState), > + VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > + VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > }, > }; > @@ -2063,7 +2063,7 @@ static const VMStateDescription vmstate_spapr = { > &vmstate_spapr_cap_large_decr, > &vmstate_spapr_cap_ccf_assist, > &vmstate_spapr_cap_fwnmi, > - &vmstate_spapr_machine_check, > + &vmstate_spapr_fwnmi, > NULL > } > }; > @@ -2884,7 +2884,7 @@ static void spapr_machine_init(MachineState *machine) > spapr_create_lmb_dr_connectors(spapr); > } > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_ON) { > /* Create the error string for live migration blocker */ > error_setg(&spapr->fwnmi_migration_blocker, > "A machine check is being handled during migration. The handler" > @@ -3053,7 +3053,7 @@ static void spapr_machine_init(MachineState *machine) > kvmppc_spapr_enable_inkernel_multitce(); > } > > - qemu_cond_init(&spapr->mc_delivery_cond); > + qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); > } > > static int spapr_kvm_type(MachineState *machine, const char *vm_type) > @@ -4534,7 +4534,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF; > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; > spapr_caps_add_properties(smc, &error_abort); > smc->irq = &spapr_irq_dual; > smc->dr_phb_enabled = true; > @@ -4612,7 +4612,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc) > spapr_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_OFF; > smc->rma_limit = 16 * GiB; > mc->nvdimm_supported = false; > } > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 8b27d3ac09..f626d769a0 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -509,7 +509,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, > } > } > > -static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val, > +static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > if (!val) { > @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > .type = "bool", > .apply = cap_ccf_assist_apply, > }, > - [SPAPR_CAP_FWNMI_MCE] = { > - .name = "fwnmi-mce", > - .description = "Handle fwnmi machine check exceptions", > - .index = SPAPR_CAP_FWNMI_MCE, > + [SPAPR_CAP_FWNMI] = { > + .name = "fwnmi", > + .description = "Implements PAPR FWNMI option", > + .index = SPAPR_CAP_FWNMI, > .get = spapr_cap_get_bool, > .set = spapr_cap_set_bool, > .type = "bool", > - .apply = cap_fwnmi_mce_apply, > + .apply = cap_fwnmi_apply, > }, > }; > > @@ -774,7 +774,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE); > SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); > SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); > SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); > -SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE); > +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); > > void spapr_caps_init(SpaprMachineState *spapr) > { > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 11303258d4..27ba8a2c19 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -837,7 +837,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > > env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; > env->msr = msr; > - env->nip = spapr->guest_machine_check_addr; > + env->nip = spapr->fwnmi_machine_check_addr; > > g_free(ext_elog); > } > @@ -849,7 +849,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > int ret; > Error *local_err = NULL; > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* > * This implies that we have hit a machine check either when the > * guest has not registered FWNMI (i.e., "ibm,nmi-register" not > @@ -861,19 +861,19 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > return; > } > > - while (spapr->mc_status != -1) { > + while (spapr->fwnmi_machine_check_interlock != -1) { > /* > * Check whether the same CPU got machine check error > * while still handling the mc error (i.e., before > * that CPU called "ibm,nmi-interlock") > */ > - if (spapr->mc_status == cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) { > qemu_system_guest_panicked(NULL); > return; > } > - qemu_cond_wait_iothread(&spapr->mc_delivery_cond); > + qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); > /* Meanwhile if the system is reset, then just return */ > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > return; > } > } > @@ -889,7 +889,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > warn_report("Received a fwnmi while migration was in progress"); > } > > - spapr->mc_status = cpu->vcpu_id; > + spapr->fwnmi_machine_check_interlock = cpu->vcpu_id; > spapr_mce_dispatch_elog(cpu, recovered); > } > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index fe83b50c66..0b8c481593 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -415,7 +415,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > { > hwaddr rtas_addr; > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > @@ -426,7 +426,8 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->guest_machine_check_addr = rtas_ld(args, 1); > + spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > > @@ -436,18 +437,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > target_ulong args, > uint32_t nret, target_ulong rets) > { > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* NMI register not called */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > - if (spapr->mc_status != cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { > /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > @@ -455,10 +456,10 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > > /* > * vCPU issuing "ibm,nmi-interlock" is done with NMI handling, > - * hence unset mc_status. > + * hence unset fwnmi_machine_check_interlock. > */ > - spapr->mc_status = -1; > - qemu_cond_signal(&spapr->mc_delivery_cond); > + spapr->fwnmi_machine_check_interlock = -1; > + qemu_cond_signal(&spapr->fwnmi_machine_check_interlock_cond); > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 35b489a549..64b83402cb 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -79,10 +79,10 @@ typedef enum { > #define SPAPR_CAP_LARGE_DECREMENTER 0x08 > /* Count Cache Flush Assist HW Instruction */ > #define SPAPR_CAP_CCF_ASSIST 0x09 > -/* FWNMI machine check handling */ > -#define SPAPR_CAP_FWNMI_MCE 0x0A > +/* Implements PAPR FWNMI option */ > +#define SPAPR_CAP_FWNMI 0x0A > /* Num Caps */ > -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1) > +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) > > /* > * Capability Values > @@ -192,14 +192,21 @@ struct SpaprMachineState { > * occurs during the unplug process. */ > QTAILQ_HEAD(, SpaprDimmState) pending_dimm_unplugs; > > - /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */ > - target_ulong guest_machine_check_addr; > - /* > - * mc_status is set to -1 if mc is not in progress, else is set to the CPU > - * handling the mc. > + /* State related to FWNMI option */ > + > + /* Machine Check Notification Routine address > + * registered by "ibm,nmi-register" RTAS call. > + */ > + target_ulong fwnmi_machine_check_addr; > + > + /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is > + * set to -1 if a FWNMI machine check is not in progress, else is set to > + * the CPU that was delivered the machine check, and is set back to -1 > + * when that CPU makes an "ibm,nmi-interlock" RTAS call. The cond is used > + * to synchronize other CPUs. > */ > - int mc_status; > - QemuCond mc_delivery_cond; > + int fwnmi_machine_check_interlock; > + QemuCond fwnmi_machine_check_interlock_cond; > > /*< public >*/ > char *kvm_type; > diff --git a/tests/qtest/libqos/libqos-spapr.h b/tests/qtest/libqos/libqos-spapr.h > index d9c4c22343..16174dbada 100644 > --- a/tests/qtest/libqos/libqos-spapr.h > +++ b/tests/qtest/libqos/libqos-spapr.h > @@ -13,6 +13,6 @@ void qtest_spapr_shutdown(QOSState *qs); > "cap-sbbc=broken," \ > "cap-ibs=broken," \ > "cap-ccf-assist=off," \ > - "cap-fwnmi-mce=off" > + "cap-fwnmi=off" > > #endif ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin 2020-03-16 17:15 ` Greg Kurz @ 2020-03-16 17:18 ` Mahesh J Salgaonkar 2020-03-16 17:25 ` Greg Kurz 2020-03-16 17:32 ` Cédric Le Goater 2020-03-16 22:46 ` David Gibson 3 siblings, 1 reply; 38+ messages in thread From: Mahesh J Salgaonkar @ 2020-03-16 17:18 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc, David Gibson On 2020-03-17 00:26:07 Tue, Nicholas Piggin wrote: > The option is called "FWNMI", and it involves more than just machine > checks, also machine checks can be delivered without the FWNMI option, > so re-name various things to reflect that. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr.c | 28 ++++++++++++++-------------- > hw/ppc/spapr_caps.c | 14 +++++++------- > hw/ppc/spapr_events.c | 14 +++++++------- > hw/ppc/spapr_rtas.c | 17 +++++++++-------- > include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- > tests/qtest/libqos/libqos-spapr.h | 2 +- > 6 files changed, 55 insertions(+), 47 deletions(-) > [...] > @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > .type = "bool", > .apply = cap_ccf_assist_apply, > }, > - [SPAPR_CAP_FWNMI_MCE] = { > - .name = "fwnmi-mce", > - .description = "Handle fwnmi machine check exceptions", > - .index = SPAPR_CAP_FWNMI_MCE, > + [SPAPR_CAP_FWNMI] = { > + .name = "fwnmi", I guess this should be fine and should hit QEMU 5.0 release so that we don't end up with two different CAP names for 5.0 and future releases. Thanks, -Mahesh. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 17:18 ` Mahesh J Salgaonkar @ 2020-03-16 17:25 ` Greg Kurz 0 siblings, 0 replies; 38+ messages in thread From: Greg Kurz @ 2020-03-16 17:25 UTC (permalink / raw) To: Mahesh J Salgaonkar Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Ganesh Goudar, qemu-ppc, David Gibson On Mon, 16 Mar 2020 22:48:45 +0530 Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote: > On 2020-03-17 00:26:07 Tue, Nicholas Piggin wrote: > > The option is called "FWNMI", and it involves more than just machine > > checks, also machine checks can be delivered without the FWNMI option, > > so re-name various things to reflect that. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > hw/ppc/spapr.c | 28 ++++++++++++++-------------- > > hw/ppc/spapr_caps.c | 14 +++++++------- > > hw/ppc/spapr_events.c | 14 +++++++------- > > hw/ppc/spapr_rtas.c | 17 +++++++++-------- > > include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- > > tests/qtest/libqos/libqos-spapr.h | 2 +- > > 6 files changed, 55 insertions(+), 47 deletions(-) > > > [...] > > @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > > .type = "bool", > > .apply = cap_ccf_assist_apply, > > }, > > - [SPAPR_CAP_FWNMI_MCE] = { > > - .name = "fwnmi-mce", > > - .description = "Handle fwnmi machine check exceptions", > > - .index = SPAPR_CAP_FWNMI_MCE, > > + [SPAPR_CAP_FWNMI] = { > > + .name = "fwnmi", > > I guess this should be fine and should hit QEMU 5.0 release so that we > don't end up with two different CAP names for 5.0 and future releases. > Yeah we really want this patch and the next one (which affects migration) to go to 5.0. > Thanks, > -Mahesh. > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin 2020-03-16 17:15 ` Greg Kurz 2020-03-16 17:18 ` Mahesh J Salgaonkar @ 2020-03-16 17:32 ` Cédric Le Goater 2020-03-16 22:46 ` David Gibson 3 siblings, 0 replies; 38+ messages in thread From: Cédric Le Goater @ 2020-03-16 17:32 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Aravinda Prasad, David Gibson, Ganesh Goudar, qemu-devel, Greg Kurz On 3/16/20 3:26 PM, Nicholas Piggin wrote: > The option is called "FWNMI", and it involves more than just machine > checks, also machine checks can be delivered without the FWNMI option, > so re-name various things to reflect that. Reviewed-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr.c | 28 ++++++++++++++-------------- > hw/ppc/spapr_caps.c | 14 +++++++------- > hw/ppc/spapr_events.c | 14 +++++++------- > hw/ppc/spapr_rtas.c | 17 +++++++++-------- > include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- > tests/qtest/libqos/libqos-spapr.h | 2 +- > 6 files changed, 55 insertions(+), 47 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d3db3ec56e..b03b26370d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,11 +1704,11 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > - spapr->mc_status = -1; > - spapr->guest_machine_check_addr = -1; > + spapr->fwnmi_machine_check_addr = -1; > + spapr->fwnmi_machine_check_interlock = -1; > > /* Signal all vCPUs waiting on this condition */ > - qemu_cond_broadcast(&spapr->mc_delivery_cond); > + qemu_cond_broadcast(&spapr->fwnmi_machine_check_interlock_cond); > > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > @@ -1997,7 +1997,7 @@ static bool spapr_fwnmi_needed(void *opaque) > { > SpaprMachineState *spapr = (SpaprMachineState *)opaque; > > - return spapr->guest_machine_check_addr != -1; > + return spapr->fwnmi_machine_check_addr != -1; > } > > static int spapr_fwnmi_pre_save(void *opaque) > @@ -2008,7 +2008,7 @@ static int spapr_fwnmi_pre_save(void *opaque) > * Check if machine check handling is in progress and print a > * warning message. > */ > - if (spapr->mc_status != -1) { > + if (spapr->fwnmi_machine_check_interlock != -1) { > warn_report("A machine check is being handled during migration. The" > "handler may run and log hardware error on the destination"); > } > @@ -2016,15 +2016,15 @@ static int spapr_fwnmi_pre_save(void *opaque) > return 0; > } > > -static const VMStateDescription vmstate_spapr_machine_check = { > - .name = "spapr_machine_check", > +static const VMStateDescription vmstate_spapr_fwnmi = { > + .name = "spapr_fwnmi", > .version_id = 1, > .minimum_version_id = 1, > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > - VMSTATE_UINT64(guest_machine_check_addr, SpaprMachineState), > - VMSTATE_INT32(mc_status, SpaprMachineState), > + VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > + VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > }, > }; > @@ -2063,7 +2063,7 @@ static const VMStateDescription vmstate_spapr = { > &vmstate_spapr_cap_large_decr, > &vmstate_spapr_cap_ccf_assist, > &vmstate_spapr_cap_fwnmi, > - &vmstate_spapr_machine_check, > + &vmstate_spapr_fwnmi, > NULL > } > }; > @@ -2884,7 +2884,7 @@ static void spapr_machine_init(MachineState *machine) > spapr_create_lmb_dr_connectors(spapr); > } > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_ON) { > /* Create the error string for live migration blocker */ > error_setg(&spapr->fwnmi_migration_blocker, > "A machine check is being handled during migration. The handler" > @@ -3053,7 +3053,7 @@ static void spapr_machine_init(MachineState *machine) > kvmppc_spapr_enable_inkernel_multitce(); > } > > - qemu_cond_init(&spapr->mc_delivery_cond); > + qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); > } > > static int spapr_kvm_type(MachineState *machine, const char *vm_type) > @@ -4534,7 +4534,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF; > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; > spapr_caps_add_properties(smc, &error_abort); > smc->irq = &spapr_irq_dual; > smc->dr_phb_enabled = true; > @@ -4612,7 +4612,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc) > spapr_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_OFF; > smc->rma_limit = 16 * GiB; > mc->nvdimm_supported = false; > } > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 8b27d3ac09..f626d769a0 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -509,7 +509,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, > } > } > > -static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val, > +static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > if (!val) { > @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > .type = "bool", > .apply = cap_ccf_assist_apply, > }, > - [SPAPR_CAP_FWNMI_MCE] = { > - .name = "fwnmi-mce", > - .description = "Handle fwnmi machine check exceptions", > - .index = SPAPR_CAP_FWNMI_MCE, > + [SPAPR_CAP_FWNMI] = { > + .name = "fwnmi", > + .description = "Implements PAPR FWNMI option", > + .index = SPAPR_CAP_FWNMI, > .get = spapr_cap_get_bool, > .set = spapr_cap_set_bool, > .type = "bool", > - .apply = cap_fwnmi_mce_apply, > + .apply = cap_fwnmi_apply, > }, > }; > > @@ -774,7 +774,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE); > SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); > SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); > SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); > -SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE); > +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); > > void spapr_caps_init(SpaprMachineState *spapr) > { > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 11303258d4..27ba8a2c19 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -837,7 +837,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > > env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; > env->msr = msr; > - env->nip = spapr->guest_machine_check_addr; > + env->nip = spapr->fwnmi_machine_check_addr; > > g_free(ext_elog); > } > @@ -849,7 +849,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > int ret; > Error *local_err = NULL; > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* > * This implies that we have hit a machine check either when the > * guest has not registered FWNMI (i.e., "ibm,nmi-register" not > @@ -861,19 +861,19 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > return; > } > > - while (spapr->mc_status != -1) { > + while (spapr->fwnmi_machine_check_interlock != -1) { > /* > * Check whether the same CPU got machine check error > * while still handling the mc error (i.e., before > * that CPU called "ibm,nmi-interlock") > */ > - if (spapr->mc_status == cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) { > qemu_system_guest_panicked(NULL); > return; > } > - qemu_cond_wait_iothread(&spapr->mc_delivery_cond); > + qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); > /* Meanwhile if the system is reset, then just return */ > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > return; > } > } > @@ -889,7 +889,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > warn_report("Received a fwnmi while migration was in progress"); > } > > - spapr->mc_status = cpu->vcpu_id; > + spapr->fwnmi_machine_check_interlock = cpu->vcpu_id; > spapr_mce_dispatch_elog(cpu, recovered); > } > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index fe83b50c66..0b8c481593 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -415,7 +415,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > { > hwaddr rtas_addr; > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > @@ -426,7 +426,8 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->guest_machine_check_addr = rtas_ld(args, 1); > + spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > > @@ -436,18 +437,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > target_ulong args, > uint32_t nret, target_ulong rets) > { > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* NMI register not called */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > - if (spapr->mc_status != cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { > /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > @@ -455,10 +456,10 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > > /* > * vCPU issuing "ibm,nmi-interlock" is done with NMI handling, > - * hence unset mc_status. > + * hence unset fwnmi_machine_check_interlock. > */ > - spapr->mc_status = -1; > - qemu_cond_signal(&spapr->mc_delivery_cond); > + spapr->fwnmi_machine_check_interlock = -1; > + qemu_cond_signal(&spapr->fwnmi_machine_check_interlock_cond); > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 35b489a549..64b83402cb 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -79,10 +79,10 @@ typedef enum { > #define SPAPR_CAP_LARGE_DECREMENTER 0x08 > /* Count Cache Flush Assist HW Instruction */ > #define SPAPR_CAP_CCF_ASSIST 0x09 > -/* FWNMI machine check handling */ > -#define SPAPR_CAP_FWNMI_MCE 0x0A > +/* Implements PAPR FWNMI option */ > +#define SPAPR_CAP_FWNMI 0x0A > /* Num Caps */ > -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1) > +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) > > /* > * Capability Values > @@ -192,14 +192,21 @@ struct SpaprMachineState { > * occurs during the unplug process. */ > QTAILQ_HEAD(, SpaprDimmState) pending_dimm_unplugs; > > - /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */ > - target_ulong guest_machine_check_addr; > - /* > - * mc_status is set to -1 if mc is not in progress, else is set to the CPU > - * handling the mc. > + /* State related to FWNMI option */ > + > + /* Machine Check Notification Routine address > + * registered by "ibm,nmi-register" RTAS call. > + */ > + target_ulong fwnmi_machine_check_addr; > + > + /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is > + * set to -1 if a FWNMI machine check is not in progress, else is set to > + * the CPU that was delivered the machine check, and is set back to -1 > + * when that CPU makes an "ibm,nmi-interlock" RTAS call. The cond is used > + * to synchronize other CPUs. > */ > - int mc_status; > - QemuCond mc_delivery_cond; > + int fwnmi_machine_check_interlock; > + QemuCond fwnmi_machine_check_interlock_cond; > > /*< public >*/ > char *kvm_type; > diff --git a/tests/qtest/libqos/libqos-spapr.h b/tests/qtest/libqos/libqos-spapr.h > index d9c4c22343..16174dbada 100644 > --- a/tests/qtest/libqos/libqos-spapr.h > +++ b/tests/qtest/libqos/libqos-spapr.h > @@ -13,6 +13,6 @@ void qtest_spapr_shutdown(QOSState *qs); > "cap-sbbc=broken," \ > "cap-ibs=broken," \ > "cap-ccf-assist=off," \ > - "cap-fwnmi-mce=off" > + "cap-fwnmi=off" > > #endif > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/8] ppc/spapr: Change FWNMI names 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin ` (2 preceding siblings ...) 2020-03-16 17:32 ` Cédric Le Goater @ 2020-03-16 22:46 ` David Gibson 3 siblings, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 22:46 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 14100 bytes --] On Tue, Mar 17, 2020 at 12:26:07AM +1000, Nicholas Piggin wrote: > The option is called "FWNMI", and it involves more than just machine > checks, also machine checks can be delivered without the FWNMI option, > so re-name various things to reflect that. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied to ppc-for-5.0, thanks. > --- > hw/ppc/spapr.c | 28 ++++++++++++++-------------- > hw/ppc/spapr_caps.c | 14 +++++++------- > hw/ppc/spapr_events.c | 14 +++++++------- > hw/ppc/spapr_rtas.c | 17 +++++++++-------- > include/hw/ppc/spapr.h | 27 +++++++++++++++++---------- > tests/qtest/libqos/libqos-spapr.h | 2 +- > 6 files changed, 55 insertions(+), 47 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index d3db3ec56e..b03b26370d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,11 +1704,11 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > - spapr->mc_status = -1; > - spapr->guest_machine_check_addr = -1; > + spapr->fwnmi_machine_check_addr = -1; > + spapr->fwnmi_machine_check_interlock = -1; > > /* Signal all vCPUs waiting on this condition */ > - qemu_cond_broadcast(&spapr->mc_delivery_cond); > + qemu_cond_broadcast(&spapr->fwnmi_machine_check_interlock_cond); > > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > @@ -1997,7 +1997,7 @@ static bool spapr_fwnmi_needed(void *opaque) > { > SpaprMachineState *spapr = (SpaprMachineState *)opaque; > > - return spapr->guest_machine_check_addr != -1; > + return spapr->fwnmi_machine_check_addr != -1; > } > > static int spapr_fwnmi_pre_save(void *opaque) > @@ -2008,7 +2008,7 @@ static int spapr_fwnmi_pre_save(void *opaque) > * Check if machine check handling is in progress and print a > * warning message. > */ > - if (spapr->mc_status != -1) { > + if (spapr->fwnmi_machine_check_interlock != -1) { > warn_report("A machine check is being handled during migration. The" > "handler may run and log hardware error on the destination"); > } > @@ -2016,15 +2016,15 @@ static int spapr_fwnmi_pre_save(void *opaque) > return 0; > } > > -static const VMStateDescription vmstate_spapr_machine_check = { > - .name = "spapr_machine_check", > +static const VMStateDescription vmstate_spapr_fwnmi = { > + .name = "spapr_fwnmi", > .version_id = 1, > .minimum_version_id = 1, > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > - VMSTATE_UINT64(guest_machine_check_addr, SpaprMachineState), > - VMSTATE_INT32(mc_status, SpaprMachineState), > + VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > + VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > }, > }; > @@ -2063,7 +2063,7 @@ static const VMStateDescription vmstate_spapr = { > &vmstate_spapr_cap_large_decr, > &vmstate_spapr_cap_ccf_assist, > &vmstate_spapr_cap_fwnmi, > - &vmstate_spapr_machine_check, > + &vmstate_spapr_fwnmi, > NULL > } > }; > @@ -2884,7 +2884,7 @@ static void spapr_machine_init(MachineState *machine) > spapr_create_lmb_dr_connectors(spapr); > } > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_ON) { > /* Create the error string for live migration blocker */ > error_setg(&spapr->fwnmi_migration_blocker, > "A machine check is being handled during migration. The handler" > @@ -3053,7 +3053,7 @@ static void spapr_machine_init(MachineState *machine) > kvmppc_spapr_enable_inkernel_multitce(); > } > > - qemu_cond_init(&spapr->mc_delivery_cond); > + qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond); > } > > static int spapr_kvm_type(MachineState *machine, const char *vm_type) > @@ -4534,7 +4534,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF; > smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; > spapr_caps_add_properties(smc, &error_abort); > smc->irq = &spapr_irq_dual; > smc->dr_phb_enabled = true; > @@ -4612,7 +4612,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc) > spapr_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; > - smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; > + smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_OFF; > smc->rma_limit = 16 * GiB; > mc->nvdimm_supported = false; > } > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 8b27d3ac09..f626d769a0 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -509,7 +509,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, > } > } > > -static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val, > +static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > if (!val) { > @@ -626,14 +626,14 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > .type = "bool", > .apply = cap_ccf_assist_apply, > }, > - [SPAPR_CAP_FWNMI_MCE] = { > - .name = "fwnmi-mce", > - .description = "Handle fwnmi machine check exceptions", > - .index = SPAPR_CAP_FWNMI_MCE, > + [SPAPR_CAP_FWNMI] = { > + .name = "fwnmi", > + .description = "Implements PAPR FWNMI option", > + .index = SPAPR_CAP_FWNMI, > .get = spapr_cap_get_bool, > .set = spapr_cap_set_bool, > .type = "bool", > - .apply = cap_fwnmi_mce_apply, > + .apply = cap_fwnmi_apply, > }, > }; > > @@ -774,7 +774,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE); > SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); > SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); > SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); > -SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE); > +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); > > void spapr_caps_init(SpaprMachineState *spapr) > { > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 11303258d4..27ba8a2c19 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -837,7 +837,7 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > > env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; > env->msr = msr; > - env->nip = spapr->guest_machine_check_addr; > + env->nip = spapr->fwnmi_machine_check_addr; > > g_free(ext_elog); > } > @@ -849,7 +849,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > int ret; > Error *local_err = NULL; > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* > * This implies that we have hit a machine check either when the > * guest has not registered FWNMI (i.e., "ibm,nmi-register" not > @@ -861,19 +861,19 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > return; > } > > - while (spapr->mc_status != -1) { > + while (spapr->fwnmi_machine_check_interlock != -1) { > /* > * Check whether the same CPU got machine check error > * while still handling the mc error (i.e., before > * that CPU called "ibm,nmi-interlock") > */ > - if (spapr->mc_status == cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) { > qemu_system_guest_panicked(NULL); > return; > } > - qemu_cond_wait_iothread(&spapr->mc_delivery_cond); > + qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); > /* Meanwhile if the system is reset, then just return */ > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > return; > } > } > @@ -889,7 +889,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > warn_report("Received a fwnmi while migration was in progress"); > } > > - spapr->mc_status = cpu->vcpu_id; > + spapr->fwnmi_machine_check_interlock = cpu->vcpu_id; > spapr_mce_dispatch_elog(cpu, recovered); > } > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index fe83b50c66..0b8c481593 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -415,7 +415,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > { > hwaddr rtas_addr; > > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > @@ -426,7 +426,8 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->guest_machine_check_addr = rtas_ld(args, 1); > + spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > > @@ -436,18 +437,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > target_ulong args, > uint32_t nret, target_ulong rets) > { > - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) { > + if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > return; > } > > - if (spapr->guest_machine_check_addr == -1) { > + if (spapr->fwnmi_machine_check_addr == -1) { > /* NMI register not called */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > } > > - if (spapr->mc_status != cpu->vcpu_id) { > + if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { > /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > return; > @@ -455,10 +456,10 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > > /* > * vCPU issuing "ibm,nmi-interlock" is done with NMI handling, > - * hence unset mc_status. > + * hence unset fwnmi_machine_check_interlock. > */ > - spapr->mc_status = -1; > - qemu_cond_signal(&spapr->mc_delivery_cond); > + spapr->fwnmi_machine_check_interlock = -1; > + qemu_cond_signal(&spapr->fwnmi_machine_check_interlock_cond); > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > migrate_del_blocker(spapr->fwnmi_migration_blocker); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 35b489a549..64b83402cb 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -79,10 +79,10 @@ typedef enum { > #define SPAPR_CAP_LARGE_DECREMENTER 0x08 > /* Count Cache Flush Assist HW Instruction */ > #define SPAPR_CAP_CCF_ASSIST 0x09 > -/* FWNMI machine check handling */ > -#define SPAPR_CAP_FWNMI_MCE 0x0A > +/* Implements PAPR FWNMI option */ > +#define SPAPR_CAP_FWNMI 0x0A > /* Num Caps */ > -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1) > +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) > > /* > * Capability Values > @@ -192,14 +192,21 @@ struct SpaprMachineState { > * occurs during the unplug process. */ > QTAILQ_HEAD(, SpaprDimmState) pending_dimm_unplugs; > > - /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */ > - target_ulong guest_machine_check_addr; > - /* > - * mc_status is set to -1 if mc is not in progress, else is set to the CPU > - * handling the mc. > + /* State related to FWNMI option */ > + > + /* Machine Check Notification Routine address > + * registered by "ibm,nmi-register" RTAS call. > + */ > + target_ulong fwnmi_machine_check_addr; > + > + /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is > + * set to -1 if a FWNMI machine check is not in progress, else is set to > + * the CPU that was delivered the machine check, and is set back to -1 > + * when that CPU makes an "ibm,nmi-interlock" RTAS call. The cond is used > + * to synchronize other CPUs. > */ > - int mc_status; > - QemuCond mc_delivery_cond; > + int fwnmi_machine_check_interlock; > + QemuCond fwnmi_machine_check_interlock_cond; > > /*< public >*/ > char *kvm_type; > diff --git a/tests/qtest/libqos/libqos-spapr.h b/tests/qtest/libqos/libqos-spapr.h > index d9c4c22343..16174dbada 100644 > --- a/tests/qtest/libqos/libqos-spapr.h > +++ b/tests/qtest/libqos/libqos-spapr.h > @@ -13,6 +13,6 @@ void qtest_spapr_shutdown(QOSState *qs); > "cap-sbbc=broken," \ > "cap-ibs=broken," \ > "cap-ccf-assist=off," \ > - "cap-fwnmi-mce=off" > + "cap-fwnmi=off" > > #endif -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin 2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 17:17 ` Greg Kurz ` (3 more replies) 2020-03-16 14:26 ` [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery Nicholas Piggin ` (4 subsequent siblings) 7 siblings, 4 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson The FWNMI option must deliver system reset interrupts to their registered address, and there are a few constraints on the handler addresses specified in PAPR. Add the system reset address state and checks. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr.c | 2 ++ hw/ppc/spapr_rtas.c | 14 +++++++++++++- include/hw/ppc/spapr.h | 3 ++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b03b26370d..5f93c49706 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine) spapr->cas_reboot = false; + spapr->fwnmi_system_reset_addr = -1; spapr->fwnmi_machine_check_addr = -1; spapr->fwnmi_machine_check_interlock = -1; @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = { .needed = spapr_fwnmi_needed, .pre_save = spapr_fwnmi_pre_save, .fields = (VMStateField[]) { + VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState), VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), VMSTATE_END_OF_LIST() diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 0b8c481593..521e6b0b72 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, uint32_t nret, target_ulong rets) { hwaddr rtas_addr; + target_ulong sreset_addr, mce_addr; if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, return; } - spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); + sreset_addr = rtas_ld(args, 0); + mce_addr = rtas_ld(args, 1); + + /* PAPR requires these are in the first 32M of memory and within RMA */ + if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size || + mce_addr >= 32 * MiB || mce_addr >= spapr->rma_size) { + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + return; + } + + spapr->fwnmi_system_reset_addr = sreset_addr; + spapr->fwnmi_machine_check_addr = mce_addr; rtas_st(rets, 0, RTAS_OUT_SUCCESS); } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 64b83402cb..42d64a0368 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -194,9 +194,10 @@ struct SpaprMachineState { /* State related to FWNMI option */ - /* Machine Check Notification Routine address + /* System Reset and Machine Check Notification Routine addresses * registered by "ibm,nmi-register" RTAS call. */ + target_ulong fwnmi_system_reset_addr; target_ulong fwnmi_machine_check_addr; /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin @ 2020-03-16 17:17 ` Greg Kurz 2020-03-16 17:29 ` Mahesh J Salgaonkar ` (2 subsequent siblings) 3 siblings, 0 replies; 38+ messages in thread From: Greg Kurz @ 2020-03-16 17:17 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Ganesh Goudar, qemu-ppc, David Gibson On Tue, 17 Mar 2020 00:26:08 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > The FWNMI option must deliver system reset interrupts to their > registered address, and there are a few constraints on the handler > addresses specified in PAPR. Add the system reset address state and > checks. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr.c | 2 ++ > hw/ppc/spapr_rtas.c | 14 +++++++++++++- > include/hw/ppc/spapr.h | 3 ++- > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b03b26370d..5f93c49706 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > + spapr->fwnmi_system_reset_addr = -1; > spapr->fwnmi_machine_check_addr = -1; > spapr->fwnmi_machine_check_interlock = -1; > > @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = { > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > + VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState), > VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 0b8c481593..521e6b0b72 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > hwaddr rtas_addr; > + target_ulong sreset_addr, mce_addr; > > if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + sreset_addr = rtas_ld(args, 0); > + mce_addr = rtas_ld(args, 1); > + > + /* PAPR requires these are in the first 32M of memory and within RMA */ > + if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size || > + mce_addr >= 32 * MiB || mce_addr >= spapr->rma_size) { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + spapr->fwnmi_system_reset_addr = sreset_addr; > + spapr->fwnmi_machine_check_addr = mce_addr; > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 64b83402cb..42d64a0368 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -194,9 +194,10 @@ struct SpaprMachineState { > > /* State related to FWNMI option */ > > - /* Machine Check Notification Routine address > + /* System Reset and Machine Check Notification Routine addresses > * registered by "ibm,nmi-register" RTAS call. > */ > + target_ulong fwnmi_system_reset_addr; > target_ulong fwnmi_machine_check_addr; > > /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin 2020-03-16 17:17 ` Greg Kurz @ 2020-03-16 17:29 ` Mahesh J Salgaonkar 2020-03-16 17:46 ` Cédric Le Goater 2020-03-16 22:47 ` David Gibson 3 siblings, 0 replies; 38+ messages in thread From: Mahesh J Salgaonkar @ 2020-03-16 17:29 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc, David Gibson On 2020-03-17 00:26:08 Tue, Nicholas Piggin wrote: > The FWNMI option must deliver system reset interrupts to their > registered address, and there are a few constraints on the handler > addresses specified in PAPR. Add the system reset address state and > checks. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Looks good to me. Reviwed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com> Thanks, -Mahesh. > --- > hw/ppc/spapr.c | 2 ++ > hw/ppc/spapr_rtas.c | 14 +++++++++++++- > include/hw/ppc/spapr.h | 3 ++- > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b03b26370d..5f93c49706 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > + spapr->fwnmi_system_reset_addr = -1; > spapr->fwnmi_machine_check_addr = -1; > spapr->fwnmi_machine_check_interlock = -1; > > @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = { > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > + VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState), > VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 0b8c481593..521e6b0b72 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > hwaddr rtas_addr; > + target_ulong sreset_addr, mce_addr; > > if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + sreset_addr = rtas_ld(args, 0); > + mce_addr = rtas_ld(args, 1); > + > + /* PAPR requires these are in the first 32M of memory and within RMA */ > + if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size || > + mce_addr >= 32 * MiB || mce_addr >= spapr->rma_size) { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + spapr->fwnmi_system_reset_addr = sreset_addr; > + spapr->fwnmi_machine_check_addr = mce_addr; > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 64b83402cb..42d64a0368 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -194,9 +194,10 @@ struct SpaprMachineState { > > /* State related to FWNMI option */ > > - /* Machine Check Notification Routine address > + /* System Reset and Machine Check Notification Routine addresses > * registered by "ibm,nmi-register" RTAS call. > */ > + target_ulong fwnmi_system_reset_addr; > target_ulong fwnmi_machine_check_addr; > > /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is > -- > 2.23.0 > > -- Mahesh J Salgaonkar ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin 2020-03-16 17:17 ` Greg Kurz 2020-03-16 17:29 ` Mahesh J Salgaonkar @ 2020-03-16 17:46 ` Cédric Le Goater 2020-03-16 22:47 ` David Gibson 3 siblings, 0 replies; 38+ messages in thread From: Cédric Le Goater @ 2020-03-16 17:46 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Aravinda Prasad, David Gibson, Ganesh Goudar, qemu-devel, Greg Kurz On 3/16/20 3:26 PM, Nicholas Piggin wrote: > The FWNMI option must deliver system reset interrupts to their > registered address, and there are a few constraints on the handler > addresses specified in PAPR. Add the system reset address state and > checks. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> This is in sync with the latest PAPR 2.9 Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr.c | 2 ++ > hw/ppc/spapr_rtas.c | 14 +++++++++++++- > include/hw/ppc/spapr.h | 3 ++- > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b03b26370d..5f93c49706 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > + spapr->fwnmi_system_reset_addr = -1; > spapr->fwnmi_machine_check_addr = -1; > spapr->fwnmi_machine_check_interlock = -1; > > @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = { > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > + VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState), > VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 0b8c481593..521e6b0b72 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > hwaddr rtas_addr; > + target_ulong sreset_addr, mce_addr; > > if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + sreset_addr = rtas_ld(args, 0); > + mce_addr = rtas_ld(args, 1); > + > + /* PAPR requires these are in the first 32M of memory and within RMA */ > + if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size || > + mce_addr >= 32 * MiB || mce_addr >= spapr->rma_size) { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + spapr->fwnmi_system_reset_addr = sreset_addr; > + spapr->fwnmi_machine_check_addr = mce_addr; > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 64b83402cb..42d64a0368 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -194,9 +194,10 @@ struct SpaprMachineState { > > /* State related to FWNMI option */ > > - /* Machine Check Notification Routine address > + /* System Reset and Machine Check Notification Routine addresses > * registered by "ibm,nmi-register" RTAS call. > */ > + target_ulong fwnmi_system_reset_addr; > target_ulong fwnmi_machine_check_addr; > > /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin ` (2 preceding siblings ...) 2020-03-16 17:46 ` Cédric Le Goater @ 2020-03-16 22:47 ` David Gibson 3 siblings, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 22:47 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 3496 bytes --] On Tue, Mar 17, 2020 at 12:26:08AM +1000, Nicholas Piggin wrote: > The FWNMI option must deliver system reset interrupts to their > registered address, and there are a few constraints on the handler > addresses specified in PAPR. Add the system reset address state and > checks. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied to ppc-for-5.0, thanks. > --- > hw/ppc/spapr.c | 2 ++ > hw/ppc/spapr_rtas.c | 14 +++++++++++++- > include/hw/ppc/spapr.h | 3 ++- > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b03b26370d..5f93c49706 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine) > > spapr->cas_reboot = false; > > + spapr->fwnmi_system_reset_addr = -1; > spapr->fwnmi_machine_check_addr = -1; > spapr->fwnmi_machine_check_interlock = -1; > > @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = { > .needed = spapr_fwnmi_needed, > .pre_save = spapr_fwnmi_pre_save, > .fields = (VMStateField[]) { > + VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState), > VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState), > VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState), > VMSTATE_END_OF_LIST() > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 0b8c481593..521e6b0b72 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > hwaddr rtas_addr; > + target_ulong sreset_addr, mce_addr; > > if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) { > rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, > return; > } > > - spapr->fwnmi_machine_check_addr = rtas_ld(args, 1); > + sreset_addr = rtas_ld(args, 0); > + mce_addr = rtas_ld(args, 1); > + > + /* PAPR requires these are in the first 32M of memory and within RMA */ > + if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size || > + mce_addr >= 32 * MiB || mce_addr >= spapr->rma_size) { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + spapr->fwnmi_system_reset_addr = sreset_addr; > + spapr->fwnmi_machine_check_addr = mce_addr; > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 64b83402cb..42d64a0368 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -194,9 +194,10 @@ struct SpaprMachineState { > > /* State related to FWNMI option */ > > - /* Machine Check Notification Routine address > + /* System Reset and Machine Check Notification Routine addresses > * registered by "ibm,nmi-register" RTAS call. > */ > + target_ulong fwnmi_system_reset_addr; > target_ulong fwnmi_machine_check_addr; > > /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin ` (2 preceding siblings ...) 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 17:59 ` Cédric Le Goater 2020-03-16 14:26 ` [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG Nicholas Piggin ` (3 subsequent siblings) 7 siblings, 1 reply; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson FWNMI machine check delivery misses a few things that will make it fail with TCG at least (which we would like to allow in future to improve testing). It's not nice to scatter interrupt delivery logic around the tree, so move it to excp_helper.c and share code where possible. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_events.c | 24 +++---------- target/ppc/cpu.h | 1 + target/ppc/excp_helper.c | 74 ++++++++++++++++++++++++++++------------ 3 files changed, 57 insertions(+), 42 deletions(-) diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 27ba8a2c19..323fcef4aa 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -785,28 +785,13 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered, static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) { SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); - uint64_t rtas_addr; + CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; - PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); - target_ulong msr = 0; + uint64_t rtas_addr; struct rtas_error_log log; struct mc_extended_log *ext_elog; uint32_t summary; - /* - * Properly set bits in MSR before we invoke the handler. - * SRR0/1, DAR and DSISR are properly set by KVM - */ - if (!(*pcc->interrupts_big_endian)(cpu)) { - msr |= (1ULL << MSR_LE); - } - - if (env->msr & (1ULL << MSR_SF)) { - msr |= (1ULL << MSR_SF); - } - - msr |= (1ULL << MSR_ME); - ext_elog = g_malloc0(sizeof(*ext_elog)); summary = spapr_mce_get_elog_type(cpu, recovered, ext_elog); @@ -834,12 +819,11 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) cpu_physical_memory_write(rtas_addr + RTAS_ERROR_LOG_OFFSET + sizeof(env->gpr[3]) + sizeof(log), ext_elog, sizeof(*ext_elog)); + g_free(ext_elog); env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; - env->msr = msr; - env->nip = spapr->fwnmi_machine_check_addr; - g_free(ext_elog); + ppc_cpu_do_fwnmi_machine_check(cs, spapr->fwnmi_machine_check_addr); } void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 5a55fb02bd..3953680534 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1221,6 +1221,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, int cpuid, void *opaque); #ifndef CONFIG_USER_ONLY void ppc_cpu_do_system_reset(CPUState *cs); +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); extern const VMStateDescription vmstate_ppc_cpu; #endif diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 027f54c0ed..7f2b5899d3 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -128,6 +128,37 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail) return offset; } +static inline void powerpc_set_excp_state(PowerPCCPU *cpu, + target_ulong vector, target_ulong msr) +{ + CPUState *cs = CPU(cpu); + CPUPPCState *env = &cpu->env; + + /* + * We don't use hreg_store_msr here as already have treated any + * special case that could occur. Just store MSR and update hflags + * + * Note: We *MUST* not use hreg_store_msr() as-is anyway because it + * will prevent setting of the HV bit which some exceptions might need + * to do. + */ + env->msr = msr & env->msr_mask; + hreg_compute_hflags(env); + env->nip = vector; + /* Reset exception state */ + cs->exception_index = POWERPC_EXCP_NONE; + env->error_code = 0; + + /* Reset the reservation */ + env->reserve_addr = -1; + + /* + * Any interrupt is context synchronizing, check if TCG TLB needs + * a delayed flush on ppc64 + */ + check_tlb_flush(env, false); +} + /* * Note that this function should be greatly optimized when called * with a constant excp, from ppc_hw_interrupt @@ -768,29 +799,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } } #endif - /* - * We don't use hreg_store_msr here as already have treated any - * special case that could occur. Just store MSR and update hflags - * - * Note: We *MUST* not use hreg_store_msr() as-is anyway because it - * will prevent setting of the HV bit which some exceptions might need - * to do. - */ - env->msr = new_msr & env->msr_mask; - hreg_compute_hflags(env); - env->nip = vector; - /* Reset exception state */ - cs->exception_index = POWERPC_EXCP_NONE; - env->error_code = 0; - /* Reset the reservation */ - env->reserve_addr = -1; - - /* - * Any interrupt is context synchronizing, check if TCG TLB needs - * a delayed flush on ppc64 - */ - check_tlb_flush(env, false); + powerpc_set_excp_state(cpu, vector, new_msr); } void ppc_cpu_do_interrupt(CPUState *cs) @@ -958,6 +968,26 @@ void ppc_cpu_do_system_reset(CPUState *cs) powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); } + +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) +{ + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); + target_ulong msr = 0; + + /* + * Set MSR and NIP for the handler, SRR0/1, DAR and DSISR have already + * been set by KVM. + */ + msr = (1ULL << MSR_ME); + msr |= env->msr & (1ULL << MSR_SF); + if (!(*pcc->interrupts_big_endian)(cpu)) { + msr |= (1ULL << MSR_LE); + } + + powerpc_set_excp_state(cpu, vector, msr); +} #endif /* !CONFIG_USER_ONLY */ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery 2020-03-16 14:26 ` [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery Nicholas Piggin @ 2020-03-16 17:59 ` Cédric Le Goater 2020-03-16 23:19 ` Nicholas Piggin 0 siblings, 1 reply; 38+ messages in thread From: Cédric Le Goater @ 2020-03-16 17:59 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Aravinda Prasad, David Gibson, Ganesh Goudar, qemu-devel, Greg Kurz On 3/16/20 3:26 PM, Nicholas Piggin wrote: > FWNMI machine check delivery misses a few things that will make it fail > with TCG at least (which we would like to allow in future to improve > testing). I don't understand which issues are addressed in the patch. > It's not nice to scatter interrupt delivery logic around the tree, so > move it to excp_helper.c and share code where possible. It looks correct but this is touching the ugliest routine in the QEMU PPC universe. I would split the patch in two to introduce the helper powerpc_set_excp_state(). It does not seem to need to be an inline also. C. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr_events.c | 24 +++---------- > target/ppc/cpu.h | 1 + > target/ppc/excp_helper.c | 74 ++++++++++++++++++++++++++++------------ > 3 files changed, 57 insertions(+), 42 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 27ba8a2c19..323fcef4aa 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -785,28 +785,13 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, bool recovered, > static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > { > SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > - uint64_t rtas_addr; > + CPUState *cs = CPU(cpu); > CPUPPCState *env = &cpu->env; > - PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - target_ulong msr = 0; > + uint64_t rtas_addr; > struct rtas_error_log log; > struct mc_extended_log *ext_elog; > uint32_t summary; > > - /* > - * Properly set bits in MSR before we invoke the handler. > - * SRR0/1, DAR and DSISR are properly set by KVM > - */ > - if (!(*pcc->interrupts_big_endian)(cpu)) { > - msr |= (1ULL << MSR_LE); > - } > - > - if (env->msr & (1ULL << MSR_SF)) { > - msr |= (1ULL << MSR_SF); > - } > - > - msr |= (1ULL << MSR_ME); > - > ext_elog = g_malloc0(sizeof(*ext_elog)); > summary = spapr_mce_get_elog_type(cpu, recovered, ext_elog); > > @@ -834,12 +819,11 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered) > cpu_physical_memory_write(rtas_addr + RTAS_ERROR_LOG_OFFSET + > sizeof(env->gpr[3]) + sizeof(log), ext_elog, > sizeof(*ext_elog)); > + g_free(ext_elog); > > env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET; > - env->msr = msr; > - env->nip = spapr->fwnmi_machine_check_addr; > > - g_free(ext_elog); > + ppc_cpu_do_fwnmi_machine_check(cs, spapr->fwnmi_machine_check_addr); > } > > void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 5a55fb02bd..3953680534 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1221,6 +1221,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > int cpuid, void *opaque); > #ifndef CONFIG_USER_ONLY > void ppc_cpu_do_system_reset(CPUState *cs); > +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); > extern const VMStateDescription vmstate_ppc_cpu; > #endif > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 027f54c0ed..7f2b5899d3 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -128,6 +128,37 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail) > return offset; > } > > +static inline void powerpc_set_excp_state(PowerPCCPU *cpu, > + target_ulong vector, target_ulong msr) > +{ > + CPUState *cs = CPU(cpu); > + CPUPPCState *env = &cpu->env; > + > + /* > + * We don't use hreg_store_msr here as already have treated any > + * special case that could occur. Just store MSR and update hflags > + * > + * Note: We *MUST* not use hreg_store_msr() as-is anyway because it > + * will prevent setting of the HV bit which some exceptions might need > + * to do. > + */ > + env->msr = msr & env->msr_mask; > + hreg_compute_hflags(env); > + env->nip = vector; > + /* Reset exception state */ > + cs->exception_index = POWERPC_EXCP_NONE; > + env->error_code = 0; > + > + /* Reset the reservation */ > + env->reserve_addr = -1; > + > + /* > + * Any interrupt is context synchronizing, check if TCG TLB needs > + * a delayed flush on ppc64 > + */ > + check_tlb_flush(env, false); > +} > + > /* > * Note that this function should be greatly optimized when called > * with a constant excp, from ppc_hw_interrupt > @@ -768,29 +799,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > } > #endif > - /* > - * We don't use hreg_store_msr here as already have treated any > - * special case that could occur. Just store MSR and update hflags > - * > - * Note: We *MUST* not use hreg_store_msr() as-is anyway because it > - * will prevent setting of the HV bit which some exceptions might need > - * to do. > - */ > - env->msr = new_msr & env->msr_mask; > - hreg_compute_hflags(env); > - env->nip = vector; > - /* Reset exception state */ > - cs->exception_index = POWERPC_EXCP_NONE; > - env->error_code = 0; > > - /* Reset the reservation */ > - env->reserve_addr = -1; > - > - /* > - * Any interrupt is context synchronizing, check if TCG TLB needs > - * a delayed flush on ppc64 > - */ > - check_tlb_flush(env, false); > + powerpc_set_excp_state(cpu, vector, new_msr); > } > > void ppc_cpu_do_interrupt(CPUState *cs) > @@ -958,6 +968,26 @@ void ppc_cpu_do_system_reset(CPUState *cs) > > powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); > } > + > +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + CPUPPCState *env = &cpu->env; > + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > + target_ulong msr = 0; > + > + /* > + * Set MSR and NIP for the handler, SRR0/1, DAR and DSISR have already > + * been set by KVM. > + */ > + msr = (1ULL << MSR_ME); > + msr |= env->msr & (1ULL << MSR_SF); > + if (!(*pcc->interrupts_big_endian)(cpu)) { > + msr |= (1ULL << MSR_LE); > + } > + > + powerpc_set_excp_state(cpu, vector, msr); > +} > #endif /* !CONFIG_USER_ONLY */ > > bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery 2020-03-16 17:59 ` Cédric Le Goater @ 2020-03-16 23:19 ` Nicholas Piggin 2020-03-16 23:27 ` David Gibson 0 siblings, 1 reply; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 23:19 UTC (permalink / raw) To: Cédric Le Goater, qemu-ppc Cc: Aravinda Prasad, qemu-devel, Ganesh Goudar, Greg Kurz, David Gibson Cédric Le Goater's on March 17, 2020 3:59 am: > On 3/16/20 3:26 PM, Nicholas Piggin wrote: >> FWNMI machine check delivery misses a few things that will make it fail >> with TCG at least (which we would like to allow in future to improve >> testing). > > I don't understand which issues are addressed in the patch. The existing code does not compute hflags, at least. There's a few possible other things, I didn't dig into qemu enough to know if they might be a problem (e.g., reservation and TLB). I figure it's better to keep these consistent. Keep in mind this is a bit academic right now, because we can't (AFAIKS) inject an MCE from TCG. It would be good to wire that up, but I didn't get to it. >> It's not nice to scatter interrupt delivery logic around the tree, so >> move it to excp_helper.c and share code where possible. > > It looks correct but this is touching the ugliest routine in the QEMU > PPC universe. I would split the patch in two to introduce the helper > powerpc_set_excp_state(). > > It does not seem to need to be an inline also. Yeah it's all pretty ugly. I didn't yet find a nice way to do split things up that did not require a lot of code churn, but that can come later. Inline was just because powerpc_excp is inline, I didn't want to change behaviour too much there (it obviously wants to do a lot of constant propagation but maybe only on the case statement). Anyway I just wanted to be minimal for now, it could be changed. Thanks, Nick ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery 2020-03-16 23:19 ` Nicholas Piggin @ 2020-03-16 23:27 ` David Gibson 0 siblings, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 23:27 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, qemu-devel, Greg Kurz, Ganesh Goudar, Cédric Le Goater, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 1977 bytes --] On Tue, Mar 17, 2020 at 09:19:57AM +1000, Nicholas Piggin wrote: > Cédric Le Goater's on March 17, 2020 3:59 am: > > On 3/16/20 3:26 PM, Nicholas Piggin wrote: > >> FWNMI machine check delivery misses a few things that will make it fail > >> with TCG at least (which we would like to allow in future to improve > >> testing). > > > > I don't understand which issues are addressed in the patch. > > The existing code does not compute hflags, at least. > > There's a few possible other things, I didn't dig into qemu enough > to know if they might be a problem (e.g., reservation and TLB). I > figure it's better to keep these consistent. > > Keep in mind this is a bit academic right now, because we can't > (AFAIKS) inject an MCE from TCG. It would be good to wire that up, > but I didn't get to it. > > >> It's not nice to scatter interrupt delivery logic around the tree, so > >> move it to excp_helper.c and share code where possible. > > > > It looks correct but this is touching the ugliest routine in the QEMU > > PPC universe. I would split the patch in two to introduce the helper > > powerpc_set_excp_state(). > > > > It does not seem to need to be an inline also. > > Yeah it's all pretty ugly. I didn't yet find a nice way to do > split things up that did not require a lot of code churn, but that > can come later. > > Inline was just because powerpc_excp is inline, I didn't want to > change behaviour too much there (it obviously wants to do a lot of > constant propagation but maybe only on the case statement). Anyway > I just wanted to be minimal for now, it could be changed. Yeah, I definitely want to get this in, so despite imperfections that could probably be polished with time, I've applied to ppc-for-5.0. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin ` (3 preceding siblings ...) 2020-03-16 14:26 ` [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 18:00 ` Cédric Le Goater 2020-03-16 18:01 ` Greg Kurz 2020-03-16 14:26 ` [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector Nicholas Piggin ` (2 subsequent siblings) 7 siblings, 2 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson There should no longer be a reason to prevent TCG providing FWNMI. System Reset interrupts are generated to the guest with nmi monitor command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected currently, but this could be implemented with the mce monitor cmd similarly to i386. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_caps.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index f626d769a0..679ae7959f 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, return; /* Disabled by default */ } - if (tcg_enabled()) { - warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not " - "supported in TCG"); - } else if (kvm_enabled()) { + if (kvm_enabled()) { if (kvmppc_set_fwnmi() < 0) { error_setg(errp, "Firmware Assisted Non-Maskable Interrupts(FWNMI) " "not supported by KVM"); -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG 2020-03-16 14:26 ` [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG Nicholas Piggin @ 2020-03-16 18:00 ` Cédric Le Goater 2020-03-16 18:01 ` Greg Kurz 1 sibling, 0 replies; 38+ messages in thread From: Cédric Le Goater @ 2020-03-16 18:00 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Aravinda Prasad, David Gibson, Ganesh Goudar, qemu-devel, Greg Kurz On 3/16/20 3:26 PM, Nicholas Piggin wrote: > There should no longer be a reason to prevent TCG providing FWNMI. > System Reset interrupts are generated to the guest with nmi monitor > command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected > currently, but this could be implemented with the mce monitor cmd > similarly to i386. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr_caps.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index f626d769a0..679ae7959f 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > return; /* Disabled by default */ > } > > - if (tcg_enabled()) { > - warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not " > - "supported in TCG"); > - } else if (kvm_enabled()) { > + if (kvm_enabled()) { > if (kvmppc_set_fwnmi() < 0) { > error_setg(errp, "Firmware Assisted Non-Maskable Interrupts(FWNMI) " > "not supported by KVM"); > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG 2020-03-16 14:26 ` [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG Nicholas Piggin 2020-03-16 18:00 ` Cédric Le Goater @ 2020-03-16 18:01 ` Greg Kurz 2020-03-16 23:26 ` Nicholas Piggin 1 sibling, 1 reply; 38+ messages in thread From: Greg Kurz @ 2020-03-16 18:01 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Ganesh Goudar, qemu-ppc, David Gibson On Tue, 17 Mar 2020 00:26:10 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > There should no longer be a reason to prevent TCG providing FWNMI. > System Reset interrupts are generated to the guest with nmi monitor > command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected > currently, but this could be implemented with the mce monitor cmd > similarly to i386. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr_caps.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index f626d769a0..679ae7959f 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > return; /* Disabled by default */ > } > > - if (tcg_enabled()) { > - warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not " > - "supported in TCG"); With this warning removed, we can now drop the "cap-fwnmi=off" setting in qtest, but this can be done as a followup. Reviewed-by: Greg Kurz <groug@kaod.org> > - } else if (kvm_enabled()) { > + if (kvm_enabled()) { > if (kvmppc_set_fwnmi() < 0) { > error_setg(errp, "Firmware Assisted Non-Maskable Interrupts(FWNMI) " > "not supported by KVM"); ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG 2020-03-16 18:01 ` Greg Kurz @ 2020-03-16 23:26 ` Nicholas Piggin 2020-03-17 3:56 ` David? Gibson 0 siblings, 1 reply; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 23:26 UTC (permalink / raw) To: Greg Kurz Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, qemu-ppc, Ganesh Goudar, David, Gibson Greg Kurz's on March 17, 2020 4:01 am: > On Tue, 17 Mar 2020 00:26:10 +1000 > Nicholas Piggin <npiggin@gmail.com> wrote: > >> There should no longer be a reason to prevent TCG providing FWNMI. >> System Reset interrupts are generated to the guest with nmi monitor >> command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected >> currently, but this could be implemented with the mce monitor cmd >> similarly to i386. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> hw/ppc/spapr_caps.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >> index f626d769a0..679ae7959f 100644 >> --- a/hw/ppc/spapr_caps.c >> +++ b/hw/ppc/spapr_caps.c >> @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, >> return; /* Disabled by default */ >> } >> >> - if (tcg_enabled()) { >> - warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not " >> - "supported in TCG"); > > With this warning removed, we can now drop the "cap-fwnmi=off" setting > in qtest, but this can be done as a followup. Ah right, thanks. Would you send the patch later or should I? Thanks, Nick ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG 2020-03-16 23:26 ` Nicholas Piggin @ 2020-03-17 3:56 ` David? Gibson 0 siblings, 0 replies; 38+ messages in thread From: David? Gibson @ 2020-03-17 3:56 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, Greg Kurz, qemu-devel, qemu-ppc, Ganesh Goudar [-- Attachment #1: Type: text/plain, Size: 1669 bytes --] On Tue, Mar 17, 2020 at 09:26:15AM +1000, Nicholas Piggin wrote: > Greg Kurz's on March 17, 2020 4:01 am: > > On Tue, 17 Mar 2020 00:26:10 +1000 > > Nicholas Piggin <npiggin@gmail.com> wrote: > > > >> There should no longer be a reason to prevent TCG providing FWNMI. > >> System Reset interrupts are generated to the guest with nmi monitor > >> command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected > >> currently, but this could be implemented with the mce monitor cmd > >> similarly to i386. > >> > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >> --- > >> hw/ppc/spapr_caps.c | 5 +---- > >> 1 file changed, 1 insertion(+), 4 deletions(-) > >> > >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >> index f626d769a0..679ae7959f 100644 > >> --- a/hw/ppc/spapr_caps.c > >> +++ b/hw/ppc/spapr_caps.c > >> @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > >> return; /* Disabled by default */ > >> } > >> > >> - if (tcg_enabled()) { > >> - warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not " > >> - "supported in TCG"); > > > > With this warning removed, we can now drop the "cap-fwnmi=off" setting > > in qtest, but this can be done as a followup. > > Ah right, thanks. Would you send the patch later or should I? No need, I already folded the change into your patch. > > Thanks, > Nick > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin ` (4 preceding siblings ...) 2020-03-16 14:26 ` [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 18:04 ` Greg Kurz 2020-03-16 18:15 ` Cédric Le Goater 2020-03-16 14:26 ` [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery Nicholas Piggin 2020-03-16 14:26 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm, nmi-interlock" Linux bug Nicholas Piggin 7 siblings, 2 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson Provide for an alternate delivery location, -1 defaults to the architected address. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr.c | 2 +- target/ppc/cpu.h | 2 +- target/ppc/excp_helper.c | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 5f93c49706..25221d843c 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3400,7 +3400,7 @@ static void spapr_machine_finalizefn(Object *obj) void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) { cpu_synchronize_state(cs); - ppc_cpu_do_system_reset(cs); + ppc_cpu_do_system_reset(cs, -1); } static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 3953680534..f8c7d6f19c 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, int cpuid, void *opaque); #ifndef CONFIG_USER_ONLY -void ppc_cpu_do_system_reset(CPUState *cs); +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector); void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); extern const VMStateDescription vmstate_ppc_cpu; #endif diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 7f2b5899d3..08bc885ca6 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -961,12 +961,15 @@ static void ppc_hw_interrupt(CPUPPCState *env) } } -void ppc_cpu_do_system_reset(CPUState *cs) +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); + if (vector != -1) { + env->nip = vector; + } } void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 14:26 ` [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector Nicholas Piggin @ 2020-03-16 18:04 ` Greg Kurz 2020-03-16 18:15 ` Cédric Le Goater 1 sibling, 0 replies; 38+ messages in thread From: Greg Kurz @ 2020-03-16 18:04 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Ganesh Goudar, qemu-ppc, David Gibson On Tue, 17 Mar 2020 00:26:11 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > Provide for an alternate delivery location, -1 defaults to the > architected address. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr.c | 2 +- > target/ppc/cpu.h | 2 +- > target/ppc/excp_helper.c | 5 ++++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 5f93c49706..25221d843c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3400,7 +3400,7 @@ static void spapr_machine_finalizefn(Object *obj) > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > { > cpu_synchronize_state(cs); > - ppc_cpu_do_system_reset(cs); > + ppc_cpu_do_system_reset(cs, -1); > } > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 3953680534..f8c7d6f19c 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > int cpuid, void *opaque); > #ifndef CONFIG_USER_ONLY > -void ppc_cpu_do_system_reset(CPUState *cs); > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector); > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); > extern const VMStateDescription vmstate_ppc_cpu; > #endif > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 7f2b5899d3..08bc885ca6 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -961,12 +961,15 @@ static void ppc_hw_interrupt(CPUPPCState *env) > } > } > > -void ppc_cpu_do_system_reset(CPUState *cs) > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector) > { > PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = &cpu->env; > > powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); > + if (vector != -1) { > + env->nip = vector; > + } > } > > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 14:26 ` [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector Nicholas Piggin 2020-03-16 18:04 ` Greg Kurz @ 2020-03-16 18:15 ` Cédric Le Goater 2020-03-16 23:28 ` Nicholas Piggin 2020-03-16 23:28 ` David Gibson 1 sibling, 2 replies; 38+ messages in thread From: Cédric Le Goater @ 2020-03-16 18:15 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Aravinda Prasad, David Gibson, Ganesh Goudar, qemu-devel, Greg Kurz On 3/16/20 3:26 PM, Nicholas Piggin wrote: > Provide for an alternate delivery location, -1 defaults to the > architected address. I don't know what is the best approach, to override the vector addr computed by powerpc_excp() or use a machine class handler with cpu->vhyp. > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr.c | 2 +- > target/ppc/cpu.h | 2 +- > target/ppc/excp_helper.c | 5 ++++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 5f93c49706..25221d843c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3400,7 +3400,7 @@ static void spapr_machine_finalizefn(Object *obj) > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > { > cpu_synchronize_state(cs); > - ppc_cpu_do_system_reset(cs); > + ppc_cpu_do_system_reset(cs, -1); > } > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 3953680534..f8c7d6f19c 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > int cpuid, void *opaque); > #ifndef CONFIG_USER_ONLY > -void ppc_cpu_do_system_reset(CPUState *cs); > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector); > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); > extern const VMStateDescription vmstate_ppc_cpu; > #endif > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 7f2b5899d3..08bc885ca6 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -961,12 +961,15 @@ static void ppc_hw_interrupt(CPUPPCState *env) > } > } > > -void ppc_cpu_do_system_reset(CPUState *cs) > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector) > { > PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = &cpu->env; > > powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); > + if (vector != -1) { > + env->nip = vector; > + } > } > > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 18:15 ` Cédric Le Goater @ 2020-03-16 23:28 ` Nicholas Piggin 2020-03-16 23:34 ` David Gibson 2020-03-16 23:28 ` David Gibson 1 sibling, 1 reply; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 23:28 UTC (permalink / raw) To: Cédric Le Goater, qemu-ppc Cc: Aravinda Prasad, qemu-devel, Ganesh Goudar, Greg Kurz, David Gibson Cédric Le Goater's on March 17, 2020 4:15 am: > On 3/16/20 3:26 PM, Nicholas Piggin wrote: >> Provide for an alternate delivery location, -1 defaults to the >> architected address. > > I don't know what is the best approach, to override the vector addr > computed by powerpc_excp() or use a machine class handler with > cpu->vhyp. Yeah it's getting a bit ad hoc and inconsistent with machine check etc, I just figured get something minimal in there now. The whole exception delivery needs a spring clean though. Thanks, Nick ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 23:28 ` Nicholas Piggin @ 2020-03-16 23:34 ` David Gibson 2020-03-17 10:47 ` Cédric Le Goater 0 siblings, 1 reply; 38+ messages in thread From: David Gibson @ 2020-03-16 23:34 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, qemu-devel, Greg Kurz, Ganesh Goudar, Cédric Le Goater, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 1065 bytes --] On Tue, Mar 17, 2020 at 09:28:24AM +1000, Nicholas Piggin wrote: > Cédric Le Goater's on March 17, 2020 4:15 am: > > On 3/16/20 3:26 PM, Nicholas Piggin wrote: > >> Provide for an alternate delivery location, -1 defaults to the > >> architected address. > > > > I don't know what is the best approach, to override the vector addr > > computed by powerpc_excp() or use a machine class handler with > > cpu->vhyp. > > Yeah it's getting a bit ad hoc and inconsistent with machine check > etc, I just figured get something minimal in there now. The whole > exception delivery needs a spring clean though. Yeah, there's a huge amount of cruft in nearly all the softmmu code. It's such a big task that I don't really have any plans to tackle it specifically. Instead I've been cleaning up little pieces as they impinge on things I actually care about. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 23:34 ` David Gibson @ 2020-03-17 10:47 ` Cédric Le Goater 0 siblings, 0 replies; 38+ messages in thread From: Cédric Le Goater @ 2020-03-17 10:47 UTC (permalink / raw) To: David Gibson, Nicholas Piggin Cc: Aravinda Prasad, Ganesh Goudar, qemu-ppc, Greg Kurz, qemu-devel On 3/17/20 12:34 AM, David Gibson wrote: > On Tue, Mar 17, 2020 at 09:28:24AM +1000, Nicholas Piggin wrote: >> Cédric Le Goater's on March 17, 2020 4:15 am: >>> On 3/16/20 3:26 PM, Nicholas Piggin wrote: >>>> Provide for an alternate delivery location, -1 defaults to the >>>> architected address. >>> >>> I don't know what is the best approach, to override the vector addr >>> computed by powerpc_excp() or use a machine class handler with >>> cpu->vhyp. >> >> Yeah it's getting a bit ad hoc and inconsistent with machine check >> etc, I just figured get something minimal in there now. The whole >> exception delivery needs a spring clean though. > > Yeah, there's a huge amount of cruft in nearly all the softmmu code. The MMU emulation is not that bad to read. However, the exception model is hideous as one would say. powerpc_excp() is my favorite. > It's such a big task that I don't really have any plans to tackle it > specifically. Instead I've been cleaning up little pieces as they > impinge on things I actually care about. Maybe we should extract book3s to start with. C. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector 2020-03-16 18:15 ` Cédric Le Goater 2020-03-16 23:28 ` Nicholas Piggin @ 2020-03-16 23:28 ` David Gibson 1 sibling, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 23:28 UTC (permalink / raw) To: Cédric Le Goater Cc: Aravinda Prasad, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 2864 bytes --] On Mon, Mar 16, 2020 at 07:15:14PM +0100, Cédric Le Goater wrote: > On 3/16/20 3:26 PM, Nicholas Piggin wrote: > > Provide for an alternate delivery location, -1 defaults to the > > architected address. > > I don't know what is the best approach, to override the vector addr > computed by powerpc_excp() or use a machine class handler with > cpu->vhyp. Again, in the interests of getting this in for the soft freeze, I've applied this now. We can clean it up later. > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > hw/ppc/spapr.c | 2 +- > > target/ppc/cpu.h | 2 +- > > target/ppc/excp_helper.c | 5 ++++- > > 3 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 5f93c49706..25221d843c 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3400,7 +3400,7 @@ static void spapr_machine_finalizefn(Object *obj) > > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > > { > > cpu_synchronize_state(cs); > > - ppc_cpu_do_system_reset(cs); > > + ppc_cpu_do_system_reset(cs, -1); > > } > > > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > index 3953680534..f8c7d6f19c 100644 > > --- a/target/ppc/cpu.h > > +++ b/target/ppc/cpu.h > > @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, > > int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > > int cpuid, void *opaque); > > #ifndef CONFIG_USER_ONLY > > -void ppc_cpu_do_system_reset(CPUState *cs); > > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector); > > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector); > > extern const VMStateDescription vmstate_ppc_cpu; > > #endif > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > > index 7f2b5899d3..08bc885ca6 100644 > > --- a/target/ppc/excp_helper.c > > +++ b/target/ppc/excp_helper.c > > @@ -961,12 +961,15 @@ static void ppc_hw_interrupt(CPUPPCState *env) > > } > > } > > > > -void ppc_cpu_do_system_reset(CPUState *cs) > > +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector) > > { > > PowerPCCPU *cpu = POWERPC_CPU(cs); > > CPUPPCState *env = &cpu->env; > > > > powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET); > > + if (vector != -1) { > > + env->nip = vector; > > + } > > } > > > > void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin ` (5 preceding siblings ...) 2020-03-16 14:26 ` [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 17:35 ` Mahesh J Salgaonkar 2020-03-16 23:30 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm, nmi-interlock" Linux bug Nicholas Piggin 7 siblings, 2 replies; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor delivers all system reset and machine check exceptions to the registered addresses. System Resets are delivered with registers set to the architected state, and with no interlock. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 25221d843c..78e649f47d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", maxdomains, sizeof(maxdomains))); - _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE)); + /* + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log, + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes. + * + * The system reset requirements are driven by existing Linux and PowerVM + * implementation which (contrary to PAPR) saves r3 in the error log + * structure like machine check, so Linux expects to find the saved r3 + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and + * does not look at the error value). + * + * System reset interrupts are not subject to interlock like machine + * check, so this memory area could be corrupted if the sreset is + * interrupted by a machine check (or vice versa) if it was shared. To + * prevent this, system reset uses per-CPU areas for the sreset save + * area. A system reset that interrupts a system reset handler could + * still overwrite this area, but Linux doesn't try to recover in that + * case anyway. + * + * The extra 8 bytes is required because Linux's FWNMI error log check + * is off-by-one. + */ + _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX + + ms->smp.max_cpus * sizeof(uint64_t)*2 + sizeof(uint64_t))); _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max", RTAS_ERROR_LOG_MAX)); _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate", @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj) void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) { + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + cpu_synchronize_state(cs); - ppc_cpu_do_system_reset(cs, -1); + /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */ + if (spapr->fwnmi_system_reset_addr != -1) { + uint64_t rtas_addr, addr; + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + + /* get rtas addr from fdt */ + rtas_addr = spapr_get_rtas_addr(); + if (!rtas_addr) { + qemu_system_guest_panicked(NULL); + return; + } + + addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * sizeof(uint64_t)*2; + stq_be_phys(&address_space_memory, addr, env->gpr[3]); + stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0); + env->gpr[3] = addr; + } + ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr); } static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery 2020-03-16 14:26 ` [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery Nicholas Piggin @ 2020-03-16 17:35 ` Mahesh J Salgaonkar 2020-03-16 17:52 ` Greg Kurz 2020-03-16 23:30 ` David Gibson 1 sibling, 1 reply; 38+ messages in thread From: Mahesh J Salgaonkar @ 2020-03-16 17:35 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc, David Gibson On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote: > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor > delivers all system reset and machine check exceptions to the registered > addresses. > > System Resets are delivered with registers set to the architected state, > and with no interlock. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 25221d843c..78e649f47d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) > _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", > maxdomains, sizeof(maxdomains))); > > - _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE)); > + /* > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log, > + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes. > + * > + * The system reset requirements are driven by existing Linux and PowerVM > + * implementation which (contrary to PAPR) saves r3 in the error log > + * structure like machine check, so Linux expects to find the saved r3 > + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and > + * does not look at the error value). > + * > + * System reset interrupts are not subject to interlock like machine > + * check, so this memory area could be corrupted if the sreset is > + * interrupted by a machine check (or vice versa) if it was shared. To > + * prevent this, system reset uses per-CPU areas for the sreset save > + * area. A system reset that interrupts a system reset handler could > + * still overwrite this area, but Linux doesn't try to recover in that > + * case anyway. > + * > + * The extra 8 bytes is required because Linux's FWNMI error log check > + * is off-by-one. > + */ > + _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX + > + ms->smp.max_cpus * sizeof(uint64_t)*2 + sizeof(uint64_t))); Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX). Do we need SLOF change to increase rtas area as well ? Otherwise QEMU may corrupt guest memory area OR Am I wrong ? Thanks, -Mahesh/ > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max", > RTAS_ERROR_LOG_MAX)); > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate", > @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj) > > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > { > + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + > cpu_synchronize_state(cs); > - ppc_cpu_do_system_reset(cs, -1); > + /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */ > + if (spapr->fwnmi_system_reset_addr != -1) { > + uint64_t rtas_addr, addr; > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + CPUPPCState *env = &cpu->env; > + > + /* get rtas addr from fdt */ > + rtas_addr = spapr_get_rtas_addr(); > + if (!rtas_addr) { > + qemu_system_guest_panicked(NULL); > + return; > + } > + > + addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * sizeof(uint64_t)*2; > + stq_be_phys(&address_space_memory, addr, env->gpr[3]); > + stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0); > + env->gpr[3] = addr; > + } > + ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr); > } > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) > -- > 2.23.0 > > -- Mahesh J Salgaonkar ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery 2020-03-16 17:35 ` Mahesh J Salgaonkar @ 2020-03-16 17:52 ` Greg Kurz 2020-03-16 23:31 ` David Gibson 0 siblings, 1 reply; 38+ messages in thread From: Greg Kurz @ 2020-03-16 17:52 UTC (permalink / raw) To: Mahesh J Salgaonkar Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Ganesh Goudar, qemu-ppc, David Gibson On Mon, 16 Mar 2020 23:05:00 +0530 Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote: > On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote: > > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor > > delivers all system reset and machine check exceptions to the registered > > addresses. > > > > System Resets are delivered with registers set to the architected state, > > and with no interlock. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 44 insertions(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 25221d843c..78e649f47d 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) > > _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", > > maxdomains, sizeof(maxdomains))); > > > > - _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE)); > > + /* > > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log, > > + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes. > > + * > > + * The system reset requirements are driven by existing Linux and PowerVM > > + * implementation which (contrary to PAPR) saves r3 in the error log > > + * structure like machine check, so Linux expects to find the saved r3 > > + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and > > + * does not look at the error value). > > + * > > + * System reset interrupts are not subject to interlock like machine > > + * check, so this memory area could be corrupted if the sreset is > > + * interrupted by a machine check (or vice versa) if it was shared. To > > + * prevent this, system reset uses per-CPU areas for the sreset save > > + * area. A system reset that interrupts a system reset handler could > > + * still overwrite this area, but Linux doesn't try to recover in that > > + * case anyway. > > + * > > + * The extra 8 bytes is required because Linux's FWNMI error log check > > + * is off-by-one. > > + */ > > + _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX + > > + ms->smp.max_cpus * sizeof(uint64_t)*2 + sizeof(uint64_t))); > > Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX). > Do we need SLOF change to increase rtas area as well ? Otherwise QEMU > may corrupt guest memory area OR Am I wrong ? > A change is pending for SLOF to use the "rtas-size" property provided by QEMU: https://patchwork.ozlabs.org/patch/1255264/ > Thanks, > -Mahesh/ > > > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max", > > RTAS_ERROR_LOG_MAX)); > > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate", > > @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj) > > > > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > > { > > + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > + > > cpu_synchronize_state(cs); > > - ppc_cpu_do_system_reset(cs, -1); > > + /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */ > > + if (spapr->fwnmi_system_reset_addr != -1) { > > + uint64_t rtas_addr, addr; > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + CPUPPCState *env = &cpu->env; > > + > > + /* get rtas addr from fdt */ > > + rtas_addr = spapr_get_rtas_addr(); > > + if (!rtas_addr) { > > + qemu_system_guest_panicked(NULL); > > + return; > > + } > > + > > + addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * sizeof(uint64_t)*2; > > + stq_be_phys(&address_space_memory, addr, env->gpr[3]); > > + stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0); > > + env->gpr[3] = addr; > > + } > > + ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr); > > } > > > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) > > -- > > 2.23.0 > > > > > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery 2020-03-16 17:52 ` Greg Kurz @ 2020-03-16 23:31 ` David Gibson 0 siblings, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 23:31 UTC (permalink / raw) To: Greg Kurz Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, qemu-ppc, Ganesh Goudar [-- Attachment #1: Type: text/plain, Size: 3402 bytes --] On Mon, Mar 16, 2020 at 06:52:54PM +0100, Greg Kurz wrote: > On Mon, 16 Mar 2020 23:05:00 +0530 > Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote: > > > On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote: > > > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor > > > delivers all system reset and machine check exceptions to the registered > > > addresses. > > > > > > System Resets are delivered with registers set to the architected state, > > > and with no interlock. > > > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > > --- > > > hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 44 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 25221d843c..78e649f47d 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) > > > _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", > > > maxdomains, sizeof(maxdomains))); > > > > > > - _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE)); > > > + /* > > > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log, > > > + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes. > > > + * > > > + * The system reset requirements are driven by existing Linux and PowerVM > > > + * implementation which (contrary to PAPR) saves r3 in the error log > > > + * structure like machine check, so Linux expects to find the saved r3 > > > + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and > > > + * does not look at the error value). > > > + * > > > + * System reset interrupts are not subject to interlock like machine > > > + * check, so this memory area could be corrupted if the sreset is > > > + * interrupted by a machine check (or vice versa) if it was shared. To > > > + * prevent this, system reset uses per-CPU areas for the sreset save > > > + * area. A system reset that interrupts a system reset handler could > > > + * still overwrite this area, but Linux doesn't try to recover in that > > > + * case anyway. > > > + * > > > + * The extra 8 bytes is required because Linux's FWNMI error log check > > > + * is off-by-one. > > > + */ > > > + _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX + > > > + ms->smp.max_cpus * sizeof(uint64_t)*2 + sizeof(uint64_t))); > > > > Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX). > > Do we need SLOF change to increase rtas area as well ? Otherwise QEMU > > may corrupt guest memory area OR Am I wrong ? > > > > A change is pending for SLOF to use the "rtas-size" property > provided by QEMU: > > https://patchwork.ozlabs.org/patch/1255264/ In the meantime, this is still correct. Because we rebuild the device tree at CAS time, the qemu supplied value will be the one the guest sees in the end. We obviously want that qemu update to avoid confusion, but we don't need it for things to work. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery 2020-03-16 14:26 ` [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery Nicholas Piggin 2020-03-16 17:35 ` Mahesh J Salgaonkar @ 2020-03-16 23:30 ` David Gibson 1 sibling, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 23:30 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 3879 bytes --] On Tue, Mar 17, 2020 at 12:26:12AM +1000, Nicholas Piggin wrote: > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor > delivers all system reset and machine check exceptions to the registered > addresses. > > System Resets are delivered with registers set to the architected state, > and with no interlock. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied to ppc-for-5.0. > --- > hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 25221d843c..78e649f47d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) > _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", > maxdomains, sizeof(maxdomains))); > > - _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE)); > + /* > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log, > + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes. > + * > + * The system reset requirements are driven by existing Linux and PowerVM > + * implementation which (contrary to PAPR) saves r3 in the error log > + * structure like machine check, so Linux expects to find the saved r3 > + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and > + * does not look at the error value). > + * > + * System reset interrupts are not subject to interlock like machine > + * check, so this memory area could be corrupted if the sreset is > + * interrupted by a machine check (or vice versa) if it was shared. To > + * prevent this, system reset uses per-CPU areas for the sreset save > + * area. A system reset that interrupts a system reset handler could > + * still overwrite this area, but Linux doesn't try to recover in that > + * case anyway. > + * > + * The extra 8 bytes is required because Linux's FWNMI error log check > + * is off-by-one. > + */ > + _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX + > + ms->smp.max_cpus * sizeof(uint64_t)*2 + sizeof(uint64_t))); > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max", > RTAS_ERROR_LOG_MAX)); > _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate", > @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj) > > void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg) > { > + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + > cpu_synchronize_state(cs); > - ppc_cpu_do_system_reset(cs, -1); > + /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */ > + if (spapr->fwnmi_system_reset_addr != -1) { > + uint64_t rtas_addr, addr; > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + CPUPPCState *env = &cpu->env; > + > + /* get rtas addr from fdt */ > + rtas_addr = spapr_get_rtas_addr(); > + if (!rtas_addr) { > + qemu_system_guest_panicked(NULL); > + return; > + } > + > + addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * sizeof(uint64_t)*2; > + stq_be_phys(&address_space_memory, addr, env->gpr[3]); > + stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0); > + env->gpr[3] = addr; > + } > + ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr); > } > > static void spapr_nmi(NMIState *n, int cpu_index, Error **errp) -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 8/8] ppc/spapr: Ignore common "ibm, nmi-interlock" Linux bug 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin ` (6 preceding siblings ...) 2020-03-16 14:26 ` [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery Nicholas Piggin @ 2020-03-16 14:26 ` Nicholas Piggin 2020-03-16 23:32 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm,nmi-interlock" " David Gibson 7 siblings, 1 reply; 38+ messages in thread From: Nicholas Piggin @ 2020-03-16 14:26 UTC (permalink / raw) To: qemu-ppc Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Nicholas Piggin, Greg Kurz, Ganesh Goudar, David Gibson Linux kernels call "ibm,nmi-interlock" in their system reset handlers contrary to PAPR. Returning an error because the CPU does not hold the interlock here causes Linux to print warning messages. PowerVM returns success in this case, so do the same for now. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_rtas.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 521e6b0b72..9fb8c8632a 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -461,8 +461,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, } if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { - /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ - rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + /* + * The vCPU that hit the NMI should invoke "ibm,nmi-interlock" + * This should be PARAM_ERROR, but Linux calls "ibm,nmi-interlock" + * for system reset interrupts, despite them not being interlocked. + * PowerVM silently ignores this and returns success here. Returning + * failure causes Linux to print the error "FWNMI: nmi-interlock + * failed: -3", although no other apparent ill effects, this is a + * regression for the user when enabling FWNMI. So for now, match + * PowerVM. When most Linux clients are fixed, this could be + * changed. + */ + rtas_st(rets, 0, RTAS_OUT_SUCCESS); return; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 8/8] ppc/spapr: Ignore common "ibm,nmi-interlock" Linux bug 2020-03-16 14:26 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm, nmi-interlock" Linux bug Nicholas Piggin @ 2020-03-16 23:32 ` David Gibson 0 siblings, 0 replies; 38+ messages in thread From: David Gibson @ 2020-03-16 23:32 UTC (permalink / raw) To: Nicholas Piggin Cc: Aravinda Prasad, Alexey Kardashevskiy, qemu-devel, Greg Kurz, Ganesh Goudar, qemu-ppc [-- Attachment #1: Type: text/plain, Size: 1860 bytes --] On Tue, Mar 17, 2020 at 12:26:13AM +1000, Nicholas Piggin wrote: > Linux kernels call "ibm,nmi-interlock" in their system reset handlers > contrary to PAPR. Returning an error because the CPU does not hold the > interlock here causes Linux to print warning messages. PowerVM returns > success in this case, so do the same for now. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied, thanks. > --- > hw/ppc/spapr_rtas.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 521e6b0b72..9fb8c8632a 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -461,8 +461,18 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, > } > > if (spapr->fwnmi_machine_check_interlock != cpu->vcpu_id) { > - /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ > - rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + /* > + * The vCPU that hit the NMI should invoke "ibm,nmi-interlock" > + * This should be PARAM_ERROR, but Linux calls "ibm,nmi-interlock" > + * for system reset interrupts, despite them not being interlocked. > + * PowerVM silently ignores this and returns success here. Returning > + * failure causes Linux to print the error "FWNMI: nmi-interlock > + * failed: -3", although no other apparent ill effects, this is a > + * regression for the user when enabling FWNMI. So for now, match > + * PowerVM. When most Linux clients are fixed, this could be > + * changed. > + */ > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > return; > } > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2020-03-17 10:51 UTC | newest] Thread overview: 38+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-16 14:26 [PATCH v2 0/8] FWNMI fixes / changes Nicholas Piggin 2020-03-16 14:26 ` [PATCH v2 1/8] ppc/spapr: Fix FWNMI machine check failure handling Nicholas Piggin 2020-03-16 15:27 ` Greg Kurz 2020-03-16 22:46 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 2/8] ppc/spapr: Change FWNMI names Nicholas Piggin 2020-03-16 17:15 ` Greg Kurz 2020-03-16 17:18 ` Mahesh J Salgaonkar 2020-03-16 17:25 ` Greg Kurz 2020-03-16 17:32 ` Cédric Le Goater 2020-03-16 22:46 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state Nicholas Piggin 2020-03-16 17:17 ` Greg Kurz 2020-03-16 17:29 ` Mahesh J Salgaonkar 2020-03-16 17:46 ` Cédric Le Goater 2020-03-16 22:47 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery Nicholas Piggin 2020-03-16 17:59 ` Cédric Le Goater 2020-03-16 23:19 ` Nicholas Piggin 2020-03-16 23:27 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG Nicholas Piggin 2020-03-16 18:00 ` Cédric Le Goater 2020-03-16 18:01 ` Greg Kurz 2020-03-16 23:26 ` Nicholas Piggin 2020-03-17 3:56 ` David? Gibson 2020-03-16 14:26 ` [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector Nicholas Piggin 2020-03-16 18:04 ` Greg Kurz 2020-03-16 18:15 ` Cédric Le Goater 2020-03-16 23:28 ` Nicholas Piggin 2020-03-16 23:34 ` David Gibson 2020-03-17 10:47 ` Cédric Le Goater 2020-03-16 23:28 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery Nicholas Piggin 2020-03-16 17:35 ` Mahesh J Salgaonkar 2020-03-16 17:52 ` Greg Kurz 2020-03-16 23:31 ` David Gibson 2020-03-16 23:30 ` David Gibson 2020-03-16 14:26 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm, nmi-interlock" Linux bug Nicholas Piggin 2020-03-16 23:32 ` [PATCH v2 8/8] ppc/spapr: Ignore common "ibm,nmi-interlock" " David Gibson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).