Re: [PATCH v1 1/1] hvf: arm: Properly sync guest time on migration

[Peter Collingbourne]

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
>>>
>>> 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>
>>>
>>>
>>> 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