| Message ID | 155323646571.18748.12843943976827665328.stgit@aravinda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target-ppc/spapr: Add FWNMI support in QEMU for PowerKVM guests | expand |
On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> > --- > hw/ppc/spapr_events.c | 17 +++++++++++++++++ > hw/ppc/spapr_rtas.c | 4 ++++ > include/hw/ppc/spapr.h | 3 +++ > 3 files changed, 24 insertions(+) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index d7cc0a4..6356a38 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -41,6 +41,7 @@ > #include "qemu/bcd.h" > #include "hw/ppc/spapr_ovec.h" > #include <libfdt.h> > +#include "migration/blocker.h" > > #define RTAS_LOG_VERSION_MASK 0xff000000 > #define RTAS_LOG_VERSION_6 0x06000000 > @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) > void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > { > SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + int ret; > + Error *local_err = NULL; > + > + error_setg(&spapr->migration_blocker, > + "Live migration not supported during machine check handling"); > + 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 > + * the migration or killing the VM. Can't you just discard the error in that case? > + */ > + error_free(spapr->migration_blocker); > + fprintf(stderr, "Warning: Machine check during VM migration\n"); > + } > > while (spapr->mc_status != -1) { > /* > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 939f428..b0676f1 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -50,6 +50,7 @@ > #include "target/ppc/mmu-hash64.h" > #include "target/ppc/mmu-book3s-v3.h" > #include "kvm_ppc.h" > +#include "migration/blocker.h" > > static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr, > uint32_t token, uint32_t nargs, > @@ -391,6 +392,9 @@ 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); > + spapr->migration_blocker = NULL; > } > } > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 0c9dec1..b8eced4 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -10,6 +10,7 @@ > #include "hw/ppc/spapr_irq.h" > #include "hw/ppc/spapr_xive.h" /* For SpaprXive */ > #include "hw/ppc/xics.h" /* For ICSState */ > +#include "qapi/error.h" > > struct SpaprVioBus; > struct SpaprPhbState; > @@ -212,6 +213,8 @@ struct SpaprMachineState { > SpaprCapabilities def, eff, mig; > > unsigned gpu_numa_id; > + > + Error *migration_blocker; > }; > > #define H_SUCCESS 0 >
On Monday 25 March 2019 12:03 PM, David Gibson wrote: > On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_events.c | 17 +++++++++++++++++ >> hw/ppc/spapr_rtas.c | 4 ++++ >> include/hw/ppc/spapr.h | 3 +++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c >> index d7cc0a4..6356a38 100644 >> --- a/hw/ppc/spapr_events.c >> +++ b/hw/ppc/spapr_events.c >> @@ -41,6 +41,7 @@ >> #include "qemu/bcd.h" >> #include "hw/ppc/spapr_ovec.h" >> #include <libfdt.h> >> +#include "migration/blocker.h" >> >> #define RTAS_LOG_VERSION_MASK 0xff000000 >> #define RTAS_LOG_VERSION_6 0x06000000 >> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) >> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) >> { >> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); >> + int ret; >> + Error *local_err = NULL; >> + >> + error_setg(&spapr->migration_blocker, >> + "Live migration not supported during machine check handling"); >> + 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 >> + * the migration or killing the VM. > > Can't you just discard the error in that case? I am actually discarding the error. Do you mean I don't have to print the below warning or am I missing something? Regards, Aravinda > >> + */ >> + error_free(spapr->migration_blocker); >> + fprintf(stderr, "Warning: Machine check during VM migration\n"); >> + } >> >> while (spapr->mc_status != -1) { >> /* >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index 939f428..b0676f1 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -50,6 +50,7 @@ >> #include "target/ppc/mmu-hash64.h" >> #include "target/ppc/mmu-book3s-v3.h" >> #include "kvm_ppc.h" >> +#include "migration/blocker.h" >> >> static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr, >> uint32_t token, uint32_t nargs, >> @@ -391,6 +392,9 @@ 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); >> + spapr->migration_blocker = NULL; >> } >> } >> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 0c9dec1..b8eced4 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -10,6 +10,7 @@ >> #include "hw/ppc/spapr_irq.h" >> #include "hw/ppc/spapr_xive.h" /* For SpaprXive */ >> #include "hw/ppc/xics.h" /* For ICSState */ >> +#include "qapi/error.h" >> >> struct SpaprVioBus; >> struct SpaprPhbState; >> @@ -212,6 +213,8 @@ struct SpaprMachineState { >> SpaprCapabilities def, eff, mig; >> >> unsigned gpu_numa_id; >> + >> + Error *migration_blocker; >> }; >> >> #define H_SUCCESS 0 >> >
On Mon, Mar 25, 2019 at 02:31:10PM +0530, Aravinda Prasad wrote: > > > On Monday 25 March 2019 12:03 PM, David Gibson wrote: > > On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> > >> --- > >> hw/ppc/spapr_events.c | 17 +++++++++++++++++ > >> hw/ppc/spapr_rtas.c | 4 ++++ > >> include/hw/ppc/spapr.h | 3 +++ > >> 3 files changed, 24 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > >> index d7cc0a4..6356a38 100644 > >> --- a/hw/ppc/spapr_events.c > >> +++ b/hw/ppc/spapr_events.c > >> @@ -41,6 +41,7 @@ > >> #include "qemu/bcd.h" > >> #include "hw/ppc/spapr_ovec.h" > >> #include <libfdt.h> > >> +#include "migration/blocker.h" > >> > >> #define RTAS_LOG_VERSION_MASK 0xff000000 > >> #define RTAS_LOG_VERSION_6 0x06000000 > >> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) > >> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > >> { > >> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > >> + int ret; > >> + Error *local_err = NULL; > >> + > >> + error_setg(&spapr->migration_blocker, > >> + "Live migration not supported during machine check handling"); > >> + 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 > >> + * the migration or killing the VM. > > > > Can't you just discard the error in that case? > > I am actually discarding the error. Do you mean I don't have to print > the below warning or am I missing something? Um.. I guess we should keep the warning. The tricky thing here is that the information on the machine check is no longer relevant after the migration, however presumably the event which triggered it might have already corrupted guest memory, which *would* be relevant to the guest after migration. Not sure what to do about that.
On Tuesday 26 March 2019 09:49 AM, David Gibson wrote: > On Mon, Mar 25, 2019 at 02:31:10PM +0530, Aravinda Prasad wrote: >> >> >> On Monday 25 March 2019 12:03 PM, David Gibson wrote: >>> On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> >>>> --- >>>> hw/ppc/spapr_events.c | 17 +++++++++++++++++ >>>> hw/ppc/spapr_rtas.c | 4 ++++ >>>> include/hw/ppc/spapr.h | 3 +++ >>>> 3 files changed, 24 insertions(+) >>>> >>>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c >>>> index d7cc0a4..6356a38 100644 >>>> --- a/hw/ppc/spapr_events.c >>>> +++ b/hw/ppc/spapr_events.c >>>> @@ -41,6 +41,7 @@ >>>> #include "qemu/bcd.h" >>>> #include "hw/ppc/spapr_ovec.h" >>>> #include <libfdt.h> >>>> +#include "migration/blocker.h" >>>> >>>> #define RTAS_LOG_VERSION_MASK 0xff000000 >>>> #define RTAS_LOG_VERSION_6 0x06000000 >>>> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) >>>> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) >>>> { >>>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); >>>> + int ret; >>>> + Error *local_err = NULL; >>>> + >>>> + error_setg(&spapr->migration_blocker, >>>> + "Live migration not supported during machine check handling"); >>>> + 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 >>>> + * the migration or killing the VM. >>> >>> Can't you just discard the error in that case? >> >> I am actually discarding the error. Do you mean I don't have to print >> the below warning or am I missing something? > > Um.. I guess we should keep the warning. The tricky thing here is > that the information on the machine check is no longer relevant after > the migration, however presumably the event which triggered it might > have already corrupted guest memory, which *would* be relevant to the > guest after migration. Not sure what to do about that. I think the information is still relevant after the migration, because it includes the effective address in error and the context. If the corruption is in the guest kernel context then the machine check handler inside the guest kernel will call panic(). If the corruption is in user space, then the application is sent a SIGBUS. But the problem is when the machine check handler hardware poisons the page frame backing the corrupt memory to avoid reuse of that page frame. After migration, the page frame backing the corrupt memory is clean, but we poison the clean page frame. >
On Tue, Mar 26, 2019 at 12:22:50PM +0530, Aravinda Prasad wrote: > > > On Tuesday 26 March 2019 09:49 AM, David Gibson wrote: > > On Mon, Mar 25, 2019 at 02:31:10PM +0530, Aravinda Prasad wrote: > >> > >> > >> On Monday 25 March 2019 12:03 PM, David Gibson wrote: > >>> On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> > >>>> --- > >>>> hw/ppc/spapr_events.c | 17 +++++++++++++++++ > >>>> hw/ppc/spapr_rtas.c | 4 ++++ > >>>> include/hw/ppc/spapr.h | 3 +++ > >>>> 3 files changed, 24 insertions(+) > >>>> > >>>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > >>>> index d7cc0a4..6356a38 100644 > >>>> --- a/hw/ppc/spapr_events.c > >>>> +++ b/hw/ppc/spapr_events.c > >>>> @@ -41,6 +41,7 @@ > >>>> #include "qemu/bcd.h" > >>>> #include "hw/ppc/spapr_ovec.h" > >>>> #include <libfdt.h> > >>>> +#include "migration/blocker.h" > >>>> > >>>> #define RTAS_LOG_VERSION_MASK 0xff000000 > >>>> #define RTAS_LOG_VERSION_6 0x06000000 > >>>> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) > >>>> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > >>>> { > >>>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > >>>> + int ret; > >>>> + Error *local_err = NULL; > >>>> + > >>>> + error_setg(&spapr->migration_blocker, > >>>> + "Live migration not supported during machine check handling"); > >>>> + 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 > >>>> + * the migration or killing the VM. > >>> > >>> Can't you just discard the error in that case? > >> > >> I am actually discarding the error. Do you mean I don't have to print > >> the below warning or am I missing something? > > > > Um.. I guess we should keep the warning. The tricky thing here is > > that the information on the machine check is no longer relevant after > > the migration, however presumably the event which triggered it might > > have already corrupted guest memory, which *would* be relevant to the > > guest after migration. Not sure what to do about that. > > I think the information is still relevant after the migration, because > it includes the effective address in error and the context. If the > corruption is in the guest kernel context then the machine check handler > inside the guest kernel will call panic(). If the corruption is in user > space, then the application is sent a SIGBUS. Good point. > But the problem is when the machine check handler hardware poisons the > page frame backing the corrupt memory to avoid reuse of that page frame. > After migration, the page frame backing the corrupt memory is clean, but > we poison the clean page frame. This is the guest side handler? It sounds like we we need two variants of the notification that say whether or not the area is still bad.
On Thursday 28 March 2019 06:18 AM, David Gibson wrote: > On Tue, Mar 26, 2019 at 12:22:50PM +0530, Aravinda Prasad wrote: >> >> >> On Tuesday 26 March 2019 09:49 AM, David Gibson wrote: >>> On Mon, Mar 25, 2019 at 02:31:10PM +0530, Aravinda Prasad wrote: >>>> >>>> >>>> On Monday 25 March 2019 12:03 PM, David Gibson wrote: >>>>> On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/ppc/spapr_events.c | 17 +++++++++++++++++ >>>>>> hw/ppc/spapr_rtas.c | 4 ++++ >>>>>> include/hw/ppc/spapr.h | 3 +++ >>>>>> 3 files changed, 24 insertions(+) >>>>>> >>>>>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c >>>>>> index d7cc0a4..6356a38 100644 >>>>>> --- a/hw/ppc/spapr_events.c >>>>>> +++ b/hw/ppc/spapr_events.c >>>>>> @@ -41,6 +41,7 @@ >>>>>> #include "qemu/bcd.h" >>>>>> #include "hw/ppc/spapr_ovec.h" >>>>>> #include <libfdt.h> >>>>>> +#include "migration/blocker.h" >>>>>> >>>>>> #define RTAS_LOG_VERSION_MASK 0xff000000 >>>>>> #define RTAS_LOG_VERSION_6 0x06000000 >>>>>> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) >>>>>> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) >>>>>> { >>>>>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); >>>>>> + int ret; >>>>>> + Error *local_err = NULL; >>>>>> + >>>>>> + error_setg(&spapr->migration_blocker, >>>>>> + "Live migration not supported during machine check handling"); >>>>>> + 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 >>>>>> + * the migration or killing the VM. >>>>> >>>>> Can't you just discard the error in that case? >>>> >>>> I am actually discarding the error. Do you mean I don't have to print >>>> the below warning or am I missing something? >>> >>> Um.. I guess we should keep the warning. The tricky thing here is >>> that the information on the machine check is no longer relevant after >>> the migration, however presumably the event which triggered it might >>> have already corrupted guest memory, which *would* be relevant to the >>> guest after migration. Not sure what to do about that. >> >> I think the information is still relevant after the migration, because >> it includes the effective address in error and the context. If the >> corruption is in the guest kernel context then the machine check handler >> inside the guest kernel will call panic(). If the corruption is in user >> space, then the application is sent a SIGBUS. > > Good point. > >> But the problem is when the machine check handler hardware poisons the >> page frame backing the corrupt memory to avoid reuse of that page frame. >> After migration, the page frame backing the corrupt memory is clean, but >> we poison the clean page frame. > > This is the guest side handler? It sounds like we we need two > variants of the notification that say whether or not the area is still > bad. Yes this is guest side handler. Can we just let the guest machine check handler poison a clean page as a migration during a machine check is a rare occurrence. We may at most lose a clean page which is a trade-off for additional complexity in the machine check handler. >
On Thu, Mar 28, 2019 at 12:09:05PM +0530, Aravinda Prasad wrote: > > > On Thursday 28 March 2019 06:18 AM, David Gibson wrote: > > On Tue, Mar 26, 2019 at 12:22:50PM +0530, Aravinda Prasad wrote: > >> > >> > >> On Tuesday 26 March 2019 09:49 AM, David Gibson wrote: > >>> On Mon, Mar 25, 2019 at 02:31:10PM +0530, Aravinda Prasad wrote: > >>>> > >>>> > >>>> On Monday 25 March 2019 12:03 PM, David Gibson wrote: > >>>>> On Fri, Mar 22, 2019 at 12:04:25PM +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 <aravinda@linux.vnet.ibm.com> > >>>>>> --- > >>>>>> hw/ppc/spapr_events.c | 17 +++++++++++++++++ > >>>>>> hw/ppc/spapr_rtas.c | 4 ++++ > >>>>>> include/hw/ppc/spapr.h | 3 +++ > >>>>>> 3 files changed, 24 insertions(+) > >>>>>> > >>>>>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > >>>>>> index d7cc0a4..6356a38 100644 > >>>>>> --- a/hw/ppc/spapr_events.c > >>>>>> +++ b/hw/ppc/spapr_events.c > >>>>>> @@ -41,6 +41,7 @@ > >>>>>> #include "qemu/bcd.h" > >>>>>> #include "hw/ppc/spapr_ovec.h" > >>>>>> #include <libfdt.h> > >>>>>> +#include "migration/blocker.h" > >>>>>> > >>>>>> #define RTAS_LOG_VERSION_MASK 0xff000000 > >>>>>> #define RTAS_LOG_VERSION_6 0x06000000 > >>>>>> @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) > >>>>>> void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) > >>>>>> { > >>>>>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > >>>>>> + int ret; > >>>>>> + Error *local_err = NULL; > >>>>>> + > >>>>>> + error_setg(&spapr->migration_blocker, > >>>>>> + "Live migration not supported during machine check handling"); > >>>>>> + 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 > >>>>>> + * the migration or killing the VM. > >>>>> > >>>>> Can't you just discard the error in that case? > >>>> > >>>> I am actually discarding the error. Do you mean I don't have to print > >>>> the below warning or am I missing something? > >>> > >>> Um.. I guess we should keep the warning. The tricky thing here is > >>> that the information on the machine check is no longer relevant after > >>> the migration, however presumably the event which triggered it might > >>> have already corrupted guest memory, which *would* be relevant to the > >>> guest after migration. Not sure what to do about that. > >> > >> I think the information is still relevant after the migration, because > >> it includes the effective address in error and the context. If the > >> corruption is in the guest kernel context then the machine check handler > >> inside the guest kernel will call panic(). If the corruption is in user > >> space, then the application is sent a SIGBUS. > > > > Good point. > > > >> But the problem is when the machine check handler hardware poisons the > >> page frame backing the corrupt memory to avoid reuse of that page frame. > >> After migration, the page frame backing the corrupt memory is clean, but > >> we poison the clean page frame. > > > > This is the guest side handler? It sounds like we we need two > > variants of the notification that say whether or not the area is still > > bad. > > Yes this is guest side handler. > > Can we just let the guest machine check handler poison a clean page as a > migration during a machine check is a rare occurrence. We may at most > lose a clean page which is a trade-off for additional complexity in the > machine check handler. Yeah, that seems reasonable.
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index d7cc0a4..6356a38 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -41,6 +41,7 @@ #include "qemu/bcd.h" #include "hw/ppc/spapr_ovec.h" #include <libfdt.h> +#include "migration/blocker.h" #define RTAS_LOG_VERSION_MASK 0xff000000 #define RTAS_LOG_VERSION_6 0x06000000 @@ -866,6 +867,22 @@ static void spapr_mce_build_elog(PowerPCCPU *cpu, bool recovered) void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) { SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + int ret; + Error *local_err = NULL; + + error_setg(&spapr->migration_blocker, + "Live migration not supported during machine check handling"); + 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 + * the migration or killing the VM. + */ + error_free(spapr->migration_blocker); + fprintf(stderr, "Warning: Machine check during VM migration\n"); + } while (spapr->mc_status != -1) { /* diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 939f428..b0676f1 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -50,6 +50,7 @@ #include "target/ppc/mmu-hash64.h" #include "target/ppc/mmu-book3s-v3.h" #include "kvm_ppc.h" +#include "migration/blocker.h" static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr, uint32_t token, uint32_t nargs, @@ -391,6 +392,9 @@ 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); + spapr->migration_blocker = NULL; } } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 0c9dec1..b8eced4 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -10,6 +10,7 @@ #include "hw/ppc/spapr_irq.h" #include "hw/ppc/spapr_xive.h" /* For SpaprXive */ #include "hw/ppc/xics.h" /* For ICSState */ +#include "qapi/error.h" struct SpaprVioBus; struct SpaprPhbState; @@ -212,6 +213,8 @@ struct SpaprMachineState { SpaprCapabilities def, eff, mig; unsigned gpu_numa_id; + + Error *migration_blocker; }; #define H_SUCCESS 0
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 <aravinda@linux.vnet.ibm.com> --- hw/ppc/spapr_events.c | 17 +++++++++++++++++ hw/ppc/spapr_rtas.c | 4 ++++ include/hw/ppc/spapr.h | 3 +++ 3 files changed, 24 insertions(+)