| Message ID | 20200317050215.159334-3-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | FWNMI follow up patches | expand |
On Tue, 17 Mar 2020 15:02:12 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > Some of the conditions are not as clearly documented as they could be. > Also the non-FWNMI case does not need a large comment. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- LGTM Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_events.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 323fcef4aa..05337f0671 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -834,17 +834,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > Error *local_err = NULL; > > 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 > - * called) or between system reset and "ibm,nmi-register". > - * Fall back to the old machine check behavior in such cases. > - */ > + /* Non-FWNMI case, deliver it like an architected CPU interrupt. */ > cs->exception_index = POWERPC_EXCP_MCHECK; > ppc_cpu_do_interrupt(cs); > return; > } > > + /* Wait for FWNMI interlock. */ > while (spapr->fwnmi_machine_check_interlock != -1) { > /* > * Check whether the same CPU got machine check error > @@ -856,8 +852,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > return; > } > qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); > - /* Meanwhile if the system is reset, then just return */ > if (spapr->fwnmi_machine_check_addr == -1) { > + /* > + * If the machine was reset while waiting for the interlock, > + * abort the delivery. The machine check applies to a context > + * that no longer exists, so it wouldn't make sense to deliver > + * it now. > + */ > return; > } > } > @@ -868,7 +869,9 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > * We don't want to abort so we let the migration to continue. > * In a rare case, the machine check handler will run on the target. > * Though this is not preferable, it is better than aborting > - * the migration or killing the VM. > + * the migration or killing the VM. It is okay to call > + * migrate_del_blocker on a blocker that was not added (which the > + * nmi-interlock handler would do when it's called after this). > */ > warn_report("Received a fwnmi while migration was in progress"); > }
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 323fcef4aa..05337f0671 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -834,17 +834,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) Error *local_err = NULL; 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 - * called) or between system reset and "ibm,nmi-register". - * Fall back to the old machine check behavior in such cases. - */ + /* Non-FWNMI case, deliver it like an architected CPU interrupt. */ cs->exception_index = POWERPC_EXCP_MCHECK; ppc_cpu_do_interrupt(cs); return; } + /* Wait for FWNMI interlock. */ while (spapr->fwnmi_machine_check_interlock != -1) { /* * Check whether the same CPU got machine check error @@ -856,8 +852,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) return; } qemu_cond_wait_iothread(&spapr->fwnmi_machine_check_interlock_cond); - /* Meanwhile if the system is reset, then just return */ if (spapr->fwnmi_machine_check_addr == -1) { + /* + * If the machine was reset while waiting for the interlock, + * abort the delivery. The machine check applies to a context + * that no longer exists, so it wouldn't make sense to deliver + * it now. + */ return; } } @@ -868,7 +869,9 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) * We don't want to abort so we let the migration to continue. * In a rare case, the machine check handler will run on the target. * Though this is not preferable, it is better than aborting - * the migration or killing the VM. + * the migration or killing the VM. It is okay to call + * migrate_del_blocker on a blocker that was not added (which the + * nmi-interlock handler would do when it's called after this). */ warn_report("Received a fwnmi while migration was in progress"); }
Some of the conditions are not as clearly documented as they could be. Also the non-FWNMI case does not need a large comment. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_events.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)