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

[Eduardo Habkost]

On Tue, Mar 14, 2017 at 02:16:32PM -0300, Eduardo Habkost wrote:
> On Tue, Mar 14, 2017 at 03:08:06PM +0100, Julian Kirsch wrote:
> > Add two new functions to provide read/write access to model specific 
> > registers
> > (MSRs) on x86. Move original functionality to new functions
> > x86_cpu_[rd|wr]msr and make list of available MSRs consistent with KVM.
> 
> I would prefer to see code movement and other changes (like
> reordering) in separate patches, to make it easier to review.
> 
> To help reviewers, below is the diff between the old functions in
> misc_helper.c and new functions in helper.c:
> 
> --- /tmp/msrfuncs-old.c       2017-03-14 14:12:04.739686970 -0300
> +++ /tmp/msrfuncs-new.c       2017-03-14 14:11:50.108486602 -0300
> @@ -1,10 +1,10 @@
> -void helper_rdmsr(CPUX86State *env)
> +uint64_t x86_cpu_rdmsr(CPUX86State *env, uint32_t idx, bool *valid)
>  {
>      uint64_t val;
> +    *valid = true;
>  
> -    cpu_svm_check_intercept_param(env, SVM_EXIT_MSR, 0, GETPC());
> -
> -    switch ((uint32_t)env->regs[R_ECX]) {
> +    /* MSR list loosely following the order in kvm.c */
> +    switch (idx) {
>      case MSR_IA32_SYSENTER_CS:
>          val = env->sysenter_cs;
>          break;
[...]
> +#ifdef CONFIG_KVM
> +    /* Export kvm specific pseudo MSRs using their new ordinals only */

I suggest including KVM-specific code in a separate patch.

Also, why are you adding only MSR_KVM_SYSTEM_TIME to wrmsr, and
only MSR_KVM_SYSTEM_TIME_NEW to rdmsr? Shouldn't we include both
addresses on both helper functions?

> +    case MSR_KVM_SYSTEM_TIME_NEW:
> +        val = env->system_time_msr;
> +        break;
> +    case MSR_KVM_WALL_CLOCK_NEW:
> +        val = env->wall_clock_msr;
>          break;
> -    case MSR_TSC_AUX:
> -        val = env->tsc_aux;
> +    case MSR_KVM_ASYNC_PF_EN:
> +        val = env->async_pf_en_msr;
> +        break;
> +    case MSR_KVM_PV_EOI_EN:
> +        val = env->pv_eoi_en_msr;
> +        break;
> +    case MSR_KVM_STEAL_TIME:
> +        val = env->steal_time_msr;
>          break;
>  #endif
[...]
>  
> -void helper_wrmsr(CPUX86State *env)
> +void x86_cpu_wrmsr(CPUX86State *env, uint32_t idx, uint64_t val, bool *valid)
>  {
> -    uint64_t val;
> -
> -    cpu_svm_check_intercept_param(env, SVM_EXIT_MSR, 1, GETPC());
> -
> -    val = ((uint32_t)env->regs[R_EAX]) |
> -        ((uint64_t)((uint32_t)env->regs[R_EDX]) << 32);
> +    *valid = true;
> +    /* FIXME: With KVM nabled, only report success if we are sure the new 
> value
> +     * will actually be written back by the KVM subsystem later. */
>  
> -    switch ((uint32_t)env->regs[R_ECX]) {
> +    switch (idx) {
>      case MSR_IA32_SYSENTER_CS:
>          env->sysenter_cs = val & 0xffff;
>          break;
[...]
> +#ifdef CONFIG_KVM
> +    case MSR_KVM_SYSTEM_TIME:
> +        env->system_time_msr = val;
> +        break;
> +    case MSR_KVM_WALL_CLOCK:
> +        env->wall_clock_msr = val;
> +        break;
> +    case MSR_KVM_ASYNC_PF_EN:
> +        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF)) {
> +            env->async_pf_en_msr = val;
> +        } else {
> +            *valid = false;
> +        }
> +    case MSR_KVM_PV_EOI_EN:
> +        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_PV_EOI)) {
> +            env->pv_eoi_en_msr = val;
> +        } else {
> +            *valid = false;
> +        }
> +
> +#endif
[...]

-- 
Eduardo