From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrey Smetanin Subject: Re: [PATCH v2 2/5] target-i386/kvm: Hyper-V SynIC MSR's support Date: Wed, 11 Nov 2015 12:09:08 +0300 Message-ID: <564305B4.9060907@virtuozzo.com> References: <1447159964-22987-1-git-send-email-asmetanin@virtuozzo.com> <1447159964-22987-3-git-send-email-asmetanin@virtuozzo.com> <5641EDA2.6020707@redhat.com> Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Denis V. Lunev" , Richard Henderson , Eduardo Habkost , =?UTF-8?Q?Andreas_F=c3=a4rber?= , Marcelo Tosatti , Roman Kagan , To: Paolo Bonzini , Return-path: Received: from mx2.parallels.com ([199.115.105.18]:50606 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750773AbbKKJKr (ORCPT ); Wed, 11 Nov 2015 04:10:47 -0500 In-Reply-To: <5641EDA2.6020707@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 11/10/2015 04:14 PM, Paolo Bonzini wrote: > > > On 10/11/2015 13:52, Andrey Smetanin wrote: >> This patch does Hyper-V Synthetic interrupt >> controller(Hyper-V SynIC) MSR's support and >> migration. Hyper-V SynIC is enabled by cpu's >> 'hv-synic' option. >> >> This patch does not allow cpu creation if >> 'hv-synic' option specified but kernel >> doesn't support Hyper-V SynIC. >> >> Changes v2: >> * activate Hyper-V SynIC by enabling corresponding vcpu cap >> * reject cpu initialization if user requested Hyper-V SynIC >> but kernel does not support Hyper-V SynIC >> >> Signed-off-by: Andrey Smetanin >> Reviewed-by: Roman Kagan >> Signed-off-by: Denis V. Lunev >> CC: Paolo Bonzini >> CC: Richard Henderson >> CC: Eduardo Habkost >> CC: "Andreas F=C3=A4rber" >> CC: Marcelo Tosatti >> CC: Roman Kagan >> CC: Denis V. Lunev >> CC: kvm@vger.kernel.org >> >> --- >> target-i386/cpu-qom.h | 1 + >> target-i386/cpu.c | 1 + >> target-i386/cpu.h | 5 ++++ >> target-i386/kvm.c | 67 +++++++++++++++++++++++++++++++++++++++= +++++++++++- >> target-i386/machine.c | 39 ++++++++++++++++++++++++++++++ >> 5 files changed, 112 insertions(+), 1 deletion(-) >> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h >> index e3bfe9d..7ea5b34 100644 >> --- a/target-i386/cpu-qom.h >> +++ b/target-i386/cpu-qom.h >> @@ -94,6 +94,7 @@ typedef struct X86CPU { >> bool hyperv_reset; >> bool hyperv_vpindex; >> bool hyperv_runtime; >> + bool hyperv_synic; >> bool check_cpuid; >> bool enforce_cpuid; >> bool expose_kvm; >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index e5f1c5b..1462e19 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -3142,6 +3142,7 @@ static Property x86_cpu_properties[] =3D { >> DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false), >> DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false), >> DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), >> + DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false), >> DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), >> DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), >> DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >> index fc4a605..8cf33df 100644 >> --- a/target-i386/cpu.h >> +++ b/target-i386/cpu.h >> @@ -918,6 +918,11 @@ typedef struct CPUX86State { >> uint64_t msr_hv_tsc; >> uint64_t msr_hv_crash_params[HV_X64_MSR_CRASH_PARAMS]; >> uint64_t msr_hv_runtime; >> + uint64_t msr_hv_synic_control; >> + uint64_t msr_hv_synic_version; >> + uint64_t msr_hv_synic_evt_page; >> + uint64_t msr_hv_synic_msg_page; >> + uint64_t msr_hv_synic_sint[HV_SYNIC_SINT_COUNT]; >> >> /* exception/interrupt handling */ >> int error_code; >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >> index 2a9953b..cfcd01d 100644 >> --- a/target-i386/kvm.c >> +++ b/target-i386/kvm.c >> @@ -86,6 +86,7 @@ static bool has_msr_hv_crash; >> static bool has_msr_hv_reset; >> static bool has_msr_hv_vpindex; >> static bool has_msr_hv_runtime; >> +static bool has_msr_hv_synic; >> static bool has_msr_mtrr; >> static bool has_msr_xss; >> >> @@ -521,7 +522,8 @@ static bool hyperv_enabled(X86CPU *cpu) >> cpu->hyperv_crash || >> cpu->hyperv_reset || >> cpu->hyperv_vpindex || >> - cpu->hyperv_runtime); >> + cpu->hyperv_runtime || >> + cpu->hyperv_synic); >> } >> >> static Error *invtsc_mig_blocker; >> @@ -610,6 +612,14 @@ int kvm_arch_init_vcpu(CPUState *cs) >> if (cpu->hyperv_runtime && has_msr_hv_runtime) { >> c->eax |=3D HV_X64_MSR_VP_RUNTIME_AVAILABLE; >> } >> + if (cpu->hyperv_synic) { >> + if (!has_msr_hv_synic || >> + kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) { >> + fprintf(stderr, "Hyper-V SynIC is not supported by = kernel\n"); >> + return -ENOSYS; >> + } >> + c->eax |=3D HV_X64_MSR_SYNIC_AVAILABLE; >> + } >> c =3D &cpuid_data.entries[cpuid_i++]; >> c->function =3D HYPERV_CPUID_ENLIGHTMENT_INFO; >> if (cpu->hyperv_relaxed_timing) { >> @@ -950,6 +960,10 @@ static int kvm_get_supported_msrs(KVMState *s) >> has_msr_hv_runtime =3D true; >> continue; >> } >> + if (kvm_msr_list->indices[i] =3D=3D HV_X64_MSR_SCON= TROL) { >> + has_msr_hv_synic =3D true; >> + continue; >> + } >> } >> } >> >> @@ -1511,6 +1525,31 @@ static int kvm_put_msrs(X86CPU *cpu, int leve= l) >> kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_VP_RUNTIME, >> env->msr_hv_runtime); >> } >> + if (cpu->hyperv_synic) { >> + int j; >> + >> + if (!env->msr_hv_synic_version) { >> + /* First time initialization */ >> + env->msr_hv_synic_version =3D HV_SYNIC_VERSION_1; >> + for (j =3D 0; j < ARRAY_SIZE(env->msr_hv_synic_sint= ); j++) { >> + env->msr_hv_synic_sint[j] =3D HV_SYNIC_SINT_MAS= KED; >> + } >> + } > > I would prefer to put this in kvm_arch_init_vcpu, if possible. > Ok. I think the kvm_arch_init_vcpu() is called after migration restores= =20 cpu->env->msr_hv_synic_* values, so unconditional initialization of cpu->env->msr_hv_synic_* values can overwrite migrated values. The chec= k=20 "if (!env->msr_hv_synic_version) {" is neccessary for first time initialization to protect against such overwriting. This is why this=20 code migrates 'msr_hv_synic_version' value. >> + kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_SCONTROL, >> + env->msr_hv_synic_control); >> + kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_SVERSION, >> + env->msr_hv_synic_version); >> + kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_SIEFP, >> + env->msr_hv_synic_evt_page); >> + kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_SIMP, >> + env->msr_hv_synic_msg_page); >> + >> + for (j =3D 0; j < ARRAY_SIZE(env->msr_hv_synic_sint); j= ++) { >> + kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_SINT0 + j, >> + env->msr_hv_synic_sint[j]); >> + } >> + } >> if (has_msr_mtrr) { >> kvm_msr_entry_set(&msrs[n++], MSR_MTRRdefType, env->mt= rr_deftype); >> kvm_msr_entry_set(&msrs[n++], >> @@ -1879,6 +1918,17 @@ static int kvm_get_msrs(X86CPU *cpu) >> if (has_msr_hv_runtime) { >> msrs[n++].index =3D HV_X64_MSR_VP_RUNTIME; >> } >> + if (cpu->hyperv_synic) { >> + uint32_t msr; >> + >> + msrs[n++].index =3D HV_X64_MSR_SCONTROL; >> + msrs[n++].index =3D HV_X64_MSR_SVERSION; >> + msrs[n++].index =3D HV_X64_MSR_SIEFP; >> + msrs[n++].index =3D HV_X64_MSR_SIMP; >> + for (msr =3D HV_X64_MSR_SINT0; msr <=3D HV_X64_MSR_SINT15; = msr++) { >> + msrs[n++].index =3D msr; >> + } >> + } >> if (has_msr_mtrr) { >> msrs[n++].index =3D MSR_MTRRdefType; >> msrs[n++].index =3D MSR_MTRRfix64K_00000; >> @@ -2035,6 +2085,21 @@ static int kvm_get_msrs(X86CPU *cpu) >> case HV_X64_MSR_VP_RUNTIME: >> env->msr_hv_runtime =3D msrs[i].data; >> break; >> + case HV_X64_MSR_SCONTROL: >> + env->msr_hv_synic_control =3D msrs[i].data; >> + break; >> + case HV_X64_MSR_SVERSION: >> + env->msr_hv_synic_version =3D msrs[i].data; >> + break; >> + case HV_X64_MSR_SIEFP: >> + env->msr_hv_synic_evt_page =3D msrs[i].data; >> + break; >> + case HV_X64_MSR_SIMP: >> + env->msr_hv_synic_msg_page =3D msrs[i].data; >> + break; >> + case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15: >> + env->msr_hv_synic_sint[index - HV_X64_MSR_SINT0] =3D ms= rs[i].data; >> + break; >> case MSR_MTRRdefType: >> env->mtrr_deftype =3D msrs[i].data; >> break; >> diff --git a/target-i386/machine.c b/target-i386/machine.c >> index a18e16e..bdb2997 100644 >> --- a/target-i386/machine.c >> +++ b/target-i386/machine.c >> @@ -710,6 +710,44 @@ static const VMStateDescription vmstate_msr_hyp= erv_runtime =3D { >> } >> }; >> >> +static bool hyperv_synic_enable_needed(void *opaque) >> +{ >> + X86CPU *cpu =3D opaque; >> + CPUX86State *env =3D &cpu->env; >> + int i; >> + >> + if (env->msr_hv_synic_control !=3D 0 || >> + env->msr_hv_synic_version !=3D 0 || >> + env->msr_hv_synic_evt_page !=3D 0 || >> + env->msr_hv_synic_msg_page !=3D 0) { >> + return true; >> + } >> + >> + for (i =3D 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) { >> + if (env->msr_hv_synic_sint[i] !=3D 0) { >> + return true; >> + } >> + } >> + >> + return false; >> +} >> + >> +static const VMStateDescription vmstate_msr_hyperv_synic =3D { >> + .name =3D "cpu/msr_hyperv_synic", >> + .version_id =3D 1, >> + .minimum_version_id =3D 1, >> + .needed =3D hyperv_synic_enable_needed, >> + .fields =3D (VMStateField[]) { >> + VMSTATE_UINT64(env.msr_hv_synic_control, X86CPU), >> + VMSTATE_UINT64(env.msr_hv_synic_version, X86CPU), > > This one need not be migrated, since it's always the same value. > see my comment above > Paolo > >> + VMSTATE_UINT64(env.msr_hv_synic_evt_page, X86CPU), >> + VMSTATE_UINT64(env.msr_hv_synic_msg_page, X86CPU), >> + VMSTATE_UINT64_ARRAY(env.msr_hv_synic_sint, X86CPU, >> + HV_SYNIC_SINT_COUNT), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static bool avx512_needed(void *opaque) >> { >> X86CPU *cpu =3D opaque; >> @@ -893,6 +931,7 @@ VMStateDescription vmstate_x86_cpu =3D { >> &vmstate_msr_hyperv_time, >> &vmstate_msr_hyperv_crash, >> &vmstate_msr_hyperv_runtime, >> + &vmstate_msr_hyperv_synic, >> &vmstate_avx512, >> &vmstate_xss, >> NULL >>