Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

On Thu, Dec 19, 2019 at 03:30:12PM +0100, Andrew Jones wrote:
> On Mon, Dec 16, 2019 at 06:06:30PM +0000, Peter Maydell wrote:
> > Your approach in this patchset reads and writes on vm-paused,
> > so it won't have the pre-2015 problems.
> > 
> > It still feels odd that we're storing this bit of guest state
> > in two places now though -- in kvm_vtime, and also in its usual
> > place in the cpreg_array data structures (we write back the
> > value from kvm_vtime when the VM starts running, and we write
> > back the value from the cpreg_array for a PUT_FULL_STATE, which
> > the comments claim is only on startup or when we just loaded
> > migration state (and also undocumentedly but reasonably on
> > cpu-hotplug, which arm doesn't have yet).

I tried to get rid of the extra state location (kvm_vtime), but we still
need it because kvm_arch_get_registers() doesn't take 'level', like
kvm_arch_put_registers() does. Maybe it should? Without being able to
filter out TIMER_CNT at get time too, then if we migrate a paused guest
we'll resume with vtime including the ticks between the pause and the
start of the migration. Adding the additional state (kvm_vtime) and a
cpu_pre_save() hook to fixup the cpreg value is a possible way to resolve
that. That's what I've done for v3, which I'll post shortly.

> > 
> > I've just spent a little while digging through code, and
> > haven't been able to satisfy myself on the ordering of which
> > writeback wins: for a loadvm I think we first do a
> > cpu_synchronize_all_post_init() (writing back the counter
> > value from the migration data) and then after than we will
> > unpause the VM -- why doesn't this overwrite the correct
> > value with the wrong value from kvm_vtime ?

It wasn't overwriting because we weren't detecting a runstate
transition from paused to running. However, for v3, I've dropped
the explicit running/pause transition checking and now ensured
we get the right value with a cpu_post_load() hook.

> 
> > I just noticed also that the logic used in this patch
> > doesn't match what other architectures do in their vm_state_change
> > function -- eg cpu_ppc_clock_vm_state_change() has an
> > "if (running) { load } else { save }", and kvmclock_vm_state_change()
> > for i386 also has "if (running) { ... } else { ... }", though
> > it has an extra wrinkle where it captures "are we PAUSED?"
> > to use in the pre_save function; the comment above
> > kvmclock_pre_save() suggests maybe that would be useful for other
> > than x86, too. kvm_s390_tod_vm_state_change() has
> > logic that's a slightly more complicated variation on just
> > testing the 'running' flag, but it doesn't look at the
> > specific new state.

I think I've mimicked this logic now for arm in v3.

Thanks,
drew