[PATCH 2 of 2] V2 vpmu: Add the BTS extension

[PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Dietmar Hahn]

Add the BTS functionality to the existing vpmu implementation for Intel cpus.

Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>


 xen/arch/x86/hvm/svm/vpmu.c              |    9 +-
 xen/arch/x86/hvm/vmx/vmx.c               |   12 +-
 xen/arch/x86/hvm/vmx/vpmu_core2.c        |  181 +++++++++++++++++++++++++-----
 xen/arch/x86/hvm/vpmu.c                  |   37 +++++-
 xen/include/asm-x86/hvm/vmx/vpmu_core2.h |    1 +
 xen/include/asm-x86/hvm/vpmu.h           |   16 ++-
 xen/include/asm-x86/msr-index.h          |    5 +
 7 files changed, 216 insertions(+), 45 deletions(-)


# HG changeset patch
# User Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
# Date 1331111599 -3600
# Node ID 639568e24ab6bbf6a865191168092b1e84902584
# Parent  313da9e9c1cc230a827d9239f7b8deb9f35c2aeb
[mq]: patch_vpmu_bts_V2.diff

diff -r 313da9e9c1cc -r 639568e24ab6 xen/arch/x86/hvm/svm/vpmu.c
--- a/xen/arch/x86/hvm/svm/vpmu.c	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/arch/x86/hvm/svm/vpmu.c	Wed Mar 07 10:13:19 2012 +0100
@@ -363,10 +363,11 @@ struct arch_vpmu_ops amd_vpmu_ops = {
     .arch_vpmu_load = amd_vpmu_restore
 };
 
