[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 6/6] migration: Block migration while handlin
From: |
Aravinda Prasad |
Subject: |
Re: [Qemu-devel] [PATCH v5 6/6] migration: Block migration while handling machine check |
Date: |
Sun, 8 Oct 2017 13:37:19 +0530 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On Wednesday 04 October 2017 07:09 AM, David Gibson wrote:
> On Thu, Sep 28, 2017 at 04:08:31PM +0530, Aravinda Prasad wrote:
>> Block VM migration requests until the machine check
>> error handling is complete as (i) these errors are
>> specific to the source hardware and is irrelevant on
>> the target hardware, (ii) these errors cause data
>> corruption and should be handled before migration.
>>
>> Signed-off-by: Aravinda Prasad <address@hidden>
>> ---
>> hw/ppc/spapr_rtas.c | 3 +++
>> include/hw/ppc/spapr.h | 2 ++
>> target/ppc/kvm.c | 17 +++++++++++++++++
>> 3 files changed, 22 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index d017a67..17f6567 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -47,6 +47,7 @@
>> #include "trace.h"
>> #include "hw/ppc/fdt.h"
>> #include "kvm_ppc.h"
>> +#include "migration/blocker.h"
>>
>> static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState
>> *spapr,
>> uint32_t token, uint32_t nargs,
>> @@ -390,6 +391,8 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
>> spapr->mc_status = -1;
>> qemu_cond_signal(&spapr->mc_delivery_cond);
>> rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> + migrate_del_blocker(spapr->migration_blocker);
>> + error_free(spapr->migration_blocker);
>> }
>> }
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index a75e9cf..0890a44 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -7,6 +7,7 @@
>> #include "hw/ppc/spapr_drc.h"
>> #include "hw/mem/pc-dimm.h"
>> #include "hw/ppc/spapr_ovec.h"
>> +#include "qapi/error.h"
>>
>> struct VIOsPAPRBus;
>> struct sPAPRPHBState;
>> @@ -136,6 +137,7 @@ struct sPAPRMachineState {
>> MemoryHotplugState hotplug_memory;
>>
>> const char *icp_type;
>> + Error *migration_blocker;
>
> This isn't a good name, because it's _specifically_ the fwnmi as a
> migration blocker - trying to put another migration blocker in here
> would break horribly, because nmi-interlock would clear it regardless.
Will rename it.
>
>> };
>>
>> #define H_SUCCESS 0
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 59b3322..58de7ea 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -52,6 +52,7 @@
>> #endif
>> #include "elf.h"
>> #include "sysemu/kvm_int.h"
>> +#include "migration/blocker.h"
>>
>> //#define DEBUG_KVM
>>
>> @@ -2770,10 +2771,26 @@ int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run
>> *run)
>> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> target_ulong msr = 0;
>> + Error *local_err = NULL;
>> + int ret;
>> bool type, le;
>>
>> cpu_synchronize_state(CPU(cpu));
>>
>> + error_setg(&spapr->migration_blocker,
>> + "Live migration not supported during machine check error
>> handling");
>
> In fact, there's no real reason to generate the error here. The
> error's always the same so you could just create it at startup as a
> global and just add/remove it to the block list.
ok.
>
>> + ret = migrate_add_blocker(spapr->migration_blocker, &local_err);
>> + if (ret < 0) {
>> + /*
>> + * We don't want to abort and let the migration to continue. In a
>> + * rare case, the machine check handler will run on the target
>> + * hardware. Though this is not preferable, it is better than
>> aborting
>
> Why is it not preferable? I mean it's an edge case, but AFAICT it's
> still the correct behaviour.
Will remove that line in the comment.
>
>> + * the migration or killing the VM.
>> + */
>> + error_free(spapr->migration_blocker);
>> + fprintf(stderr, "Warning: Machine check during VM migration\n");
>
> Use error_report(), not fprintf().
ok.
Regards,
Aravinda
>
>> + }
>> +
>> /*
>> * Properly set bits in MSR before we invoke the handler.
>> * SRR0/1, DAR and DSISR are properly set by KVM
>>
>
--
Regards,
Aravinda