Date:   Mon, 11 Dec 2017 10:56:33 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Roman Kagan <rkagan@...tuozzo.com>
Cc:     kvm@...r.kernel.org, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "Michael Kelley \(EOSG\)" <Michael.H.Kelley@...rosoft.com>,
        Andy Lutomirski <luto@...nel.org>,
        Mohammed Gamal <mmorsy@...hat.com>,
        Cathy Avery <cavery@...hat.com>, linux-kernel@...r.kernel.org,
        devel@...uxdriverproject.org
Subject: Re: [PATCH 3/6] x86/hyper-v: reenlightenment notifications support

Roman Kagan <rkagan@...tuozzo.com> writes:

> On Fri, Dec 08, 2017 at 11:49:57AM +0100, Vitaly Kuznetsov wrote:
>> Hyper-V supports Live Migration notification. This is supposed to be used
>> in conjunction with TSC emulation: when we are migrated to a host with
>> different TSC frequency for some short period host emulates our accesses
>> to TSC and sends us an interrupt to notify about the event. When we're
>> done updating everything we can disable TSC emulation and everything will
>> start working fast again.
>> 
>> We didn't need these notifications before as Hyper-V guests are not
>> supposed to use TSC as a clocksource: in Linux we even mark it as unstable
>> on boot. Guests normally use 'tsc page' clocksouce and host updates its
>> values on migrations automatically.
>> 
>> Things change when we want to run nested virtualization: even when we pass
>> through PV clocksources (kvm-clock or tsc page) to our guests we need to
>> know TSC frequency and when it changes.
>> 
>> Hyper-V Top Level Functional Specification (as of v5.0b) wrongly specifies
>> EAX:BIT(12) of CPUID:0x40000009 as the feature identification bit. The
>> right one to check is EAX:BIT(13) of CPUID:0x40000003. I was assured that
>> the fix in on the way.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
>> ---
>> RFC -> v1:
>> - #include <asm/apic.h> [kbuild test robot]
>> - use div64_u64() [kbuild test robot]
>> - DECLARE_WORK -> DECLARE_DELAYED_WORK as testing showed there's some bug
>>   in Hyper-V hypervisor and disabling emulation after receiving interrupt
>>   may screw TSC counters.
>
> Looks kinda fragile...

I believe this is temporary and Microsoft will fix things up host side.

