Re: [PATCH 2/4] VMX/vPMU: fix DebugCtl MSR handling

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/12/2014 05:15 AM, Jan Beulich wrote:
> - writes with one of the vPMU-supported flags and some unsupported one
>    set got accepted, leading to a VM entry failure
> - writes with one of the vPMU-supported flags set but the Debug Store
>    feature unavailable produced a #GP (other than other writes to this
>    MSR with unsupported bits set)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1789,7 +1789,7 @@ static int svm_msr_write_intercept(unsig
>       case MSR_AMD_FAM15H_EVNTSEL3:
>       case MSR_AMD_FAM15H_EVNTSEL4:
>       case MSR_AMD_FAM15H_EVNTSEL5:
> -        vpmu_do_wrmsr(msr, msr_content);
> +        vpmu_do_wrmsr(msr, msr_content, 0);
>           break;
>   
>       case MSR_IA32_MCx_MISC(4): /* Threshold register */
> --- a/xen/arch/x86/hvm/svm/vpmu.c
> +++ b/xen/arch/x86/hvm/svm/vpmu.c
> @@ -278,11 +278,14 @@ static void context_update(unsigned int
>       }
>   }
>   
> -static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
> +                             uint64_t supported)
>   {
>       struct vcpu *v = current;
>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
>   
> +    ASSERT(!supported);
> +
>       /* For all counters, enable guest only mode for HVM guest */
>       if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) &&
>           !(is_guest_mode(msr_content)) )
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2249,7 +2249,7 @@ static int vmx_msr_write_intercept(unsig
>           if ( msr_content & ~supported )
>           {
>               /* Perhaps some other bits are supported in vpmu. */
> -            if ( !vpmu_do_wrmsr(msr, msr_content) )
> +            if ( !vpmu_do_wrmsr(msr, msr_content, supported) )
>                   break;
>           }
>           if ( msr_content & IA32_DEBUGCTLMSR_LBR )
> @@ -2278,7 +2278,7 @@ static int vmx_msr_write_intercept(unsig
>               goto gp_fault;
>           break;
>       default:
> -        if ( vpmu_do_wrmsr(msr, msr_content) )
> +        if ( vpmu_do_wrmsr(msr, msr_content, 0) )
>               return X86EMUL_OKAY;
>           if ( passive_domain_do_wrmsr(msr, msr_content) )
>               return X86EMUL_OKAY;
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -454,7 +454,8 @@ static int core2_vpmu_msr_common_check(u
>       return 1;
>   }
>   
> -static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
> +                               uint64_t supported)
>   {
>       u64 global_ctrl, non_global_ctrl;
>       char pmu_enable = 0;
> @@ -469,24 +470,26 @@ static int core2_vpmu_do_wrmsr(unsigned
>           /* Special handling for BTS */
>           if ( msr == MSR_IA32_DEBUGCTLMSR )
>           {
> -            uint64_t supported = IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
> -                                 IA32_DEBUGCTLMSR_BTINT;
> +            supported |= IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
> +                         IA32_DEBUGCTLMSR_BTINT;
>   
>               if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
>                   supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS |
>                                IA32_DEBUGCTLMSR_BTS_OFF_USR;
> -            if ( msr_content & supported )
> -            {
> -                if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> -                    return 1;
> -                gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n");
> -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -                return 0;
> -            }
> +            if ( !(msr_content & ~supported) &&
> +                 vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> +                return 1;
> +            if ( (msr_content & supported) &&
> +                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> +                printk(XENLOG_G_WARNING
> +                       "%pv: Debug Store unsupported on this CPU\n",
> +                       current);


Can we move this if statement out of VPMU code into 
vmx_msr_write_intercept()? There is not a whole lot of VPMU-specific 
logic here and I am not sure I see much reason to go through 
vendor-independent code (vpmu.c) to do something that is very much 
Intel-specific.

You will need to start using vpmu_is_set() there but the upside is that 
there is no need to introduce another parameter that does nothing on AMD 
(and even on Intel is useful only for this particular register)

Or maybe add another Intel-specific routine in vpmu_core2.c to handle 
this register?


-boris


>           }
>           return 0;
>       }
>   
> +    ASSERT(!supported);
> +
>       core2_vpmu_cxt = vpmu->context;
>       switch ( msr )
>       {
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -64,12 +64,12 @@ static void __init parse_vpmu_param(char
>       }
>   }
>   
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported)
>   {
>       struct vpmu_struct *vpmu = vcpu_vpmu(current);
>   
>       if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> -        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported);
>       return 0;
>   }
>   
> --- a/xen/include/asm-x86/hvm/vpmu.h
> +++ b/xen/include/asm-x86/hvm/vpmu.h
> @@ -45,7 +45,8 @@
>   
>   /* Arch specific operations shared by all vpmus */
>   struct arch_vpmu_ops {
> -    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content);
> +    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content,
> +                    uint64_t supported);
>       int (*do_rdmsr)(unsigned int msr, uint64_t *msr_content);
>       int (*do_interrupt)(struct cpu_user_regs *regs);
>       void (*do_cpuid)(unsigned int input,
> @@ -86,7 +87,7 @@ struct vpmu_struct {
>   #define vpmu_is_set(_vpmu, _x) ((_vpmu)->flags & (_x))
>   #define vpmu_clear(_vpmu)      ((_vpmu)->flags = 0)
>   
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content);
> +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported);
>   int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);
>   int vpmu_do_interrupt(struct cpu_user_regs *regs);
>   void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,