On Friday 29 September 2017 12:19 PM, David Gibson wrote: > I don't suppose there's a way to stop your mailer from inserting > spaces after the commas in the subject line,
Not sure how to stop the mailer from doing this as I don't have any spaces in the subject line of my actual patch. > > > On Thu, Sep 28, 2017 at 04:07:48PM +0530, Aravinda Prasad wrote: >> This patch adds support in QEMU to handle "ibm,nmi-register" >> and "ibm,nmi-interlock" RTAS calls. >> >> The machine check notification address is saved when the >> OS issues "ibm,nmi-register" RTAS call. >> >> This patch also handles the case when multiple processors >> experience machine check at or about the same time by >> handling "ibm,nmi-interlock" call. In such cases, as per >> PAPR, subsequent processors serialize waiting for the first >> processor to issue the "ibm,nmi-interlock" call. The second >> processor waits till the first processor, which also >> received a machine check error, is done reading the error >> log. The first processor issues "ibm,nmi-interlock" call >> when the error log is consumed. This patch implements the >> releasing part of the error-log while subsequent patch >> (which builds error log) handles the locking part. >> >> Signed-off-by: Aravinda Prasad <aravi...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr.c | 8 ++++++++ >> hw/ppc/spapr_rtas.c | 35 +++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr.h | 11 ++++++++++- >> 3 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 5deae30..d568ea6 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1477,6 +1477,11 @@ static void ppc_spapr_reset(void) >> first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT; >> >> spapr->cas_reboot = false; >> + >> + spapr->mc_status = -1; >> + spapr->guest_machine_check_addr = 0; > > You should probably use -1 as the default for this - strictly speaking > the guest could legitimately choose to have it's machine check > processing code at 0, though it would be a strange choice. Sure. > >> + qemu_cond_destroy(&spapr->mc_delivery_cond); >> + qemu_cond_init(&spapr->mc_delivery_cond); > > What will this do if one of the vcpus is waiting on the condition > variable right now? I suspect you're going to need to first wake up > the queue, and marshal any blocked vcpus back into dead mode instead > of this. Do we need to wake-up the queue as the system, anyway, is being reset? > >> } >> >> static void spapr_create_nvram(sPAPRMachineState *spapr) >> @@ -2598,6 +2603,9 @@ static void ppc_spapr_init(MachineState *machine) >> >> kvmppc_spapr_enable_inkernel_multitce(); >> } >> + >> + spapr->mc_status = -1; >> + qemu_cond_init(&spapr->mc_delivery_cond); >> } >> >> static int spapr_kvm_type(const char *vm_type) >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index cdf0b60..08e9a5e 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -348,6 +348,37 @@ static void rtas_get_power_level(PowerPCCPU *cpu, >> sPAPRMachineState *spapr, >> rtas_st(rets, 1, 100); >> } >> >> +static void rtas_ibm_nmi_register(PowerPCCPU *cpu, >> + sPAPRMachineState *spapr, >> + uint32_t token, uint32_t nargs, >> + target_ulong args, >> + uint32_t nret, target_ulong rets) >> +{ >> + spapr->guest_machine_check_addr = rtas_ld(args, 1); >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> +} >> + >> +static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, >> + sPAPRMachineState *spapr, >> + uint32_t token, uint32_t nargs, >> + target_ulong args, >> + uint32_t nret, target_ulong rets) >> +{ >> + if (!spapr->guest_machine_check_addr) { >> + /* NMI register not called */ >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >> + } else { >> + /* >> + * VCPU issuing "ibm,nmi-interlock" is done with NMI handling, >> + * hence unset mc_status. >> + */ >> + spapr->mc_status = -1; >> + qemu_cond_signal(&spapr->mc_delivery_cond); >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + } >> +} >> + >> + >> static struct rtas_call { >> const char *name; >> spapr_rtas_fn fn; >> @@ -489,6 +520,10 @@ static void core_rtas_register_types(void) >> rtas_set_power_level); >> spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level", >> rtas_get_power_level); >> + spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register", >> + rtas_ibm_nmi_register); >> + spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock", >> + rtas_ibm_nmi_interlock); >> } >> >> type_init(core_rtas_register_types) >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index b395aa7..28b6e2e 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -124,6 +124,13 @@ 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; > > You need to migrate this. This is separate and simpler from the > problems of migrating during machine check handling - at the moment if > the guest calls nmi-register, then you migrate, then a machine check > is triggered you will have lost the information on how to handle it > properly. I missed it. Will include. Regards, Aravinda > > >> + /* mc_status is set to -1 if mc is not in progress, else is set to the >> CPU >> + * handling the mc. */ >> + int mc_status; >> + QemuCond mc_delivery_cond; >> + >> /*< public >*/ >> char *kvm_type; >> MemoryHotplugState hotplug_memory; >> @@ -520,8 +527,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, >> target_ulong opcode, >> #define RTAS_IBM_CREATE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x27) >> #define RTAS_IBM_REMOVE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x28) >> #define RTAS_IBM_RESET_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x29) >> +#define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2A) >> +#define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2B) >> >> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2A) >> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2C) >> >> /* RTAS ibm,get-system-parameter token values */ >> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 >> > -- Regards, Aravinda