>
>> ---
>>  arch/x86/entry/entry_64.S          |  4 +++
>>  arch/x86/hyperv/hv_init.c          | 71 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/include/asm/entry_arch.h  |  4 +++
>>  arch/x86/include/asm/hw_irq.h      |  1 +
>>  arch/x86/include/asm/irq_vectors.h |  7 +++-
>>  arch/x86/include/asm/mshyperv.h    |  8 +++++
>>  arch/x86/include/uapi/asm/hyperv.h | 27 +++++++++++++++
>>  arch/x86/kernel/idt.c              |  3 ++
>>  8 files changed, 124 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index f81d50d7ceac..a32730b260bc 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -826,6 +826,10 @@ apicinterrupt SPURIOUS_APIC_VECTOR		spurious_interrupt		smp_spurious_interrupt
>>  apicinterrupt IRQ_WORK_VECTOR			irq_work_interrupt		smp_irq_work_interrupt
>>  #endif
>>  
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +apicinterrupt HYPERV_REENLIGHTENMENT_VECTOR	hyperv_reenlightenment_intr	smp_hyperv_reenlightenment_intr
>> +#endif
>> +
>>  /*
>>   * Exception entry points.
>>   */
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 1a6c63f721bc..bb62839ede81 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -18,6 +18,7 @@
>>   */
>>  
>>  #include <linux/types.h>
>> +#include <asm/apic.h>
>>  #include <asm/hypervisor.h>
>>  #include <asm/hyperv.h>
>>  #include <asm/mshyperv.h>
>> @@ -102,6 +103,76 @@ static int hv_cpu_init(unsigned int cpu)
>>  	return 0;
>>  }
>>  
>> +static void (*hv_reenlightenment_cb)(void);
>> +
>> +static void hv_reenlightenment_notify(struct work_struct *dummy)
>> +{
>> +	if (hv_reenlightenment_cb)
>> +		hv_reenlightenment_cb();
>> +}
>> +static DECLARE_DELAYED_WORK(hv_reenlightenment_work, hv_reenlightenment_notify);
>> +
>> +void hyperv_stop_tsc_emulation(void)
>> +{
>> +	u64 freq;
>> +	struct hv_tsc_emulation_status emu_status;
>> +
>> +	rdmsrl(HV_X64_MSR_TSC_EMULATION_STATUS, *(u64 *)&emu_status);
>> +	emu_status.inprogress = 0;
>> +	wrmsrl(HV_X64_MSR_TSC_EMULATION_STATUS, *(u64 *)&emu_status);
>> +
>> +	rdmsrl(HV_X64_MSR_TSC_FREQUENCY, freq);
>
> IIRC the availability of this msr is not guaranteed (I guess in reality
> it's present if the reenlightenment is supported, but I'd rather check).
>

Will do.

>> +	tsc_khz = div64_u64(freq, 1000);
>> +}
>> +EXPORT_SYMBOL_GPL(hyperv_stop_tsc_emulation);
>> +
>> +void register_hv_tsc_update(void (*cb)(void))
>> +{
>
> The function name seems unfortunate.  IMHO such a name suggests
> registering a callback on a notifier chain (rather than unconditionally
> replacing the old callback), and having no other side effects.

I see, any suggestion? register_hv_reenlightenment_cb? register_hv_tscchange_cb?

>
>> +	struct hv_reenlightenment_control re_ctrl = {
>> +		.vector = HYPERV_REENLIGHTENMENT_VECTOR,
>> +		.enabled = 1,
>> +		.target_vp = hv_vp_index[smp_processor_id()]
>> +	};
>> +	struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>> +
>> +	if (!(ms_hyperv.features & HV_X64_ACCESS_REENLIGHTENMENT))
>> +		return;
>
> What happens then?  L2 guests keep running with their clocks ticking at
> a different speed?
>

In reallity this never happens -- in case nested virtualization is
supported reenlightenment is also available. In theory, L0 can emulate
TSC acceess for forever after migration.

>> +
>> +	hv_reenlightenment_cb = cb;
>> +
>> +	/* Make sure callback is registered before we write to MSRs */
>> +	wmb();
>> +
>> +	wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
>> +	wrmsrl(HV_X64_MSR_TSC_EMULATION_CONTROL, *((u64 *)&emu_ctrl));
>> +}
>> +EXPORT_SYMBOL_GPL(register_hv_tsc_update);
>> +
>> +void unregister_hv_tsc_update(void)
>> +{
>> +	struct hv_reenlightenment_control re_ctrl;
>> +
>> +	if (!(ms_hyperv.features & HV_X64_ACCESS_REENLIGHTENMENT))
>> +		return;
>> +
>> +	rdmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *(u64 *)&re_ctrl);
>> +	re_ctrl.enabled = 0;
>> +	wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *(u64 *)&re_ctrl);
>> +
>> +	hv_reenlightenment_cb = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(unregister_hv_tsc_update);
>> +
>> +asmlinkage __visible void
>> +__irq_entry smp_hyperv_reenlightenment_intr(struct pt_regs *regs)
>> +{
>> +	entering_ack_irq();
>> +
>> +	schedule_delayed_work(&hv_reenlightenment_work, HZ/10);
>> +
>> +	exiting_irq();
>> +}
>> +
>>  /*
>>   * This function is to be invoked early in the boot sequence after the
>>   * hypervisor has been detected.
>> diff --git a/arch/x86/include/asm/entry_arch.h b/arch/x86/include/asm/entry_arch.h
>> index 416422762845..eb936cc49b62 100644
>> --- a/arch/x86/include/asm/entry_arch.h
>> +++ b/arch/x86/include/asm/entry_arch.h
>> @@ -54,3 +54,7 @@ BUILD_INTERRUPT(threshold_interrupt,THRESHOLD_APIC_VECTOR)
>>  BUILD_INTERRUPT(deferred_error_interrupt, DEFERRED_ERROR_VECTOR)
>>  #endif
>>  #endif
>> +
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +BUILD_INTERRUPT(hyperv_reenlightenment_intr, HYPERV_REENLIGHTENMENT_VECTOR)
>> +#endif
>> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
>> index 2851077b6051..c65193dac7d9 100644
>> --- a/arch/x86/include/asm/hw_irq.h
>> +++ b/arch/x86/include/asm/hw_irq.h
>> @@ -36,6 +36,7 @@ extern asmlinkage void kvm_posted_intr_wakeup_ipi(void);
>>  extern asmlinkage void kvm_posted_intr_nested_ipi(void);
>>  extern asmlinkage void error_interrupt(void);
>>  extern asmlinkage void irq_work_interrupt(void);
>> +extern asmlinkage void hyperv_reenlightenment_intr(void);
>>  
>>  extern asmlinkage void spurious_interrupt(void);
>>  extern asmlinkage void thermal_interrupt(void);
>> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
>> index 67421f649cfa..e71c1120426b 100644
>> --- a/arch/x86/include/asm/irq_vectors.h
>> +++ b/arch/x86/include/asm/irq_vectors.h
>> @@ -103,7 +103,12 @@
>>  #endif
>>  
>>  #define MANAGED_IRQ_SHUTDOWN_VECTOR	0xef
>> -#define LOCAL_TIMER_VECTOR		0xee
>> +
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +#define HYPERV_REENLIGHTENMENT_VECTOR	0xee
>> +#endif
>> +
>> +#define LOCAL_TIMER_VECTOR		0xed
>>  
>>  #define NR_VECTORS			 256
>>  
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index a0b34994f453..43164b097585 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -314,11 +314,19 @@ void hyper_alloc_mmu(void);
>>  void hyperv_report_panic(struct pt_regs *regs, long err);
>>  bool hv_is_hypercall_page_setup(void);
>>  void hyperv_cleanup(void);
>> +
>> +asmlinkage void smp_hyperv_reenlightenment_intr(struct pt_regs *regs);
>> +void register_hv_tsc_update(void (*cb)(void));
>> +void unregister_hv_tsc_update(void);
>> +void hyperv_stop_tsc_emulation(void);
>>  #else /* CONFIG_HYPERV */
>>  static inline void hyperv_init(void) {}
>>  static inline bool hv_is_hypercall_page_setup(void) { return false; }
>>  static inline void hyperv_cleanup(void) {}
>>  static inline void hyperv_setup_mmu_ops(void) {}
>> +static inline void register_hv_tsc_update(void (*cb)(void)) {}
>> +static inline void unregister_hv_tsc_update(void) {}
>> +static inline void hyperv_stop_tsc_emulation(void) {};
>>  #endif /* CONFIG_HYPERV */
>>  
>>  #ifdef CONFIG_HYPERV_TSCPAGE
>> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h
>> index 1a5bfead93b4..197c2e6c7376 100644
>> --- a/arch/x86/include/uapi/asm/hyperv.h
>> +++ b/arch/x86/include/uapi/asm/hyperv.h
>> @@ -40,6 +40,9 @@
>>   */
>>  #define HV_X64_ACCESS_FREQUENCY_MSRS		(1 << 11)
>>  
>> +/* AccessReenlightenmentControls privilege */
>> +#define HV_X64_ACCESS_REENLIGHTENMENT		BIT(13)
>> +
>>  /*
>>   * Basic SynIC MSRs (HV_X64_MSR_SCONTROL through HV_X64_MSR_EOM
>>   * and HV_X64_MSR_SINT0 through HV_X64_MSR_SINT15) available
>> @@ -234,6 +237,30 @@
>>  #define HV_X64_MSR_CRASH_PARAMS		\
>>  		(1 + (HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0))
>>  
>> +/* TSC emulation after migration */
>> +#define HV_X64_MSR_REENLIGHTENMENT_CONTROL	0x40000106
>> +
>> +struct hv_reenlightenment_control {
>> +	u64 vector:8;
>> +	u64 reserved1:8;
>> +	u64 enabled:1;
>> +	u64 reserved2:15;
>> +	u64 target_vp:32;
>> +};
>> +
>> +#define HV_X64_MSR_TSC_EMULATION_CONTROL	0x40000107
>> +#define HV_X64_MSR_TSC_EMULATION_STATUS		0x40000108
>> +
>> +struct hv_tsc_emulation_control {
>> +	u64 enabled:1;
>> +	u64 reserved:63;
>> +};
>> +
>> +struct hv_tsc_emulation_status {
>> +	u64 inprogress:1;
>> +	u64 reserved:63;
>> +};
>> +
>>  #define HV_X64_MSR_HYPERCALL_ENABLE		0x00000001
>>  #define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT	12
>>  #define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_MASK	\
>> diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
>> index d985cef3984f..5b8512d48aa3 100644
>> --- a/arch/x86/kernel/idt.c
>> +++ b/arch/x86/kernel/idt.c
>> @@ -140,6 +140,9 @@ static const __initdata struct idt_data apic_idts[] = {
>>  # ifdef CONFIG_IRQ_WORK
>>  	INTG(IRQ_WORK_VECTOR,		irq_work_interrupt),
>>  # endif
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +	INTG(HYPERV_REENLIGHTENMENT_VECTOR, hyperv_reenlightenment_intr),
>> +#endif
>>  	INTG(SPURIOUS_APIC_VECTOR,	spurious_interrupt),
>>  	INTG(ERROR_APIC_VECTOR,		error_interrupt),
>>  #endif
>
> Roman.

-- 
  Vitaly

Powered by blists - more mailing lists