On Wed, Mar 15, 2017 at 01:59:20PM +0100, Julian Kirsch wrote: > Hi Eduardo, > > thanks for taking the time to look through the patch series. To recapitulate > for > the next iteration: > > 1.) Let the rd/wrmsr functions set the valid variable in case of > CONFIG_USER_ONLY being set. > 2.) Split up patch into code movement followed by reordering
After the code movement, I would also separate simple code reordering from the addition of new MSRs. > 3.) I included only MSR_KVM_SYSTEM_TIME_NEW because exposing > MSR_KVM_SYSTEM_TIME > to users will hardcode its presence forever, and from the comments next > to the macro definitions I figured MSR_KVM_SYSTEM_TIME should not be used > anymore. New guest code shouldn't use it, but we do implement them, and being able to look at it using the new helpers would still be useful. The existing kvm_put_msrs()/kvm_get_msrs() code still references MSR_KVM_SYSTEM_TIME, for example, and I am looking for ways to make it reuse x86_cpu_rdmsr()/x86_cpu_wrmsr(). > 4.) HyperV MSRs were assumed to be x86_64 specific because they have X64 > in their names, but it seems I was wrong on this one. Sorry for being > lured simply due to the naming convention. I was confused, too. I had to double-check the KVM kernel code to be sure it was really supported on i386. > > Best, > Julian > > On 14.03.2017 23:33, Eduardo Habkost wrote: > > Found something else that confused me: > > > > On Tue, Mar 14, 2017 at 02:16:32PM -0300, Eduardo Habkost wrote: > > [...] > >> +#if defined CONFIG_KVM && defined TARGET_X86_64 > > > > Why exactly are you making the hyperv MSRs x86_64-specific? I > > don't see anything at the QEMU code or kernel-side KVM code that > > makes it x86_64-specific. > > > >> + case HV_X64_MSR_HYPERCALL: > >> + val = env->msr_hv_hypercall; > >> + break; > >> + case HV_X64_MSR_GUEST_OS_ID: > >> + val = env->msr_hv_guest_os_id; > >> + break; > >> + case HV_X64_MSR_APIC_ASSIST_PAGE: > >> + val = env->msr_hv_vapic; > >> + break; > >> + case HV_X64_MSR_REFERENCE_TSC: > >> + val = env->msr_hv_tsc; > >> + break; > >> + case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4: > >> + val = env->msr_hv_crash_params[idx - HV_X64_MSR_CRASH_P0]; > >> + break; > >> + case HV_X64_MSR_VP_RUNTIME: > >> + val = env->msr_hv_runtime; > >> + break; > >> + case HV_X64_MSR_SCONTROL: > >> + val = env->msr_hv_synic_control; > >> + break; > >> + case HV_X64_MSR_SVERSION: > >> + val = env->msr_hv_synic_version; > >> + break; > >> + case HV_X64_MSR_SIEFP: > >> + val = env->msr_hv_synic_evt_page; > >> + break; > >> + case HV_X64_MSR_SIMP: > >> + val = env->msr_hv_synic_msg_page; > >> + break; > >> + case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15: > >> + val = env->msr_hv_synic_sint[idx - HV_X64_MSR_SINT0]; > >> + break; > >> + case HV_X64_MSR_STIMER0_CONFIG: > >> + case HV_X64_MSR_STIMER1_CONFIG: > >> + case HV_X64_MSR_STIMER2_CONFIG: > >> + case HV_X64_MSR_STIMER3_CONFIG: > >> + val = env->msr_hv_stimer_config[(idx - HV_X64_MSR_STIMER0_CONFIG) > >> / 2]; > >> + break; > >> + case HV_X64_MSR_STIMER0_COUNT: > >> + case HV_X64_MSR_STIMER1_COUNT: > >> + case HV_X64_MSR_STIMER2_COUNT: > >> + case HV_X64_MSR_STIMER3_COUNT: > >> + val = env->msr_hv_stimer_count[(idx - HV_X64_MSR_STIMER0_COUNT) / > >> 2]; > >> + break; > >> +#endif > > [...] > > > -- Eduardo