-int svm_vpmu_initialise(struct vcpu *v)
+int svm_vpmu_initialise(struct vcpu *v, int vpmu_flags /* unused */)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     uint8_t family = current_cpu_data.x86;
+    int ret = 0;
 
     switch ( family )
     {
@@ -374,8 +375,10 @@ int svm_vpmu_initialise(struct vcpu *v)
     case 0x12:
     case 0x14:
     case 0x15:
-        vpmu->arch_vpmu_ops = &amd_vpmu_ops;
-        return amd_vpmu_initialise(v);
+        ret = amd_vpmu_initialise(v);
+        if ( !ret )
+            vpmu->arch_vpmu_ops = &amd_vpmu_ops;
+        return ret;
     }
 
     printk("VPMU: Initialization failed. "
diff -r 313da9e9c1cc -r 639568e24ab6 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Mar 07 10:13:19 2012 +0100
@@ -1833,6 +1833,9 @@ static int vmx_msr_read_intercept(unsign
         /* Debug Trace Store is not supported. */
         *msr_content |= MSR_IA32_MISC_ENABLE_BTS_UNAVAIL |
                        MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
+        /* Perhaps vpmu will change some bits. */
+        if ( vpmu_do_rdmsr(msr, msr_content) )
+            goto done;
         break;
     default:
         if ( vpmu_do_rdmsr(msr, msr_content) )
@@ -1960,9 +1963,14 @@ static int vmx_msr_write_intercept(unsig
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
 
-        if ( !msr_content || (msr_content & ~supported) )
+        if ( !msr_content )
             break;
-
+        if ( msr_content & ~supported )
+        {
+            /* Perhaps some other bits are supported in vpmu. */
+            if ( !vpmu_do_wrmsr(msr, msr_content) )
+                break;
+        }
         if ( msr_content & IA32_DEBUGCTLMSR_LBR )
         {
             const struct lbr_info *lbr = last_branch_msr_get();
diff -r 313da9e9c1cc -r 639568e24ab6 xen/arch/x86/hvm/vmx/vpmu_core2.c
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c	Wed Mar 07 10:13:19 2012 +0100
@@ -169,7 +169,7 @@ static int is_core2_vpmu_msr(u32 msr_ind
             return 1;
         }
     }
-    
+
     for ( i = 0; i < core2_ctrls.num; i++ )
     {
         if ( core2_ctrls.msr[i] == msr_index )
@@ -406,7 +406,31 @@ static int core2_vpmu_do_wrmsr(unsigned 
     struct core2_vpmu_context *core2_vpmu_cxt = NULL;
 
     if ( !core2_vpmu_msr_common_check(msr, &type, &index) )
+    {
+        /* Special handling for BTS */
+        if ( msr == MSR_IA32_DEBUGCTLMSR )
+        {
+            uint64_t 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) )
+                {
+                    gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n");
+                    vmx_inject_hw_exception(TRAP_gp_fault, 0);
+                    return 0;
+                }
+                return 1;
+            }
+        }
         return 0;
+    }
 
     core2_vpmu_cxt = vpmu->context;
     switch ( msr )
@@ -425,8 +449,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
                      "which is not supported.\n");
         return 1;
     case MSR_IA32_DS_AREA:
-        gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
-        return 1;
+        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
+        {
+            if (!msr_content || !is_canonical_address(msr_content))
+            {
+                gdprintk(XENLOG_WARNING, "Illegal address for IA32_DS_AREA: 0x%lx\n",
+                                                            msr_content);
+                vmx_inject_hw_exception(TRAP_gp_fault, 0);
+                return 1;
+            }
+            else
+            {
+                core2_vpmu_cxt->pmu_enable->ds_area_enable = msr_content ? 1 : 0;
+                break;
+            }
+        }
+        else
+        {
+            gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
+            return 1;
+        }
     case MSR_CORE_PERF_GLOBAL_CTRL:
         global_ctrl = msr_content;
         for ( i = 0; i < core2_get_pmc_count(); i++ )
@@ -471,6 +513,7 @@ static int core2_vpmu_do_wrmsr(unsigned 
         pmu_enable |= core2_vpmu_cxt->pmu_enable->fixed_ctr_enable[i];
     for ( i = 0; i < core2_get_pmc_count(); i++ )
         pmu_enable |= core2_vpmu_cxt->pmu_enable->arch_pmc_enable[i];
+    pmu_enable |= core2_vpmu_cxt->pmu_enable->ds_area_enable;
     if ( pmu_enable )
         vpmu_set(vpmu, VPMU_RUNNING);
     else
@@ -496,6 +539,8 @@ static int core2_vpmu_do_wrmsr(unsigned 
                 inject_gp = 1;
             break;
         case MSR_TYPE_CTRL:           /* IA32_FIXED_CTR_CTRL */
+            if  ( msr == MSR_IA32_DS_AREA )
+                break;
             /* 4 bits per counter, currently 3 fixed counters implemented. */
             mask = ~((1ull << (3 * 4)) - 1);
             if (msr_content & mask)
@@ -525,25 +570,35 @@ static int core2_vpmu_do_rdmsr(unsigned 
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     struct core2_vpmu_context *core2_vpmu_cxt = NULL;
 
-    if ( !core2_vpmu_msr_common_check(msr, &type, &index) )
-        return 0;
-
-    core2_vpmu_cxt = vpmu->context;
-    switch ( msr )
+    if ( core2_vpmu_msr_common_check(msr, &type, &index) )
     {
-    case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-        *msr_content = 0;
-        break;
-    case MSR_CORE_PERF_GLOBAL_STATUS:
-        *msr_content = core2_vpmu_cxt->global_ovf_status;
-        break;
-    case MSR_CORE_PERF_GLOBAL_CTRL:
-        vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, msr_content);
-        break;
-    default:
-        rdmsrl(msr, *msr_content);
+        core2_vpmu_cxt = vpmu->context;
+        switch ( msr )
+        {
+        case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
+            *msr_content = 0;
+            break;
+        case MSR_CORE_PERF_GLOBAL_STATUS:
+            *msr_content = core2_vpmu_cxt->global_ovf_status;
+            break;
+        case MSR_CORE_PERF_GLOBAL_CTRL:
+            vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, msr_content);
+            break;
+        default:
+            rdmsrl(msr, *msr_content);
+        }
     }
-
+    else
+    {
+        /* Extension for BTS */
+        if ( msr == MSR_IA32_MISC_ENABLE)
+        {
+            if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
+                *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
+        }
+        else
+            return 0;
+    }
     return 1;
 }
 
@@ -551,6 +606,16 @@ static void core2_vpmu_do_cpuid(unsigned
                                 unsigned int *eax, unsigned int *ebx,
                                 unsigned int *ecx, unsigned int *edx)
 {
+    if (input == 0x1)
+    {
+        struct vpmu_struct *vpmu = vcpu_vpmu(current);
+
+        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
+        {
+            /* Switch on the 'Debug Store' feature in CPUID.EAX[1]:EDX[21] */
+            *edx |= cpufeat_mask(X86_FEATURE_DS);
+        }
+    }
 }
 
 static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs)
