[PATCH 0/4] VMX: DebugCtl MSR related adjustments

[PATCH 0/4] VMX: DebugCtl MSR related adjustments

[Jan Beulich]

1: fix DebugCtl MSR clearing
2: vPMU: fix DebugCtl MSR handling
3: allow RTM advanced debugging to be used by guests
4: vPMU: reduce core2_vpmu_initialise() verbosity

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/4] VMX: fix DebugCtl MSR clearing

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

The previous shortcut was wrong, as it bypassed the necessary vmwrite:
All we really want to avoid if the guest writes zero is to add the MSR
to the host-load list.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2246,8 +2246,6 @@ static int vmx_msr_write_intercept(unsig
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
 
-        if ( !msr_content )
-            break;
         if ( msr_content & ~supported )
         {
             /* Perhaps some other bits are supported in vpmu. */
@@ -2267,12 +2265,10 @@ static int vmx_msr_write_intercept(unsig
         }
 
         if ( (rc < 0) ||
-             (vmx_add_host_load_msr(msr) < 0) )
+             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
             hvm_inject_hw_exception(TRAP_machine_check, 0);
         else
-        {
             __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
-        }
 
         break;
     }




[-- Attachment #2: VMX-debuctl-clearing.patch --]
[-- Type: text/plain, Size: 1075 bytes --]

VMX: fix DebugCtl MSR clearing

The previous shortcut was wrong, as it bypassed the necessary vmwrite:
All we really want to avoid if the guest writes zero is to add the MSR
to the host-load list.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2246,8 +2246,6 @@ static int vmx_msr_write_intercept(unsig
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
 
-        if ( !msr_content )
-            break;
         if ( msr_content & ~supported )
         {
             /* Perhaps some other bits are supported in vpmu. */
@@ -2267,12 +2265,10 @@ static int vmx_msr_write_intercept(unsig
         }
 
         if ( (rc < 0) ||
-             (vmx_add_host_load_msr(msr) < 0) )
+             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
             hvm_inject_hw_exception(TRAP_machine_check, 0);
         else
-        {
             __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
-        }
 
         break;
     }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] VMX: fix DebugCtl MSR clearing

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1310 bytes --]

On 12/08/14 10:14, Jan Beulich wrote:
> The previous shortcut was wrong, as it bypassed the necessary vmwrite:
> All we really want to avoid if the guest writes zero is to add the MSR
> to the host-load list.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2246,8 +2246,6 @@ static int vmx_msr_write_intercept(unsig
>          int i, rc = 0;
>          uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
>  
> -        if ( !msr_content )
> -            break;
>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> @@ -2267,12 +2265,10 @@ static int vmx_msr_write_intercept(unsig
>          }
>  
>          if ( (rc < 0) ||
> -             (vmx_add_host_load_msr(msr) < 0) )
> +             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
>              hvm_inject_hw_exception(TRAP_machine_check, 0);
>          else
> -        {
>              __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
> -        }
>  
>          break;
>      }
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2193 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] VMX: fix DebugCtl MSR clearing

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, August 12, 2014 2:14 AM
> 
> The previous shortcut was wrong, as it bypassed the necessary vmwrite:
> All we really want to avoid if the guest writes zero is to add the MSR
> to the host-load list.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2246,8 +2246,6 @@ static int vmx_msr_write_intercept(unsig
>          int i, rc = 0;
>          uint64_t supported = IA32_DEBUGCTLMSR_LBR |
> IA32_DEBUGCTLMSR_BTF;
> 
> -        if ( !msr_content )
> -            break;
>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> @@ -2267,12 +2265,10 @@ static int vmx_msr_write_intercept(unsig
>          }
> 
>          if ( (rc < 0) ||
> -             (vmx_add_host_load_msr(msr) < 0) )
> +             (msr_content && (vmx_add_host_load_msr(msr) < 0)) )
>              hvm_inject_hw_exception(TRAP_machine_check, 0);
>          else
> -        {
>              __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
> -        }
> 
>          break;
>      }
> 
> 

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5696 bytes --]

- 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);
         }
         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,



[-- Attachment #2: VMX-vPMU-debugctl.patch --]
[-- Type: text/plain, Size: 5731 bytes --]

VMX/vPMU: fix DebugCtl MSR handling

- 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);
         }
         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,

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 6184 bytes --]

On 12/08/14 10:15, 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);
>          }
>          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);

