On Wed, Dec 2, 2020 at 3:26 PM Frank Yang <l...@google.com> wrote: > > > > On Wed, Dec 2, 2020 at 2:57 PM Alexander Graf <ag...@csgraf.de> wrote: >> >> >> On 02.12.20 23:46, Frank Yang wrote: >> >> >> >> On Wed, Dec 2, 2020 at 2:28 PM Alexander Graf <ag...@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-ag...@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 <l...@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