@@ -564,15 +629,21 @@ static int core2_vpmu_do_interrupt(struc
     struct vlapic *vlapic = vcpu_vlapic(v);
 
     rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content);
-    if ( !msr_content )
-        return 0;
-
-    if ( is_pmc_quirk )
-        handle_pmc_quirk(msr_content);
-
-    core2_vpmu_cxt->global_ovf_status |= msr_content;
-    msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1);
-    wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
+    if ( msr_content )
+    {
+        if ( is_pmc_quirk )
+            handle_pmc_quirk(msr_content);
+        core2_vpmu_cxt->global_ovf_status |= msr_content;
+        msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1);
+        wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
+    }
+    else
+    {
+        /* No PMC overflow but perhaps a Trace Message interrupt. */
+        msr_content = __vmread(GUEST_IA32_DEBUGCTL);
+        if ( !(msr_content & IA32_DEBUGCTLMSR_TR) )
+            return 0;
+    }
 
     apic_write_around(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
 
@@ -589,8 +660,48 @@ static int core2_vpmu_do_interrupt(struc
     return 1;
 }
 
-static int core2_vpmu_initialise(struct vcpu *v)
+static int core2_vpmu_initialise(struct vcpu *v, int vpmu_flags)
 {
+    struct vpmu_struct *vpmu = vcpu_vpmu(v);
+    u64 msr_content;
+    struct cpuinfo_x86 *c = &current_cpu_data;
+
+    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) )
+    {
+        if ( !cpu_has(c, X86_FEATURE_DTES64) )
+        {
+            gdprintk(XENLOG_WARNING, "CPU doesn't support 64-bit DS Area - Debug Store disabled\n");
+            goto func_out;
+        }
+        vpmu_set(vpmu, VPMU_CPU_HAS_DS);
+        rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
+        if ( msr_content & MSR_IA32_MISC_ENABLE_BTS_UNAVAIL )
+        {
+            /* If BTS_UNAVAIL is set reset the DS feature. */
+            vpmu_reset(vpmu, VPMU_CPU_HAS_DS);
+            gdprintk(XENLOG_WARNING, "CPU has set BTS_UNAVAIL - Debug Store disabled\n");
+        }
+        else
+        {
+            vpmu_set(vpmu, VPMU_CPU_HAS_BTS);
+            if ( !cpu_has(c, X86_FEATURE_DSCPL) )
+            {
+                gdprintk(XENLOG_WARNING, "vpmu: cpu doesn't support CPL-Qualified BTS\n");
+            }
+            printk("********************************************************\n");
+            printk("******* WARNING: Emulation of BTS Feature is switched on\n");
+            printk("******* Using this processor feature in a virtualized   \n");
+            printk("******* environment is not 100%% safe.                  \n");
+            printk("******* Setting the DS buffer address with wrong values \n");
+            printk("******* may lead to hypervisor hangs or crashes.        \n");
+            printk("******* It is NOT recommended for production use!       \n");
+            printk("********************************************************\n");
+        }
+    }
+func_out:
     check_pmc_quirk();
     return 0;
 }
@@ -620,11 +731,12 @@ struct arch_vpmu_ops core2_vpmu_ops = {
     .arch_vpmu_load = core2_vpmu_load
 };
 
-int vmx_vpmu_initialise(struct vcpu *v)
+int vmx_vpmu_initialise(struct vcpu *v, int vpmu_flags)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     uint8_t family = current_cpu_data.x86;
     uint8_t cpu_model = current_cpu_data.x86_model;
+    int ret = 0;
 
     if ( family == 6 )
     {
@@ -639,11 +751,12 @@ int vmx_vpmu_initialise(struct vcpu *v)
         case 46:
         case 47:
         case 58:
-            vpmu->arch_vpmu_ops = &core2_vpmu_ops;
-            return core2_vpmu_initialise(v);
+            ret = core2_vpmu_initialise(v, vpmu_flags);
+            if ( !ret)
+                vpmu->arch_vpmu_ops = &core2_vpmu_ops;
+            return ret;
         }
     }
-
     printk("VPMU: Initialization failed. "
            "Intel processor family %d model %d has not "
            "been supported\n", family, cpu_model);
diff -r 313da9e9c1cc -r 639568e24ab6 xen/arch/x86/hvm/vpmu.c
--- a/xen/arch/x86/hvm/vpmu.c	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/arch/x86/hvm/vpmu.c	Wed Mar 07 10:13:19 2012 +0100
@@ -32,8 +32,37 @@
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/svm/vmcb.h>
 
