Re: [Qemu-devel] [PATCH v3 1/2] X86: Move rdmsr/wrmsr functionality to standalone functions

[Eduardo Habkost]

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