On 14/11/2016 15:00, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 14/11/2016 13:36, Marcelo Tosatti wrote:
>>> + /* local (running VM) restore */
>>> + if (s->clock_valid) {
>>> + /*
>>> + * if host does not support reliable KVM_GET_CLOCK,
>>> + * read kvmclock value from memory
>>> + */
>>> + if (!kvm_has_adjust_clock_stable()) {
>>> + time_at_migration = kvmclock_current_nsec(s);
>>
>> Just assign to s->clock here...
>
> If kvmclock is not enabled, you want to use s->clock,
> rather than 0.
>
>>> + }
>>> + /* migration/savevm/init restore */
>>> + } else {
>>> + /*
>>> + * use s->clock in case machine uses reliable
>>> + * get clock and host where vm was executing
>>> + * supported reliable get clock
>>> + */
>>> + if (!s->mach_use_reliable_get_clock ||
>>> + !s->src_use_reliable_get_clock) {
>>> + time_at_migration = kvmclock_current_nsec(s);
>>
>> ... and here, so that time_at_migration is not needed anymore.
>
> Same as above.
You're right.
>> Also here it's enough to look at s->src_user_reliable_get_clock, because
>> if s->mach_use_reliable_get_clock is false,
>> s->src_use_reliable_get_clock will be false as well.
>
> Yes, but i like the code annotation.
Ah, I think we're looking at it differently.
I'm thinking "mach_use_reliable_get_clock is just for migration,
src_use_reliable_get_clock is the state". Perhaps you're thinking of
enabling/disabling the whole new code for old machines? What is the
advantage?
>>> + }
>>> + }
>>>
>>> - /* We can't rely on the migrated clock value, just discard it */
>>> + /* We can't rely on the saved clock value, just discard it */
>>> if (time_at_migration) {
>>> s->clock = time_at_migration;
>>
>> [...]
>>
>>>
>>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
>>> +{
>>> + KVMClockState *s = opaque;
>>> +
>>> + /*
>>> + * On machine types that support reliable KVM_GET_CLOCK,
>>> + * if host kernel does provide reliable KVM_GET_CLOCK,
>>> + * set src_use_reliable_get_clock=true so that destination
>>> + * avoids reading kvmclock from memory.
>>> + */
>>> + if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
>>> + s->src_use_reliable_get_clock = true;
>>> + }
>>> +
>>> + return s->src_use_reliable_get_clock;
>>> +}
>>
>> Here you can just return s->mach_use_reliable_get_clock.
>
> mach_use_reliable_get_clock can be true but host might not support it.
Yes, but the "needed" function is only required to avoid breaking
pc-i440fx-2.7 and earlier. If you return true here, you can still
migrate a "false" value for src_use_reliable_get_clock.
>> To set
>> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
>> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.
>
> KVM_CLOCK_TSC_STABLE bit in the kvmclock structure !=
> KVM_GET_CLOCK returns reliable value, right?
It is the same as "is using masterclock", which is actually a stricter
condition than the KVM_CHECK_EXTENSION return value. The right check to
use is whether masterclock is in use, and then the idea is to treat
clock,src_use_reliable_get_clock as one tuple that is updated atomically.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