-static bool_t __read_mostly opt_vpmu_enabled;
-boolean_param("vpmu", opt_vpmu_enabled);
+
+/*
+ * "vpmu" :     vpmu generally enabled
+ * "vpmu=off" : vpmu generally disabled
+ * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
+ */
+static int __read_mostly opt_vpmu_enabled;
+static void parse_vpmu_param(char *s);
+custom_param("vpmu", parse_vpmu_param);
+
+static void __init parse_vpmu_param(char *s)
+{
+
+    if ( !parse_bool(s) )
+    {
+        opt_vpmu_enabled = 0;
+    }
+    else
+    {
+        opt_vpmu_enabled |= VPMU_BOOT_ENABLED;
+        if ( !strcmp(s, "bts") )
+        {
+            opt_vpmu_enabled |= VPMU_BOOT_BTS;
+        }
+        else
+        {
+            printk("VPMU: unknown vpmu flag: %s - vpmu disabled!\n", s);
+            opt_vpmu_enabled = 0;
+        }
+    }
+}
 
 int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
 {
@@ -104,12 +133,12 @@ void vpmu_initialise(struct vcpu *v)
     switch ( vendor )
     {
     case X86_VENDOR_AMD:
-        if ( svm_vpmu_initialise(v) != 0 )
+        if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
             opt_vpmu_enabled = 0;
         break;
 
     case X86_VENDOR_INTEL:
-        if ( vmx_vpmu_initialise(v) != 0 )
+        if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
             opt_vpmu_enabled = 0;
         break;
 
diff -r 313da9e9c1cc -r 639568e24ab6 xen/include/asm-x86/hvm/vmx/vpmu_core2.h
--- a/xen/include/asm-x86/hvm/vmx/vpmu_core2.h	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vpmu_core2.h	Wed Mar 07 10:13:19 2012 +0100
@@ -29,6 +29,7 @@ struct arch_msr_pair {
 };
 
 struct core2_pmu_enable {
+    char ds_area_enable;
     char fixed_ctr_enable[3];
     char arch_pmc_enable[1];
 };
diff -r 313da9e9c1cc -r 639568e24ab6 xen/include/asm-x86/hvm/vpmu.h
--- a/xen/include/asm-x86/hvm/vpmu.h	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/include/asm-x86/hvm/vpmu.h	Wed Mar 07 10:13:19 2012 +0100
@@ -22,6 +22,15 @@
 #ifndef __ASM_X86_HVM_VPMU_H_
 #define __ASM_X86_HVM_VPMU_H_
 
+
+/*
+ * Flag bits given as a string on the hypervisor boot parameter 'vpmu'.
+ * See arch/x86/hvm/vpmu.c.
+ */
+#define VPMU_BOOT_ENABLED 0x1    /* vpmu generally enabled. */
+#define VPMU_BOOT_BTS     0x2    /* Intel BTS feature wanted. */
+
+
 #define msraddr_to_bitpos(x) (((x)&0xffff) + ((x)>>31)*0x2000)
 #define vcpu_vpmu(vcpu)   (&((vcpu)->arch.hvm_vcpu.vpmu))
 #define vpmu_vcpu(vpmu)   (container_of((vpmu), struct vcpu, \
@@ -48,8 +57,8 @@ struct arch_vpmu_ops {
     void (*arch_vpmu_load)(struct vcpu *v);
 };
 
-int vmx_vpmu_initialise(struct vcpu *v);
-int svm_vpmu_initialise(struct vcpu *v);
+int vmx_vpmu_initialise(struct vcpu *v, int vpmu_flags);
+int svm_vpmu_initialise(struct vcpu *v, int vpmu_flags);
 
 struct vpmu_struct {
     u32 flags;
@@ -61,6 +70,9 @@ struct vpmu_struct {
 #define VPMU_CONTEXT_LOADED                 0x2
 #define VPMU_RUNNING                        0x4
 #define VPMU_PASSIVE_DOMAIN_ALLOCATED       0x8
+#define VPMU_CPU_HAS_DS                     0x10 /* Has Debug Store */
+#define VPMU_CPU_HAS_BTS                    0x20 /* Has Branche Trace Store */
+
 
 #define vpmu_set(_vpmu, _x)    ((_vpmu)->flags |= (_x))
 #define vpmu_reset(_vpmu, _x)  ((_vpmu)->flags &= ~(_x))
diff -r 313da9e9c1cc -r 639568e24ab6 xen/include/asm-x86/msr-index.h
--- a/xen/include/asm-x86/msr-index.h	Tue Mar 06 14:24:07 2012 +0100
+++ b/xen/include/asm-x86/msr-index.h	Wed Mar 07 10:13:19 2012 +0100
@@ -67,6 +67,11 @@
 #define MSR_IA32_DEBUGCTLMSR		0x000001d9
 #define IA32_DEBUGCTLMSR_LBR		(1<<0) /* Last Branch Record */
 #define IA32_DEBUGCTLMSR_BTF		(1<<1) /* Single Step on Branches */
+#define IA32_DEBUGCTLMSR_TR		(1<<6) /* Trace Message Enable */
+#define IA32_DEBUGCTLMSR_BTS		(1<<7) /* Branch Trace Store */
+#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 MSR_IA32_LASTBRANCHFROMIP	0x000001db
 #define MSR_IA32_LASTBRANCHTOIP		0x000001dc


--
Company details: http://ts.fujitsu.com/imprint.html

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Jan Beulich]

>>> On 07.03.12 at 14:13, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote:
> Add the BTS functionality to the existing vpmu implementation for Intel 
> cpus.
> 
> Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>

I was about to get this committed, but I noticed some inconsistency:

> @@ -425,8 +449,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
>                       "which is not supported.\n");
>          return 1;
>      case MSR_IA32_DS_AREA:
> -        gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> -        return 1;
> +        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
> +        {
> +            if (!msr_content || !is_canonical_address(msr_content))
> +            {
> +                gdprintk(XENLOG_WARNING, "Illegal address for IA32_DS_AREA: 0x%lx\n",
> +                                                            msr_content);
> +                vmx_inject_hw_exception(TRAP_gp_fault, 0);
> +                return 1;
> +            }
> +            else
> +            {
> +                core2_vpmu_cxt->pmu_enable->ds_area_enable = msr_content ? 1 : 0;

In the if() above you already exclude msr_content == 0, so why
the conditional here? Or is it the other way around - the conditional
here is correct, and the respective part of the if() clause above is
wrong?

As I did already make a few other cleanup adjustments to your patch,
please just let me know.

Jan

> +                break;
> +            }
> +        }
> +        else
> +        {
> +            gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> +            return 1;
> +        }
>      case MSR_CORE_PERF_GLOBAL_CTRL:
>          global_ctrl = msr_content;
>          for ( i = 0; i < core2_get_pmc_count(); i++ )

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Dietmar Hahn]

Am Dienstag 13 März 2012, 14:22:45 schrieb Jan Beulich:
> >>> On 07.03.12 at 14:13, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote:
> > Add the BTS functionality to the existing vpmu implementation for Intel 
> > cpus.
> > 
> > Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> 
> I was about to get this committed, but I noticed some inconsistency:
> 
> > @@ -425,8 +449,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
> >                       "which is not supported.\n");
> >          return 1;
> >      case MSR_IA32_DS_AREA:
> > -        gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> > -        return 1;
> > +        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
> > +        {
> > +            if (!msr_content || !is_canonical_address(msr_content))
> > +            {
> > +                gdprintk(XENLOG_WARNING, "Illegal address for IA32_DS_AREA: 0x%lx\n",
> > +                                                            msr_content);
> > +                vmx_inject_hw_exception(TRAP_gp_fault, 0);
> > +                return 1;
> > +            }
> > +            else
> > +            {
> > +                core2_vpmu_cxt->pmu_enable->ds_area_enable = msr_content ? 1 : 0;
> 
> In the if() above you already exclude msr_content == 0, so why
> the conditional here? Or is it the other way around - the conditional
> here is correct, and the respective part of the if() clause above is
> wrong?
> 
> As I did already make a few other cleanup adjustments to your patch,
> please just let me know.

You are right. I'am always impressed how you guys looked at the code to see
such things ;-)
OK I think it's remained from my experiments with valid addresses. As in my
tests 0x0 can be used to write to the MSR_IA32_DS_AREA the check of
!msr_content in the if (... can be removed so it's possible to write 0x0 into
the msr.
But this worked only on boxes I was able to test ...
Thanks!

Dietmar.


> 
> Jan
> 
> > +                break;
> > +            }
> > +        }
> > +        else
> > +        {
> > +            gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
> > +            return 1;
> > +        }
> >      case MSR_CORE_PERF_GLOBAL_CTRL:
> >          global_ctrl = msr_content;
> >          for ( i = 0; i < core2_get_pmc_count(); i++ )

