Re: [PATCH 3/3] i386: Make sure kvm_arch_set_tsc_khz() succeeds on migration when 'hv-reenlightenment' was exposed

[Vitaly Kuznetsov]

Marcelo Tosatti <mtosatti@redhat.com> writes:

> On Thu, Mar 18, 2021 at 05:38:00PM +0100, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 18/03/21 17:02, Vitaly Kuznetsov wrote:
>> >> KVM doesn't fully support Hyper-V reenlightenment notifications on
>> >> migration. In particular, it doesn't support emulating TSC frequency
>> >> of the source host by trapping all TSC accesses so unless TSC scaling
>> >> is supported on the destination host and KVM_SET_TSC_KHZ succeeds, it
>> >> is unsafe to proceed with migration.
>> >> 
>> >> Normally, we only require KVM_SET_TSC_KHZ to succeed when 'user_tsc_khz'
>> >> was set and just 'try' KVM_SET_TSC_KHZ without otherwise.
>> >> 
>> >> Introduce a new vmstate section (which is added when the guest has
>> >> reenlightenment feature enabled) and add env.tsc_khz to it. We already
>> >> have env.tsc_khz packed in 'cpu/tsc_khz' but we don't want to be dependent
>> >> on the section order.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >
>> > Could we instead fail to load the reenlightenment section if 
>> > user_tsc_khz was not set?  This seems to be user (well, management) 
>> > error really, since reenlightenment has to be enabled manually (or with 
>> > hv-passthrough which blocks migration too).
>
> Seems to match the strategy of the patchset...
>
>> Yes, we certainly could do that but what's the added value of
>> user_tsc_khz which upper layer will have to set explicitly (probably to
>> the tsc frequency of the source host anyway)?
>
> Yes. I think what happened was "evolution":
>
> 1) Added support to set tsc frequency (with hardware multiplier)
> in KVM, so add -tsc-khz VAL (kHz) option to KVM.
>
> 2) Scaling is enabled only if -tsc-khz VAL is supplied.
>
> 3) libvirt switches to using -tsc-khz HVAL, where HVAL it retrieves
> from KVM_GET_TSC_KHZ of newly created KVM_CREATE_VM instance.
>
> It could have been done inside qemu instead.
>
>> In case we just want to avoid calling KVM_SET_TSC_KHZ twice, we can probably 
>> achieve that by
>> adding a CPU flag or something.
>
> Avoid calling KVM_SET_TSC_KHZ twice ? Don't see why you would avoid
> that.
>

Actually, we already do KVM_SET_TSC_KHZ twice, my patch adds just
another call for KVM_SET_TSC_KHZ. We already do one call in
kvm_arch_put_registers() but we don't propagate errors from it so in case
TSC scaling is unsupported, migration still succeeds and this is
intentional unless 'tsc-khz' was explicitly specified. When 'tsc-khz' is
specified, the error is propageted from kvm_arch_init_vcpu() (second
call site). We can also achieve the goal of this patch if we follow
Paolo's suggestion: just make 'tsc-khz' a must with reenlightenment.

-- 
Vitaly