On Tue, 17 Mar 2020 15:02:12 +1000
Nicholas Piggin <npig...@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 <npig...@gmail.com>
> ---
LGTM
Reviewed-by: Greg Kurz <gr...@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");
> }
