[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 01/13] ppc/spapr: Change FWNMI names
|
From: |
Greg Kurz |
|
Subject: |
Re: [RFC PATCH 01/13] ppc/spapr: Change FWNMI names |
|
Date: |
Wed, 4 Mar 2020 12:14:50 +0100 |
On Wed, 4 Mar 2020 15:28:38 +1000
Nicholas Piggin <address@hidden> 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 <address@hidden>
> ---
LGTM also. It just misses a change in tests/qtest/libqos/libqos-spapr.h :
/* List of capabilities needed to silence warnings with TCG */
#define PSERIES_DEFAULT_CAPABILITIES \
"cap-cfpc=broken," \
"cap-sbbc=broken," \
"cap-ibs=broken," \
"cap-ccf-assist=off," \
"cap-fwnmi-mce=off"
^^^^
> 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 +++++++++++++++++----------
> 5 files changed, 54 insertions(+), 46 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cc10798be4..27b0c2c425 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1696,11 +1696,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);
> }
> @@ -1989,7 +1989,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)
> @@ -2000,7 +2000,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");
> }
> @@ -2008,15 +2008,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()
> },
> };
> @@ -2055,7 +2055,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
> }
> };
> @@ -2869,7 +2869,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"
> @@ -3045,7 +3045,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)
> @@ -4526,7 +4526,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;
> @@ -4604,7 +4604,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;
> 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 8b32b7eea5..5f39190c13 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -839,7 +839,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);
> }
> @@ -851,7 +851,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
> @@ -863,19 +863,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;
> }
> }
> @@ -891,7 +891,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 656fdd2216..571615445a 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 09110961a5..0879b68676 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;