| Message ID | CAEkmjvXGccGs2QhaiLFupTDWcnp4O+qez-uj8QdbRG13UfCy2Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] hvf: arm: Properly sync guest time on migration | expand |
We've gotten the Android Emulator snapshots working again on M1 and noticed a time warp issue where the stopwatch app would, on a snapshot load, resume including the time the emulator wasn't running. This seems to fix it. Now we have snapshots mostly working (though file backed ram is a bit busted, still working on that) On Wed, Dec 2, 2020 at 2:19 PM Frank Yang <lfy@google.com> wrote: > > From downstream: > https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 > > Based on v3 of Alexander Graf's patches > > https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de > > We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we > can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the > require effort to do that accurately---with individual values, even if > they are a tiny bit off it can result in a lockup due to inconsistent > time differences between vCPUs. So just use a global approximate value > for now. > > Not tested in upstream yet, but Android emulator snapshots work without > time warp now. > > Signed-off-by: Lingfeng Yang <lfy@google.com> > --- > accel/hvf/hvf-cpus.c | 3 +++ > include/sysemu/hvf_int.h | 4 ++++ > target/arm/hvf/hvf.c | 43 +++++++++++++++++++++++++++++++++++++++- > target/i386/hvf/hvf.c | 4 ++++ > 4 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/accel/hvf/hvf-cpus.c b/accel/hvf/hvf-cpus.c > index a981ccde70..484c7717f5 100644 > --- a/accel/hvf/hvf-cpus.c > +++ b/accel/hvf/hvf-cpus.c > @@ -456,6 +456,9 @@ static int hvf_accel_init(MachineState *ms) > hvf_state = s; > memory_listener_register(&hvf_memory_listener, &address_space_memory); > cpus_register_accel(&hvf_cpus); > + > + hvf_arch_init(s); > + > return 0; > } > > diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h > index 13adf6ea77..08830782c9 100644 > --- a/include/sysemu/hvf_int.h > +++ b/include/sysemu/hvf_int.h > @@ -55,6 +55,9 @@ struct HVFState { > hvf_slot slots[32]; > int num_slots; > > +#if defined(__aarch64__) > + uint64_t ticks; > +#endif > hvf_vcpu_caps *hvf_caps; > }; > extern HVFState *hvf_state; > @@ -73,5 +76,6 @@ void hvf_arch_vcpu_destroy(CPUState *cpu); > int hvf_vcpu_exec(CPUState *cpu); > hvf_slot *hvf_find_overlap_slot(uint64_t, uint64_t); > void hvf_kick_vcpu_thread(CPUState *cpu); > +void hvf_arch_init(HVFState* s); > > #endif > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c > index 9442e2f232..37380c6c53 100644 > --- a/target/arm/hvf/hvf.c > +++ b/target/arm/hvf/hvf.c > @@ -312,6 +312,10 @@ int hvf_put_registers(CPUState *cpu) > uint64_t val; > int i; > > + /* Sync up CNTVOFF_EL2 */ > + env->cp15.cntvoff_el2 = hvf_state->ticks; > + hv_vcpu_set_vtimer_offset(cpu->hvf->fd, env->cp15.cntvoff_el2); > + > for (i = 0; i < ARRAY_SIZE(hvf_reg_match); i++) { > val = *(uint64_t *)((void *)env + hvf_reg_match[i].offset); > ret = hv_vcpu_set_reg(cpu->hvf->fd, hvf_reg_match[i].reg, val); > @@ -418,6 +422,8 @@ void hvf_arch_vcpu_destroy(CPUState *cpu) > { > } > > +static HVFState* hvf_state = 0; > + > int hvf_arch_init_vcpu(CPUState *cpu) > { > ARMCPU *arm_cpu = ARM_CPU(cpu); > @@ -795,7 +801,11 @@ int hvf_vcpu_exec(CPUState *cpu) > &cval); > assert_hvf_ok(r); > > - int64_t ticks_to_sleep = cval - mach_absolute_time(); > + /* mach_absolute_time() is an absolute host tick number. > We > + * have set up the guest to use the host tick number > offset > + * by env->cp15.cntvoff_el2. > + */ > + int64_t ticks_to_sleep = cval - (mach_absolute_time() - > env->cp15.cntvoff_el2); > if (ticks_to_sleep < 0) { > break; > } > @@ -855,3 +865,34 @@ int hvf_vcpu_exec(CPUState *cpu) > } > } > } > + > +static int hvf_mig_state_pre_save(void* opaque) { > + struct HVFState* s = opaque; > + s->ticks -= mach_absolute_time(); > + return 0; > +} > + > +static int hvf_mig_state_post_load(void* opaque) { > + struct HVFState* s = opaque; > + m->ticks += mach_absolute_time(); > + return 0; > +} > + > + > +const VMStateDescription vmstate_hvf_migration = { > + .name = "hvf-migration", > + .version_id = 1, > + .minimum_version_id = 1, > + .pre_save = hvf_mig_state_pre_save, > + .post_load = hvf_mig_state_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(ticks_to_save, HVFState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +void hvf_arch_init(HVFState* s) { > + hvf_state = s; > + hvf_state->ticks = 0; > + vmstate_register(NULL, 0, &vmstate_hvf_migration, hvf_state); > +} > diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c > index 08b4adecd9..7ca6387620 100644 > --- a/target/i386/hvf/hvf.c > +++ b/target/i386/hvf/hvf.c > @@ -557,3 +557,7 @@ int hvf_vcpu_exec(CPUState *cpu) > > return ret; > } > + > +void hvf_arch_init(HVFState* s) { > + (void)s; > +} > -- > 2.24.3 (Apple Git-128) >
On 02.12.20 23:19, Frank Yang wrote: > > From downstream: > https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 > <https://android-review.googlesource.com/c/platform/external/qemu/+/1515002> > > Based on v3 of Alexander Graf's patches > > https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de > <https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de> > > We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we > can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the > require effort to do that accurately---with individual values, even if > they are a tiny bit off it can result in a lockup due to inconsistent > time differences between vCPUs. So just use a global approximate value > for now. > > Not tested in upstream yet, but Android emulator snapshots work without > time warp now. > > Signed-off-by: Lingfeng Yang <lfy@google.com <mailto:lfy@google.com>> If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we should be able to just recover the offset after migration by looking at QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? That would end up much easier than this patch I hope. Alex
On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de> wrote: > > On 02.12.20 23:19, Frank Yang wrote: > > > From downstream: > https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 > > Based on v3 of Alexander Graf's patches > > https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de > > We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we > can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the > require effort to do that accurately---with individual values, even if > they are a tiny bit off it can result in a lockup due to inconsistent > time differences between vCPUs. So just use a global approximate value > for now. > > Not tested in upstream yet, but Android emulator snapshots work without > time warp now. > > Signed-off-by: Lingfeng Yang <lfy@google.com> > > > If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we > should be able to just recover the offset after migration by looking at > QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? > > That would end up much easier than this patch I hope. > > > The virtual clock interfaces/implementations in QEMU seem complex to me relative to the fix needed here and they don't seem to compute ticks with mach_absolute_time() (which in this case we want since we want to compute in timer ticks instead of having to mess with ns / cycle conversions). I do agree this patch does seem more complicated on the surface though versus "just" setting cntvoff directly to some value. Maybe we should simplify the QEMU_CLOCK_VIRTUAL implementation first to maintain CNTVOFF_EL2/CNTV using mach_absolute_time() first? > Alex > > >
On 02.12.20 23:46, Frank Yang wrote: > > > On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de > <mailto:agraf@csgraf.de>> wrote: > > > On 02.12.20 23:19, Frank Yang wrote: >> >> From downstream: >> https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >> <https://android-review.googlesource.com/c/platform/external/qemu/+/1515002> >> >> Based on v3 of Alexander Graf's patches >> >> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >> <https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de> >> >> We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even >> though we >> can set separate CNTVOFF_EL2 values per vCPU, it just is not >> worth the >> require effort to do that accurately---with individual values, >> even if >> they are a tiny bit off it can result in a lockup due to inconsistent >> time differences between vCPUs. So just use a global approximate >> value >> for now. >> >> Not tested in upstream yet, but Android emulator snapshots work >> without >> time warp now. >> >> Signed-off-by: Lingfeng Yang <lfy@google.com <mailto:lfy@google.com>> > > > If we just always make CNTV start at the same 0 as > QEMU_CLOCK_VIRTUAL, we should be able to just recover the offset > after migration by looking at QEMU_CLOCK_VIRTUAL to set CNTVOFF, > right? > > That would end up much easier than this patch I hope. > > > > The virtual clock interfaces/implementations in QEMU seem complex to > me relative to the fix needed here and they don't seem to compute > ticks with mach_absolute_time() (which in this case we want since we > want to compute in timer ticks instead of having to mess with ns / > cycle conversions). I do agree this patch does seem more complicated > on the surface though versus "just" setting cntvoff directly to some > value. Maybe we should simplify the QEMU_CLOCK_VIRTUAL implementation > first to maintain CNTVOFF_EL2/CNTV using mach_absolute_time() first? So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an offset to gettimeofday(). This offset is already part of the live migration stream[1]. So if you just configure CNTVOFF_EL2 based on QEMU_CLOCK_VIRTUAL adjusted by the clock frequency on vcpu init, you should have everything you need. You can do that on every CPU init even, as the virtual clock will just be 0 on start. The only thing we need to change then is to move the WFI from a direct call to mach_absolute_time() to also check the virtual clock instead. I would hope that gettimeofday() calls mach_absolute_time() in the background too to speed it up. Alex [1] https://git.qemu.org/?p=qemu.git;a=blob;f=softmmu/cpu-timers.c;h=1eb7c675c18bda7773d4a9c549f0157c6e978a83;hb=HEAD#l229
On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <agraf@csgraf.de> wrote: > > On 02.12.20 23:46, Frank Yang wrote: > > > > On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de> wrote: > >> >> On 02.12.20 23:19, Frank Yang wrote: >> >> >> From downstream: >> https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >> >> Based on v3 of Alexander Graf's patches >> >> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >> >> We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we >> can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the >> require effort to do that accurately---with individual values, even if >> they are a tiny bit off it can result in a lockup due to inconsistent >> time differences between vCPUs. So just use a global approximate value >> for now. >> >> Not tested in upstream yet, but Android emulator snapshots work without >> time warp now. >> >> Signed-off-by: Lingfeng Yang <lfy@google.com> >> >> >> If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we >> should be able to just recover the offset after migration by looking at >> QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? >> >> That would end up much easier than this patch I hope. >> >> >> > The virtual clock interfaces/implementations in QEMU seem complex to me > relative to the fix needed here and they don't seem to compute ticks with > mach_absolute_time() (which in this case we want since we want to compute > in timer ticks instead of having to mess with ns / cycle conversions). I do > agree this patch does seem more complicated on the surface though versus > "just" setting cntvoff directly to some value. Maybe we should simplify the > QEMU_CLOCK_VIRTUAL implementation first to maintain CNTVOFF_EL2/CNTV using > mach_absolute_time() first? > > > So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an offset to > gettimeofday(). This offset is already part of the live migration > stream[1]. So if you just configure CNTVOFF_EL2 based on QEMU_CLOCK_VIRTUAL > adjusted by the clock frequency on vcpu init, you should have everything > you need. You can do that on every CPU init even, as the virtual clock will > just be 0 on start. > > The only thing we need to change then is to move the WFI from a direct > call to mach_absolute_time() to also check the virtual clock instead. I > would hope that gettimeofday() calls mach_absolute_time() in the background > too to speed it up. > > Sounds plausible, but I noticed that we also have cpu_ticks_offset as part of the migration stream, and I prefer mach_absolute_time() (ticks) instead of seconds in WFI as well as it makes the calculation more accurate (ticks against ticks instead of conversion between ns and ticks). Should we look at integrating this with cpu_ticks_offset instead? > > Alex > > > [1] > https://git.qemu.org/?p=qemu.git;a=blob;f=softmmu/cpu-timers.c;h=1eb7c675c18bda7773d4a9c549f0157c6e978a83;hb=HEAD#l229 >
On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <agraf@csgraf.de> wrote: > > > On 02.12.20 23:46, Frank Yang wrote: > > > > On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de> wrote: >> >> >> On 02.12.20 23:19, Frank Yang wrote: >> >> >> From downstream: https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >> >> Based on v3 of Alexander Graf's patches >> >> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >> >> We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we >> can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the >> require effort to do that accurately---with individual values, even if >> they are a tiny bit off it can result in a lockup due to inconsistent >> time differences between vCPUs. So just use a global approximate value >> for now. >> >> Not tested in upstream yet, but Android emulator snapshots work without >> time warp now. >> >> Signed-off-by: Lingfeng Yang <lfy@google.com> >> >> >> If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we should be able to just recover the offset after migration by looking at QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? >> >> That would end up much easier than this patch I hope. >> >> > > The virtual clock interfaces/implementations in QEMU seem complex to me relative to the fix needed here and they don't seem to compute ticks with mach_absolute_time() (which in this case we want since we want to compute in timer ticks instead of having to mess with ns / cycle conversions). I do agree this patch does seem more complicated on the surface though versus "just" setting cntvoff directly to some value. Maybe we should simplify the QEMU_CLOCK_VIRTUAL implementation first to maintain CNTVOFF_EL2/CNTV using mach_absolute_time() first? > > > So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an offset to gettimeofday(). This offset is already part of the live migration stream[1]. So if you just configure CNTVOFF_EL2 based on QEMU_CLOCK_VIRTUAL adjusted by the clock frequency on vcpu init, you should have everything you need. You can do that on every CPU init even, as the virtual clock will just be 0 on start. > > The only thing we need to change then is to move the WFI from a direct call to mach_absolute_time() to also check the virtual clock instead. I would hope that gettimeofday() calls mach_absolute_time() in the background too to speed it up. I'm not sure that something based on gettimeofday() (or clock_gettime(CLOCK_MONOTONIC) which it looks like cpu_get_clock() can also call) will work. It will include time spent asleep so it won't correspond to mach_absolute_time() aka guest CNTVCT_EL0. Peter
On Wed, Dec 2, 2020 at 3:26 PM Frank Yang <lfy@google.com> wrote: > > > > On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <agraf@csgraf.de> wrote: >> >> >> On 02.12.20 23:46, Frank Yang wrote: >> >> >> >> On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de> wrote: >>> >>> >>> On 02.12.20 23:19, Frank Yang wrote: >>> >>> >>> From downstream: https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >>> >>> Based on v3 of Alexander Graf's patches >>> >>> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >>> >>> We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we >>> can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the >>> require effort to do that accurately---with individual values, even if >>> they are a tiny bit off it can result in a lockup due to inconsistent >>> time differences between vCPUs. So just use a global approximate value >>> for now. >>> >>> Not tested in upstream yet, but Android emulator snapshots work without >>> time warp now. >>> >>> Signed-off-by: Lingfeng Yang <lfy@google.com> >>> >>> >>> If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we should be able to just recover the offset after migration by looking at QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? >>> >>> That would end up much easier than this patch I hope. >>> >>> >> >> The virtual clock interfaces/implementations in QEMU seem complex to me relative to the fix needed here and they don't seem to compute ticks with mach_absolute_time() (which in this case we want since we want to compute in timer ticks instead of having to mess with ns / cycle conversions). I do agree this patch does seem more complicated on the surface though versus "just" setting cntvoff directly to some value. Maybe we should simplify the QEMU_CLOCK_VIRTUAL implementation first to maintain CNTVOFF_EL2/CNTV using mach_absolute_time() first? >> >> >> So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an offset to gettimeofday(). This offset is already part of the live migration stream[1]. So if you just configure CNTVOFF_EL2 based on QEMU_CLOCK_VIRTUAL adjusted by the clock frequency on vcpu init, you should have everything you need. You can do that on every CPU init even, as the virtual clock will just be 0 on start. >> >> The only thing we need to change then is to move the WFI from a direct call to mach_absolute_time() to also check the virtual clock instead. I would hope that gettimeofday() calls mach_absolute_time() in the background too to speed it up. >> > Sounds plausible, but I noticed that we also have cpu_ticks_offset as part of the migration stream, and I prefer mach_absolute_time() (ticks) instead of seconds in WFI as well as it makes the calculation more accurate (ticks against ticks instead of conversion between ns and ticks). > > Should we look at integrating this with cpu_ticks_offset instead? Seems plausible to me. As far as I can tell the intent is that cpu_get_host_ticks() does not increment while asleep (e.g. on x86 it uses RDTSC which as far as I know does not increment while asleep), so we could provide an implementation on Mac that calls mach_absolute_time(). Peter
On 03.12.20 00:25, Frank Yang wrote: > > > On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <agraf@csgraf.de > <mailto:agraf@csgraf.de>> wrote: > > > On 02.12.20 23:46, Frank Yang wrote: >> >> >> On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de >> <mailto:agraf@csgraf.de>> wrote: >> >> >> On 02.12.20 23:19, Frank Yang wrote: >>> >>> From downstream: >>> https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >>> <https://android-review.googlesource.com/c/platform/external/qemu/+/1515002> >>> >>> Based on v3 of Alexander Graf's patches >>> >>> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >>> <https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de> >>> >>> We need to adjust CNTVOFF_EL2 so that time doesnt warp. >>> Even though we >>> can set separate CNTVOFF_EL2 values per vCPU, it just is not >>> worth the >>> require effort to do that accurately---with individual >>> values, even if >>> they are a tiny bit off it can result in a lockup due to >>> inconsistent >>> time differences between vCPUs. So just use a global >>> approximate value >>> for now. >>> >>> Not tested in upstream yet, but Android emulator snapshots >>> work without >>> time warp now. >>> >>> Signed-off-by: Lingfeng Yang <lfy@google.com >>> <mailto:lfy@google.com>> >> >> >> If we just always make CNTV start at the same 0 as >> QEMU_CLOCK_VIRTUAL, we should be able to just recover the >> offset after migration by looking at QEMU_CLOCK_VIRTUAL to >> set CNTVOFF, right? >> >> That would end up much easier than this patch I hope. >> >> >> >> The virtual clock interfaces/implementations in QEMU seem complex >> to me relative to the fix needed here and they don't seem to >> compute ticks with mach_absolute_time() (which in this case we >> want since we want to compute in timer ticks instead of having to >> mess with ns / cycle conversions). I do agree this patch does >> seem more complicated on the surface though versus "just" setting >> cntvoff directly to some value. Maybe we should simplify the >> QEMU_CLOCK_VIRTUAL implementation first to maintain >> CNTVOFF_EL2/CNTV using mach_absolute_time() first? > > > So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an > offset to gettimeofday(). This offset is already part of the live > migration stream[1]. So if you just configure CNTVOFF_EL2 based on > QEMU_CLOCK_VIRTUAL adjusted by the clock frequency on vcpu init, > you should have everything you need. You can do that on every CPU > init even, as the virtual clock will just be 0 on start. > > The only thing we need to change then is to move the WFI from a > direct call to mach_absolute_time() to also check the virtual > clock instead. I would hope that gettimeofday() calls > mach_absolute_time() in the background too to speed it up. > > Sounds plausible, but I noticed that we also have cpu_ticks_offset as > part of the migration stream, and I prefer mach_absolute_time() > (ticks) instead of seconds in WFI as well as it makes the calculation > more accurate (ticks against ticks instead of conversion between ns > and ticks). > > Should we look at integrating this with cpu_ticks_offset instead? The big benefit of reusing the virtual clock is that you create compatible migration streams between TCG and HVF, no? You'd lose out on that with a hack like this. Alex
On 03.12.20 00:28, Peter Collingbourne wrote: > On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <agraf@csgraf.de> wrote: >> >> On 02.12.20 23:46, Frank Yang wrote: >> >> >> >> On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <agraf@csgraf.de> wrote: >>> >>> On 02.12.20 23:19, Frank Yang wrote: >>> >>> >>> From downstream: https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 >>> >>> Based on v3 of Alexander Graf's patches >>> >>> https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de >>> >>> We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we >>> can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the >>> require effort to do that accurately---with individual values, even if >>> they are a tiny bit off it can result in a lockup due to inconsistent >>> time differences between vCPUs. So just use a global approximate value >>> for now. >>> >>> Not tested in upstream yet, but Android emulator snapshots work without >>> time warp now. >>> >>> Signed-off-by: Lingfeng Yang <lfy@google.com> >>> >>> >>> If we just always make CNTV start at the same 0 as QEMU_CLOCK_VIRTUAL, we should be able to just recover the offset after migration by looking at QEMU_CLOCK_VIRTUAL to set CNTVOFF, right? >>> >>> That would end up much easier than this patch I hope. >>> >>> >> The virtual clock interfaces/implementations in QEMU seem complex to me relative to the fix needed here and they don't seem to compute ticks with mach_absolute_time() (which in this case we want since we want to compute in timer ticks instead of having to mess with ns / cycle conversions). I do agree this patch does seem more complicated on the surface though versus "just" setting cntvoff directly to some value. Maybe we should simplify the QEMU_CLOCK_VIRTUAL implementation first to maintain CNTVOFF_EL2/CNTV using mach_absolute_time() first? >> >> >> So QEMU_CLOCK_VIRTUAL calls cpu_get_clock() which just adds an offset to gettimeofday(). This offset is already part of the live migration stream[1]. So if you just configure CNTVOFF_EL2 based on QEMU_CLOCK_VIRTUAL adjusted by the clock frequency on vcpu init, you should have everything you need. You can do that on every CPU init even, as the virtual clock will just be 0 on start. >> >> The only thing we need to change then is to move the WFI from a direct call to mach_absolute_time() to also check the virtual clock instead. I would hope that gettimeofday() calls mach_absolute_time() in the background too to speed it up. > I'm not sure that something based on gettimeofday() (or > clock_gettime(CLOCK_MONOTONIC) which it looks like cpu_get_clock() can > also call) will work. It will include time spent asleep so it won't > correspond to mach_absolute_time() aka guest CNTVCT_EL0. It will correspond to it on launch, because we'd set the offset. Afterwards, the only problem I think you're unraveling here is that we would need to adjust CNTVOFF after a suspend/resume cycle to propagate the time jump into the virtual machine. Which probably is not a terrible idea anyway, because how else would the guest know that time has passed? Alex
diff --git a/accel/hvf/hvf-cpus.c b/accel/hvf/hvf-cpus.c index a981ccde70..484c7717f5 100644 --- a/accel/hvf/hvf-cpus.c +++ b/accel/hvf/hvf-cpus.c @@ -456,6 +456,9 @@ static int hvf_accel_init(MachineState *ms) hvf_state = s; memory_listener_register(&hvf_memory_listener, &address_space_memory); cpus_register_accel(&hvf_cpus); + + hvf_arch_init(s); + return 0; } diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h index 13adf6ea77..08830782c9 100644 --- a/include/sysemu/hvf_int.h +++ b/include/sysemu/hvf_int.h @@ -55,6 +55,9 @@ struct HVFState { hvf_slot slots[32]; int num_slots; +#if defined(__aarch64__) + uint64_t ticks; +#endif hvf_vcpu_caps *hvf_caps; }; extern HVFState *hvf_state; @@ -73,5 +76,6 @@ void hvf_arch_vcpu_destroy(CPUState *cpu); int hvf_vcpu_exec(CPUState *cpu); hvf_slot *hvf_find_overlap_slot(uint64_t, uint64_t); void hvf_kick_vcpu_thread(CPUState *cpu); +void hvf_arch_init(HVFState* s); #endif diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index 9442e2f232..37380c6c53 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -312,6 +312,10 @@ int hvf_put_registers(CPUState *cpu) uint64_t val; int i; + /* Sync up CNTVOFF_EL2 */ + env->cp15.cntvoff_el2 = hvf_state->ticks; + hv_vcpu_set_vtimer_offset(cpu->hvf->fd, env->cp15.cntvoff_el2); + for (i = 0; i < ARRAY_SIZE(hvf_reg_match); i++) { val = *(uint64_t *)((void *)env + hvf_reg_match[i].offset); ret = hv_vcpu_set_reg(cpu->hvf->fd, hvf_reg_match[i].reg, val); @@ -418,6 +422,8 @@ void hvf_arch_vcpu_destroy(CPUState *cpu) { } +static HVFState* hvf_state = 0; + int hvf_arch_init_vcpu(CPUState *cpu) { ARMCPU *arm_cpu = ARM_CPU(cpu); @@ -795,7 +801,11 @@ int hvf_vcpu_exec(CPUState *cpu) &cval); assert_hvf_ok(r); - int64_t ticks_to_sleep = cval - mach_absolute_time(); + /* mach_absolute_time() is an absolute host tick number. We + * have set up the guest to use the host tick number offset + * by env->cp15.cntvoff_el2. + */ + int64_t ticks_to_sleep = cval - (mach_absolute_time() - env->cp15.cntvoff_el2); if (ticks_to_sleep < 0) { break; } @@ -855,3 +865,34 @@ int hvf_vcpu_exec(CPUState *cpu) } } } + +static int hvf_mig_state_pre_save(void* opaque) { + struct HVFState* s = opaque; + s->ticks -= mach_absolute_time(); + return 0; +} + +static int hvf_mig_state_post_load(void* opaque) { + struct HVFState* s = opaque; + m->ticks += mach_absolute_time(); + return 0; +} + + +const VMStateDescription vmstate_hvf_migration = { + .name = "hvf-migration", + .version_id = 1, + .minimum_version_id = 1, + .pre_save = hvf_mig_state_pre_save, + .post_load = hvf_mig_state_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT64(ticks_to_save, HVFState), + VMSTATE_END_OF_LIST() + }, +}; + +void hvf_arch_init(HVFState* s) { + hvf_state = s; + hvf_state->ticks = 0; + vmstate_register(NULL, 0, &vmstate_hvf_migration, hvf_state); +} diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c index 08b4adecd9..7ca6387620 100644 --- a/target/i386/hvf/hvf.c +++ b/target/i386/hvf/hvf.c @@ -557,3 +557,7 @@ int hvf_vcpu_exec(CPUState *cpu) return ret; } + +void hvf_arch_init(HVFState* s) { + (void)s; +}
From downstream: https://android-review.googlesource.com/c/platform/external/qemu/+/1515002 Based on v3 of Alexander Graf's patches https://patchew.org/QEMU/20201202190408.2041-1-agraf@csgraf.de We need to adjust CNTVOFF_EL2 so that time doesnt warp. Even though we can set separate CNTVOFF_EL2 values per vCPU, it just is not worth the require effort to do that accurately---with individual values, even if they are a tiny bit off it can result in a lockup due to inconsistent time differences between vCPUs. So just use a global approximate value for now. Not tested in upstream yet, but Android emulator snapshots work without time warp now. Signed-off-by: Lingfeng Yang <lfy@google.com> --- accel/hvf/hvf-cpus.c | 3 +++ include/sysemu/hvf_int.h | 4 ++++ target/arm/hvf/hvf.c | 43 +++++++++++++++++++++++++++++++++++++++- target/i386/hvf/hvf.c | 4 ++++ 4 files changed, 53 insertions(+), 1 deletion(-)