-- 
Company details: http://ts.fujitsu.com/imprint.html

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Jan Beulich]

>>> On 15.03.12 at 14:39, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote:
> Am Dienstag 13 März 2012, 14:22:45 schrieb Jan Beulich:
>> As I did already make a few other cleanup adjustments to your patch,
>> please just let me know.
> 
> You are right. I'am always impressed how you guys looked at the code to see
> such things ;-)
> OK I think it's remained from my experiments with valid addresses. As in my
> tests 0x0 can be used to write to the MSR_IA32_DS_AREA the check of
> !msr_content in the if (... can be removed so it's possible to write 0x0 
> into the msr.

Committed both patches, but please take a look since - as indicated - I
made a few adjustments.

Jan

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

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Dietmar Hahn]

Am Donnerstag 15 März 2012, 14:15:03 schrieb Jan Beulich:
> 
> Committed both patches, but please take a look since - as indicated - I
> made a few adjustments.

Thanks for the adjustments Jan, works very well!
Only a small fix for console logging beauty ;-) :

diff -r 1b68427875f7 xen/arch/x86/hvm/vmx/vpmu_core2.c
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Thu Mar 15 15:20:37 2012 +0000
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Fri Mar 16 09:40:16 2012 +0100
@@ -690,7 +690,7 @@ static int core2_vpmu_initialise(struct 
             printk("******************************************************\n");
             printk("** WARNING: Emulation of BTS Feature is switched on **\n");
             printk("** Using this processor feature in a virtualized    **\n");
-            printk("** environment is not 100%% safe.                   **\n");
+            printk("** environment is not 100%% safe.                    **\n");
             printk("** Setting the DS buffer address with wrong values  **\n");
             printk("** may lead to hypervisor hangs or crashes.         **\n");
             printk("** It is NOT recommended for production use!        **\n");

Dietmar.


> Jan
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

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

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Jan Beulich]