It might, for clarity sake, be preferable to name the new parameter
"supported_bits".  At the moment, "supported" could equally well refer
to the MSR itself.

~Andrew

>  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,
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 6857 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.08.14 at 11:44, <andrew.cooper3@citrix.com> wrote:
> On 12/08/14 10:15, 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);
>>          }
>>          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);
> 
> It might, for clarity sake, be preferable to name the new parameter
> "supported_bits".  At the moment, "supported" could equally well refer
> to the MSR itself.

Not really, with it being of type uint64_t. Nor does the context
really suggest a meaning like what you imply.

Jan

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

[Boris Ostrovsky]

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,

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

[Jan Beulich]

>>> On 12.08.14 at 19:47, <boris.ostrovsky@oracle.com> wrote:
> On 08/12/2014 05:15 AM, Jan Beulich wrote:
>> -            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.

I'd rather not (at least not in this patch), not the least that I can
see the "supported" function parameter to potentially be useful
elsewhere when use of certain bits within an MSR is split across
components.

Jan

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

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, August 12, 2014 2:15 AM
> 
> - 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>

Acked-by: Kevin Tian <kevin.tian@intel.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);
>          }
>          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,
> 

[PATCH 3/4] VMX: allow RTM advanced debugging to be used by guests

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

All that is needed here is allowing the respective DebugCtl MSR bit to
be set by the guest.

