[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCHv4 2/5] migration: Mark CPU states dirty before inc
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm |
Date: |
Tue, 30 May 2017 16:15:23 +1000 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, May 29, 2017 at 10:46:05PM +0200, Greg Kurz wrote:
> On Fri, 26 May 2017 15:23:16 +1000
> David Gibson <address@hidden> wrote:
>
> > As a rule, CPU internal state should never be updated when
> > !cpu->kvm_vcpu_dirty (or the HAX equivalent). If that is done, then
> > subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
> > will clobber state.
> >
> > However, we routinely do this during a loadvm or incoming migration.
> > Usually this is called shortly after a reset, which will clear all the cpu
> > dirty flags with cpu_synchronize_all_post_reset(). Nothing is expected
> > to set the dirty flags again before the cpu state is loaded from the
> > incoming stream.
> >
> > This means that it isn't safe to call cpu_synchronize_state() from a
> > post_load handler, which is non-obvious and potentially inconvenient.
> >
> > We could cpu_synchronize_all_state() before the loadvm, but that would be
> > overkill since a) we expect the state to already be synchronized from the
> > reset and b) we expect to completely rewrite the state with a call to
> > cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().
> >
> > To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
> > associated helpers, which simply marks the cpu state as dirty without
> > actually changing anything. i.e. it says we want to discard any existing
> > KVM (or HAX) state and replace it with what we're going to load.
> >
>
> This makes sense and looks nicer than adding a post-load specific path to
> ppc_set_compat() indeed.
>
> Just one remark below.
>
> > Cc: Juan Quintela <address@hidden>
> > Cc: Dave Gilbert <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> > cpus.c | 9 +++++++++
> > include/sysemu/cpus.h | 1 +
> > include/sysemu/hax.h | 1 +
> > include/sysemu/hw_accel.h | 10 ++++++++++
> > include/sysemu/kvm.h | 1 +
> > kvm-all.c | 10 ++++++++++
> > migration/savevm.c | 2 ++
> > target/i386/hax-all.c | 10 ++++++++++
> > 8 files changed, 44 insertions(+)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 516e5cb..6398439 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void)
> > }
> > }
> >
> > +void cpu_synchronize_all_pre_loadvm(void)
> > +{
> > + CPUState *cpu;
> > +
> > + CPU_FOREACH(cpu) {
> > + cpu_synchronize_pre_loadvm(cpu);
> > + }
> > +}
> > +
> > static int do_vm_stop(RunState state)
> > {
> > int ret = 0;
> > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> > index a8053f1..731756d 100644
> > --- a/include/sysemu/cpus.h
> > +++ b/include/sysemu/cpus.h
> > @@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType
> > type);
> > void cpu_synchronize_all_states(void);
> > void cpu_synchronize_all_post_reset(void);
> > void cpu_synchronize_all_post_init(void);
> > +void cpu_synchronize_all_pre_loadvm(void);
> >
> > void qtest_clock_warp(int64_t dest);
> >
> > diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h
> > index d9f0239..232a68a 100644
> > --- a/include/sysemu/hax.h
> > +++ b/include/sysemu/hax.h
> > @@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size);
> > void hax_cpu_synchronize_state(CPUState *cpu);
> > void hax_cpu_synchronize_post_reset(CPUState *cpu);
> > void hax_cpu_synchronize_post_init(CPUState *cpu);
> > +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu);
> >
> > #ifdef CONFIG_HAX
> >
> > diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
> > index c9b3105..469ffda 100644
> > --- a/include/sysemu/hw_accel.h
> > +++ b/include/sysemu/hw_accel.h
> > @@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState
> > *cpu)
> > }
> > }
> >
> > +static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
> > +{
> > + if (kvm_enabled()) {
> > + kvm_cpu_synchronize_pre_loadvm(cpu);
> > + }
> > + if (hax_enabled()) {
> > + hax_cpu_synchronize_pre_loadvm(cpu);
> > + }
> > +}
> > +
> > #endif /* QEMU_HW_ACCEL_H */
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 5cc83f2..a45c145 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s,
> > void *ram_addr,
> > void kvm_cpu_synchronize_state(CPUState *cpu);
> > void kvm_cpu_synchronize_post_reset(CPUState *cpu);
> > void kvm_cpu_synchronize_post_init(CPUState *cpu);
> > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
> >
> > void kvm_init_cpu_signals(CPUState *cpu);
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 90b8573..a8485bd 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu)
> > run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
> > }
> >
> > +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu,
> > run_on_cpu_data arg)
> > +{
> > + cpu->kvm_vcpu_dirty = true;
> > +}
> > +
> > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
> > +{
> > + run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
>
> Do we really need to run_on_cpu() since we only set the dirty flag ?
Um.. yeah.. I'm not sure. I left it in out of paranoia, because I
wasn't sure if there could be memory ordering issues between the qemu
threads if we just set it from the migration thread.
I'm hoping Dave or Juan will have a better idea.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- [Qemu-ppc] [PATCHv4 0/5] Clean up compatibility mode handling, David Gibson, 2017/05/26
- [Qemu-ppc] [PATCHv4 1/5] qapi: add explicit null to string input and output visitors, David Gibson, 2017/05/26
- [Qemu-ppc] [PATCHv4 5/5] ppc: Rework CPU compatibility testing across migration, David Gibson, 2017/05/26
- [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine, David Gibson, 2017/05/26
- Re: [Qemu-ppc] [PATCHv4 0/5] Clean up compatibility mode handling, Greg Kurz, 2017/05/29