>>> On 16.03.12 at 09:58, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote:
> Am Donnerstag 15 März 2012, 14:15:03 schrieb Jan Beulich:
>> 
>> Committed both patches, but please take a look since - as indicated - I
>> made a few adjustments.
> 
> Thanks for the adjustments Jan, works very well!
> Only a small fix for console logging beauty ;-) :

Thanks. I took the liberty to add you S-o-b on this small a fix without
you explicit stating so.

Jan

> diff -r 1b68427875f7 xen/arch/x86/hvm/vmx/vpmu_core2.c
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Thu Mar 15 15:20:37 2012 +0000
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Fri Mar 16 09:40:16 2012 +0100
> @@ -690,7 +690,7 @@ static int core2_vpmu_initialise(struct 
>              
> printk("******************************************************\n");
>              printk("** WARNING: Emulation of BTS Feature is switched on 
> **\n");
>              printk("** Using this processor feature in a virtualized    
> **\n");
> -            printk("** environment is not 100%% safe.                   
> **\n");
> +            printk("** environment is not 100%% safe.                    
> **\n");
>              printk("** Setting the DS buffer address with wrong values  
> **\n");
>              printk("** may lead to hypervisor hangs or crashes.         
> **\n");
>              printk("** It is NOT recommended for production use!        
> **\n");
> 
> Dietmar.
> 
> 
>> Jan
>> 
> 
> -- 
> Company details: http://ts.fujitsu.com/imprint.html 



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

Re: [PATCH 2 of 2] V2 vpmu: Add the BTS extension

[Dietmar Hahn]

Am Freitag 16 März 2012, 09:16:27 schrieb Jan Beulich:
> >>> On 16.03.12 at 09:58, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote:
> > Am Donnerstag 15 März 2012, 14:15:03 schrieb Jan Beulich:
> >> 
> >> Committed both patches, but please take a look since - as indicated - I
> >> made a few adjustments.
> > 
> > Thanks for the adjustments Jan, works very well!
> > Only a small fix for console logging beauty ;-) :
> 
> Thanks. I took the liberty to add you S-o-b on this small a fix without
> you explicit stating so.

Yes, sure!

Dietmar.

> 
> Jan

-- 
Company details: http://ts.fujitsu.com/imprint.html

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