At once - even if PV guests can't currently use it due to missing
DebugCtl MSR virtualization - make the respective adjustments to
debugreg.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2246,6 +2246,8 @@ static int vmx_msr_write_intercept(unsig
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
 
+        if ( boot_cpu_has(X86_FEATURE_RTM) )
+            supported |= IA32_DEBUGCTLMSR_RTM;
         if ( msr_content & ~supported )
         {
             /* Perhaps some other bits are supported in vpmu. */
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -20,6 +20,7 @@
 #define DR_TRAP3        (0x8)           /* db3 */
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
+#define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
 
 /* Now define a bunch of things for manipulating the control register.
    The top two bytes of the control register consist of 4 fields of 4
@@ -62,6 +63,7 @@
 #define DR_CONTROL_RESERVED_ONE  (0x00000400ul) /* Reserved, read as one */
 #define DR_LOCAL_EXACT_ENABLE    (0x00000100ul) /* Local exact enable */
 #define DR_GLOBAL_EXACT_ENABLE   (0x00000200ul) /* Global exact enable */
+#define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging enable */
 #define DR_GENERAL_DETECT        (0x00002000ul) /* General detect enable */
 
 #define write_debugreg(reg, val) do {                       \
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -79,6 +79,7 @@
 #define IA32_DEBUGCTLMSR_BTINT		(1<<8) /* Branch Trace Interrupt */
 #define IA32_DEBUGCTLMSR_BTS_OFF_OS	(1<<9)  /* BTS off if CPL 0 */
 #define IA32_DEBUGCTLMSR_BTS_OFF_USR	(1<<10) /* BTS off if CPL > 0 */
+#define IA32_DEBUGCTLMSR_RTM		(1<<15) /* RTM debugging enable */
 
 #define MSR_IA32_LASTBRANCHFROMIP	0x000001db
 #define MSR_IA32_LASTBRANCHTOIP		0x000001dc




[-- Attachment #2: VMX-RTM-debugging.patch --]
[-- Type: text/plain, Size: 2238 bytes --]

VMX: allow RTM advanced debugging to be used by guests

All that is needed here is allowing the respective DebugCtl MSR bit to
be set by the guest.

At once - even if PV guests can't currently use it due to missing
DebugCtl MSR virtualization - make the respective adjustments to
debugreg.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2246,6 +2246,8 @@ static int vmx_msr_write_intercept(unsig
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
 
+        if ( boot_cpu_has(X86_FEATURE_RTM) )
+            supported |= IA32_DEBUGCTLMSR_RTM;
         if ( msr_content & ~supported )
         {
             /* Perhaps some other bits are supported in vpmu. */
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -20,6 +20,7 @@
 #define DR_TRAP3        (0x8)           /* db3 */
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
+#define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
 
 /* Now define a bunch of things for manipulating the control register.
    The top two bytes of the control register consist of 4 fields of 4
@@ -62,6 +63,7 @@
 #define DR_CONTROL_RESERVED_ONE  (0x00000400ul) /* Reserved, read as one */
 #define DR_LOCAL_EXACT_ENABLE    (0x00000100ul) /* Local exact enable */
 #define DR_GLOBAL_EXACT_ENABLE   (0x00000200ul) /* Global exact enable */
+#define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging enable */
 #define DR_GENERAL_DETECT        (0x00002000ul) /* General detect enable */
 
 #define write_debugreg(reg, val) do {                       \
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -79,6 +79,7 @@
 #define IA32_DEBUGCTLMSR_BTINT		(1<<8) /* Branch Trace Interrupt */
 #define IA32_DEBUGCTLMSR_BTS_OFF_OS	(1<<9)  /* BTS off if CPL 0 */
 #define IA32_DEBUGCTLMSR_BTS_OFF_USR	(1<<10) /* BTS off if CPL > 0 */
+#define IA32_DEBUGCTLMSR_RTM		(1<<15) /* RTM debugging enable */
 
 #define MSR_IA32_LASTBRANCHFROMIP	0x000001db
 #define MSR_IA32_LASTBRANCHTOIP		0x000001dc

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] VMX: allow RTM advanced debugging to be used by guests

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2554 bytes --]

On 12/08/14 10:17, Jan Beulich wrote:
> All that is needed here is allowing the respective DebugCtl MSR bit to
> be set by the guest.
>
> At once - even if PV guests can't currently use it due to missing
> DebugCtl MSR virtualization - make the respective adjustments to
> debugreg.h.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2246,6 +2246,8 @@ static int vmx_msr_write_intercept(unsig
>          int i, rc = 0;
>          uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
>  
> +        if ( boot_cpu_has(X86_FEATURE_RTM) )
> +            supported |= IA32_DEBUGCTLMSR_RTM;

This supported bitmask is runtime constant.  Is it worth precalculating
it, rather than reevaluating each time DEBUGCTL is written to?

~Andrew

>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> --- a/xen/include/asm-x86/debugreg.h
> +++ b/xen/include/asm-x86/debugreg.h
> @@ -20,6 +20,7 @@
>  #define DR_TRAP3        (0x8)           /* db3 */
>  #define DR_STEP         (0x4000)        /* single-step */
>  #define DR_SWITCH       (0x8000)        /* task switch */
> +#define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
>  
>  /* Now define a bunch of things for manipulating the control register.
>     The top two bytes of the control register consist of 4 fields of 4
> @@ -62,6 +63,7 @@
>  #define DR_CONTROL_RESERVED_ONE  (0x00000400ul) /* Reserved, read as one */
>  #define DR_LOCAL_EXACT_ENABLE    (0x00000100ul) /* Local exact enable */
>  #define DR_GLOBAL_EXACT_ENABLE   (0x00000200ul) /* Global exact enable */
> +#define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging enable */
>  #define DR_GENERAL_DETECT        (0x00002000ul) /* General detect enable */
>  
>  #define write_debugreg(reg, val) do {                       \
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -79,6 +79,7 @@
>  #define IA32_DEBUGCTLMSR_BTINT		(1<<8) /* Branch Trace Interrupt */
>  #define IA32_DEBUGCTLMSR_BTS_OFF_OS	(1<<9)  /* BTS off if CPL 0 */
>  #define IA32_DEBUGCTLMSR_BTS_OFF_USR	(1<<10) /* BTS off if CPL > 0 */
> +#define IA32_DEBUGCTLMSR_RTM		(1<<15) /* RTM debugging enable */
>  
>  #define MSR_IA32_LASTBRANCHFROMIP	0x000001db
>  #define MSR_IA32_LASTBRANCHTOIP		0x000001dc
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3360 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] VMX: allow RTM advanced debugging to be used by guests

[Jan Beulich]

>>> On 12.08.14 at 12:08, <andrew.cooper3@citrix.com> wrote:
> On 12/08/14 10:17, Jan Beulich wrote:
>> All that is needed here is allowing the respective DebugCtl MSR bit to
>> be set by the guest.
>>
>> At once - even if PV guests can't currently use it due to missing
>> DebugCtl MSR virtualization - make the respective adjustments to
>> debugreg.h.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2246,6 +2246,8 @@ static int vmx_msr_write_intercept(unsig
>>          int i, rc = 0;
>>          uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
>>  
>> +        if ( boot_cpu_has(X86_FEATURE_RTM) )
>> +            supported |= IA32_DEBUGCTLMSR_RTM;
> 
> This supported bitmask is runtime constant.  Is it worth precalculating
> it, rather than reevaluating each time DEBUGCTL is written to?

If the calculation was expensive ...

Jan

Re: [PATCH 3/4] VMX: allow RTM advanced debugging to be used by guests

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, August 12, 2014 2:17 AM
> 
> All that is needed here is allowing the respective DebugCtl MSR bit to
> be set by the guest.
> 
> At once - even if PV guests can't currently use it due to missing
> DebugCtl MSR virtualization - make the respective adjustments to
> debugreg.h.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2246,6 +2246,8 @@ static int vmx_msr_write_intercept(unsig
>          int i, rc = 0;
>          uint64_t supported = IA32_DEBUGCTLMSR_LBR |
> IA32_DEBUGCTLMSR_BTF;
> 
> +        if ( boot_cpu_has(X86_FEATURE_RTM) )
> +            supported |= IA32_DEBUGCTLMSR_RTM;
>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> --- a/xen/include/asm-x86/debugreg.h
> +++ b/xen/include/asm-x86/debugreg.h
> @@ -20,6 +20,7 @@
>  #define DR_TRAP3        (0x8)           /* db3 */
>  #define DR_STEP         (0x4000)        /* single-step */
>  #define DR_SWITCH       (0x8000)        /* task switch */
> +#define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM
> region */
> 
>  /* Now define a bunch of things for manipulating the control register.
>     The top two bytes of the control register consist of 4 fields of 4
> @@ -62,6 +63,7 @@
>  #define DR_CONTROL_RESERVED_ONE  (0x00000400ul) /* Reserved, read
> as one */
>  #define DR_LOCAL_EXACT_ENABLE    (0x00000100ul) /* Local exact
> enable */
>  #define DR_GLOBAL_EXACT_ENABLE   (0x00000200ul) /* Global exact
> enable */
> +#define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging
> enable */
>  #define DR_GENERAL_DETECT        (0x00002000ul) /* General detect
> enable */
> 
>  #define write_debugreg(reg, val) do {                       \
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -79,6 +79,7 @@
>  #define IA32_DEBUGCTLMSR_BTINT		(1<<8) /* Branch Trace Interrupt */
>  #define IA32_DEBUGCTLMSR_BTS_OFF_OS	(1<<9)  /* BTS off if CPL 0 */
>  #define IA32_DEBUGCTLMSR_BTS_OFF_USR	(1<<10) /* BTS off if CPL > 0 */
> +#define IA32_DEBUGCTLMSR_RTM		(1<<15) /* RTM debugging enable
> */
> 
>  #define MSR_IA32_LASTBRANCHFROMIP	0x000001db
>  #define MSR_IA32_LASTBRANCHTOIP		0x000001dc
> 
> 

[PATCH 4/4] VMX/vPMU: reduce core2_vpmu_initialise() verbosity

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

No need to print these messages for each vCPU, even more, no need to
print them for each domain - they all depend on CPU features that are
either there or not.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -764,19 +764,19 @@ static int core2_vpmu_initialise(struct 
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     u64 msr_content;
-    struct cpuinfo_x86 *c = &current_cpu_data;
+    static bool_t ds_warned;
 
     if ( !(vpmu_flags & VPMU_BOOT_BTS) )
         goto func_out;
     /* Check the 'Debug Store' feature in the CPUID.EAX[1]:EDX[21] */
-    if ( cpu_has(c, X86_FEATURE_DS) )
+    while ( boot_cpu_has(X86_FEATURE_DS) )
     {
-        if ( !cpu_has(c, X86_FEATURE_DTES64) )
+        if ( !boot_cpu_has(X86_FEATURE_DTES64) )
         {
-            printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
-                   " - Debug Store disabled for %pv\n",
-                   v);
-            goto func_out;
+            if ( !ds_warned )
+                printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
+                       " - Debug Store disabled for guests\n");
+            break;
         }
         vpmu_set(vpmu, VPMU_CPU_HAS_DS);
         rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
@@ -784,14 +784,16 @@ static int core2_vpmu_initialise(struct 
         {
             /* If BTS_UNAVAIL is set reset the DS feature. */
             vpmu_reset(vpmu, VPMU_CPU_HAS_DS);
-            printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
-                   " - Debug Store disabled for %pv\n",
-                   v);
+            if ( !ds_warned )
+                printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
+                       " - Debug Store disabled for guests\n");
+            break;
         }
-        else
+
+        vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
+        if ( !ds_warned )
         {
-            vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
-            if ( !cpu_has(c, X86_FEATURE_DSCPL) )
+            if ( !boot_cpu_has(X86_FEATURE_DSCPL) )
                 printk(XENLOG_G_INFO
                        "vpmu: CPU doesn't support CPL-Qualified BTS\n");
             printk("******************************************************\n");
@@ -803,8 +805,10 @@ static int core2_vpmu_initialise(struct 
             printk("** It is NOT recommended for production use!        **\n");
             printk("******************************************************\n");
         }
+        break;
     }
-func_out:
+    ds_warned = 1;
+ func_out:
     check_pmc_quirk();
     return 0;
 }




[-- Attachment #2: VMX-vPMU-verbosity.patch --]
[-- Type: text/plain, Size: 2752 bytes --]

VMX/vPMU: reduce core2_vpmu_initialise() verbosity

No need to print these messages for each vCPU, even more, no need to
print them for each domain - they all depend on CPU features that are
either there or not.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -764,19 +764,19 @@ static int core2_vpmu_initialise(struct 
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     u64 msr_content;
-    struct cpuinfo_x86 *c = &current_cpu_data;
+    static bool_t ds_warned;
 
     if ( !(vpmu_flags & VPMU_BOOT_BTS) )
         goto func_out;
     /* Check the 'Debug Store' feature in the CPUID.EAX[1]:EDX[21] */
-    if ( cpu_has(c, X86_FEATURE_DS) )
+    while ( boot_cpu_has(X86_FEATURE_DS) )
     {
-        if ( !cpu_has(c, X86_FEATURE_DTES64) )
+        if ( !boot_cpu_has(X86_FEATURE_DTES64) )
         {
-            printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
-                   " - Debug Store disabled for %pv\n",
-                   v);
-            goto func_out;
+            if ( !ds_warned )
+                printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
+                       " - Debug Store disabled for guests\n");
+            break;
         }
         vpmu_set(vpmu, VPMU_CPU_HAS_DS);
         rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
@@ -784,14 +784,16 @@ static int core2_vpmu_initialise(struct 
         {
             /* If BTS_UNAVAIL is set reset the DS feature. */
             vpmu_reset(vpmu, VPMU_CPU_HAS_DS);
-            printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
-                   " - Debug Store disabled for %pv\n",
-                   v);
+            if ( !ds_warned )
+                printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
+                       " - Debug Store disabled for guests\n");
+            break;
         }
-        else
+
+        vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
+        if ( !ds_warned )
         {
-            vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
-            if ( !cpu_has(c, X86_FEATURE_DSCPL) )
+            if ( !boot_cpu_has(X86_FEATURE_DSCPL) )
                 printk(XENLOG_G_INFO
                        "vpmu: CPU doesn't support CPL-Qualified BTS\n");
             printk("******************************************************\n");
@@ -803,8 +805,10 @@ static int core2_vpmu_initialise(struct 
             printk("** It is NOT recommended for production use!        **\n");
             printk("******************************************************\n");
         }
+        break;
     }
-func_out:
+    ds_warned = 1;
+ func_out:
     check_pmc_quirk();
     return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4/4] VMX/vPMU: reduce core2_vpmu_initialise() verbosity

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3006 bytes --]

On 12/08/14 10:18, Jan Beulich wrote:
> No need to print these messages for each vCPU, even more, no need to
> print them for each domain - they all depend on CPU features that are
> either there or not.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -764,19 +764,19 @@ static int core2_vpmu_initialise(struct 
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      u64 msr_content;
> -    struct cpuinfo_x86 *c = &current_cpu_data;
> +    static bool_t ds_warned;
>  
>      if ( !(vpmu_flags & VPMU_BOOT_BTS) )
>          goto func_out;
>      /* Check the 'Debug Store' feature in the CPUID.EAX[1]:EDX[21] */
> -    if ( cpu_has(c, X86_FEATURE_DS) )
> +    while ( boot_cpu_has(X86_FEATURE_DS) )
>      {
> -        if ( !cpu_has(c, X86_FEATURE_DTES64) )
> +        if ( !boot_cpu_has(X86_FEATURE_DTES64) )
>          {
> -            printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
> -                   " - Debug Store disabled for %pv\n",
> -                   v);
> -            goto func_out;
> +            if ( !ds_warned )
> +                printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS Area"
> +                       " - Debug Store disabled for guests\n");
> +            break;
>          }
>          vpmu_set(vpmu, VPMU_CPU_HAS_DS);
>          rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
> @@ -784,14 +784,16 @@ static int core2_vpmu_initialise(struct 
>          {
>              /* If BTS_UNAVAIL is set reset the DS feature. */
>              vpmu_reset(vpmu, VPMU_CPU_HAS_DS);
> -            printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
> -                   " - Debug Store disabled for %pv\n",
> -                   v);
> +            if ( !ds_warned )
> +                printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
> +                       " - Debug Store disabled for guests\n");
> +            break;
>          }
> -        else
> +
> +        vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
> +        if ( !ds_warned )
>          {
> -            vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
> -            if ( !cpu_has(c, X86_FEATURE_DSCPL) )
> +            if ( !boot_cpu_has(X86_FEATURE_DSCPL) )
>                  printk(XENLOG_G_INFO
>                         "vpmu: CPU doesn't support CPL-Qualified BTS\n");
>              printk("******************************************************\n");
> @@ -803,8 +805,10 @@ static int core2_vpmu_initialise(struct 
>              printk("** It is NOT recommended for production use!        **\n");
>              printk("******************************************************\n");
>          }
> +        break;
>      }
> -func_out:
> +    ds_warned = 1;
> + func_out:
>      check_pmc_quirk();
>      return 0;
>  }
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3798 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4/4] VMX/vPMU: reduce core2_vpmu_initialise() verbosity

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, August 12, 2014 2:19 AM
> 
> No need to print these messages for each vCPU, even more, no need to
> print them for each domain - they all depend on CPU features that are
> either there or not.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -764,19 +764,19 @@ static int core2_vpmu_initialise(struct
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      u64 msr_content;
> -    struct cpuinfo_x86 *c = &current_cpu_data;
> +    static bool_t ds_warned;
> 
>      if ( !(vpmu_flags & VPMU_BOOT_BTS) )
>          goto func_out;
>      /* Check the 'Debug Store' feature in the CPUID.EAX[1]:EDX[21] */
> -    if ( cpu_has(c, X86_FEATURE_DS) )
> +    while ( boot_cpu_has(X86_FEATURE_DS) )
>      {
> -        if ( !cpu_has(c, X86_FEATURE_DTES64) )
> +        if ( !boot_cpu_has(X86_FEATURE_DTES64) )
>          {
> -            printk(XENLOG_G_WARNING "CPU doesn't support 64-bit DS
> Area"
> -                   " - Debug Store disabled for %pv\n",
> -                   v);
> -            goto func_out;
> +            if ( !ds_warned )
> +                printk(XENLOG_G_WARNING "CPU doesn't support 64-bit
> DS Area"
> +                       " - Debug Store disabled for guests\n");
> +            break;
>          }
>          vpmu_set(vpmu, VPMU_CPU_HAS_DS);
>          rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
> @@ -784,14 +784,16 @@ static int core2_vpmu_initialise(struct
>          {
>              /* If BTS_UNAVAIL is set reset the DS feature. */
>              vpmu_reset(vpmu, VPMU_CPU_HAS_DS);
> -            printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
> -                   " - Debug Store disabled for %pv\n",
> -                   v);
> +            if ( !ds_warned )
> +                printk(XENLOG_G_WARNING "CPU has set BTS_UNAVAIL"
> +                       " - Debug Store disabled for guests\n");
> +            break;
>          }
> -        else
> +
> +        vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
> +        if ( !ds_warned )
>          {
> -            vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
> -            if ( !cpu_has(c, X86_FEATURE_DSCPL) )
> +            if ( !boot_cpu_has(X86_FEATURE_DSCPL) )
>                  printk(XENLOG_G_INFO
>                         "vpmu: CPU doesn't support CPL-Qualified
> BTS\n");
> 
> printk("******************************************************\n");
> @@ -803,8 +805,10 @@ static int core2_vpmu_initialise(struct
>              printk("** It is NOT recommended for production use!
> **\n");
> 
> printk("******************************************************\n");
>          }
> +        break;
>      }
> -func_out:
> +    ds_warned = 1;
> + func_out:
>      check_pmc_quirk();
>      return 0;
>  }
> 
>