thr3ads.net - Xen devel - [PATCH 2 of 2] vpmu: Add the BTS extension [Feb 2012]

If this information is useful, please help other people find it:
Share via:

Dietmar Hahn

2012-Feb-13 13:01 UTC

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

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/vmx/vmx.c               |   12 +-
 xen/arch/x86/hvm/vmx/vpmu_core2.c        |  156 +++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vpmu_core2.h |    1 +
 xen/include/asm-x86/hvm/vpmu.h           |    3 +
 xen/include/asm-x86/msr-index.h          |    5 +
 5 files changed, 148 insertions(+), 29 deletions(-)



diff -r 6c666c08cf0d xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Mon Feb 13 12:36:09 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Mon Feb 13 13:51:51 2012 +0100
@@ -1823,6 +1823,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) )
@@ -1950,9 +1953,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 6c666c08cf0d xen/arch/x86/hvm/vmx/vpmu_core2.c
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c	Mon Feb 13 12:36:09 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c	Mon Feb 13 13:51:51 2012 +0100
@@ -401,7 +401,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 )
@@ -420,8 +444,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++ )
@@ -466,6 +508,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
@@ -491,6 +534,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)
@@ -520,25 +565,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;
 }
 
@@ -553,15 +608,24 @@ 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 ( msr_content )
+    {
+        if ( is_pmc_quirk )
+            handle_pmc_quirk(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);
+        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);
 
@@ -582,10 +646,48 @@ 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 void core2_vpmu_initialise(struct vcpu *v)
 {
+    struct vpmu_struct *vpmu = vcpu_vpmu(v);
+    u64 msr_content;
+    struct cpuinfo_x86 *c = &current_cpu_data;
+
+    /* 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");
+            }
+    }
+func_out:
     check_pmc_quirk();
 }
 
diff -r 6c666c08cf0d xen/include/asm-x86/hvm/vmx/vpmu_core2.h
--- a/xen/include/asm-x86/hvm/vmx/vpmu_core2.h	Mon Feb 13 12:36:09 2012 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vpmu_core2.h	Mon Feb 13 13:51:51 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 6c666c08cf0d xen/include/asm-x86/hvm/vpmu.h
--- a/xen/include/asm-x86/hvm/vpmu.h	Mon Feb 13 12:36:09 2012 +0100
+++ b/xen/include/asm-x86/hvm/vpmu.h	Mon Feb 13 13:51:51 2012 +0100
@@ -72,6 +72,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 6c666c08cf0d xen/include/asm-x86/msr-index.h
--- a/xen/include/asm-x86/msr-index.h	Mon Feb 13 12:36:09 2012 +0100
+++ b/xen/include/asm-x86/msr-index.h	Mon Feb 13 13:51:51 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

Jan Beulich

2012-Feb-14 11:51 UTC

head link

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

>>> On 13.02.12 at 14:01, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> @@ -401,7 +401,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;
Was the code to make BTINT work magically in place already? I can''t
spot anything to the effect in the patch...
> +
> +            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 )
> @@ -420,8 +444,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;
How do you manage to get away without storing the value the guest
attempted to write?

Jan
> +            }
> +        }
> +        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++ )

Dietmar Hahn

2012-Feb-14 12:59 UTC

head link

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

Am Dienstag 14 Februar 2012, 11:51:39 schrieb Jan
Beulich:> >>> On 13.02.12 at 14:01, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> > @@ -401,7 +401,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;
> 
> Was the code to make BTINT work magically in place already? I
can''t
> spot anything to the effect in the patch...
No, BTINT wasn''t handled before.
The writing of the MSR''s is done in the calling function
vmx_msr_write_intercept() in xen/arch/x86/hvm/vmx/vmx.c.
There I added the call of vpmu_do_wrmsr() in the case of MSR_IA32_DEBUGCTLMSR.
If vpmu_do_wrmsr() returns 1 the MSR gets written in the line
   __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);

Maybe I can change this and write the MSR here in this function.
> 
> > +
> > +            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 )
> > @@ -420,8 +444,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;
> 
> How do you manage to get away without storing the value the guest
> attempted to write?
In the case of MSR_IA32_DS_AREA the value is stored some lines later
core2_vpmu_save_msr_context(v, type, index, msr_content);
in an internal data structure.
The values of this structure are loaded - core2_vpmu_load() - and stored
- core2_vpmu_save() - on context switch.

Thanks.
Dietmar.
> 
> Jan
> 
> > +            }
> > +        }
> > +        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++ )
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> -- 
Company details: http://ts.fujitsu.com/imprint.html

Jan Beulich

2012-Feb-14 13:27 UTC

head link

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

>>> On 14.02.12 at 13:59, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> Am Dienstag 14 Februar 2012, 11:51:39 schrieb Jan Beulich:
>> >>> On 13.02.12 at 14:01, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
>> > @@ -401,7 +401,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;
>> 
>> Was the code to make BTINT work magically in place already? I
can''t
>> spot anything to the effect in the patch...
> 
> No, BTINT wasn''t handled before.
> The writing of the MSR''s is done in the calling function
> vmx_msr_write_intercept() in xen/arch/x86/hvm/vmx/vmx.c.
> There I added the call of vpmu_do_wrmsr() in the case of 
> MSR_IA32_DEBUGCTLMSR.
> If vpmu_do_wrmsr() returns 1 the MSR gets written in the line
>    __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
The question was more towards what happens if a guest enables this
bit without first setting up the corresponding LVT.

Plus enforcing the buffer requirements to avoid CPU deadlock
(contiguous present pages, alignment). Failure to do so can hang the
CPU, and hence would represent a DoS vulnerability.
> Maybe I can change this and write the MSR here in this function.
That might still be good to do, so checks and actual writing are in one
place.
>> 
>> > +
>> > +            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 )
>> > @@ -420,8 +444,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;
>> 
>> How do you manage to get away without storing the value the guest
>> attempted to write?
> 
> In the case of MSR_IA32_DS_AREA the value is stored some lines later
> core2_vpmu_save_msr_context(v, type, index, msr_content);
> in an internal data structure.
> The values of this structure are loaded - core2_vpmu_load() - and stored
> - core2_vpmu_save() - on context switch.
Okay, I must have missed that part then.

However, in the context of the above the simple is_canonical_address()
check here clearly isn''t enough.

Jan

Dietmar Hahn

2012-Feb-14 14:30 UTC

head link

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

Am Dienstag 14 Februar 2012, 13:27:08 schrieb Jan
Beulich:> >>> On 14.02.12 at 13:59, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> > Am Dienstag 14 Februar 2012, 11:51:39 schrieb Jan Beulich:
> >> >>> On 13.02.12 at 14:01, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> >> > @@ -401,7 +401,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;
> >> 
> >> Was the code to make BTINT work magically in place already? I
can''t
> >> spot anything to the effect in the patch...
> > 
> > No, BTINT wasn''t handled before.
> > The writing of the MSR''s is done in the calling function
> > vmx_msr_write_intercept() in xen/arch/x86/hvm/vmx/vmx.c.
> > There I added the call of vpmu_do_wrmsr() in the case of 
> > MSR_IA32_DEBUGCTLMSR.
> > If vpmu_do_wrmsr() returns 1 the MSR gets written in the line
> >    __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
> 
> The question was more towards what happens if a guest enables this
> bit without first setting up the corresponding LVT.
The apic is checked and set, see apic_write_around() in vpmu_core2.c.
> 
> Plus enforcing the buffer requirements to avoid CPU deadlock
> (contiguous present pages, alignment). Failure to do so can hang the
> CPU, and hence would represent a DoS vulnerability.
I''m not sure what you mean here. Are you speaking about the DS buffer?
If yes, this is no problem, because the DS buffer addressm must be a domU
virtual address. The processor only writes data into the buffer, if the
domU is running so in the worst case the domU gets triggered a page fault
or what I testet a triple fault occurs and the domU gets rebootet.
> 
> > Maybe I can change this and write the MSR here in this function.
> 
> That might still be good to do, so checks and actual writing are in one
> place.
After thinking about this. The writing should better be in
vmx_msr_write_intercept() because vpmu_do_wrmsr() in this case does only a
check of illegal set bits in the vpmu environment. In such a case a
TRAP_gp_fault is initiated otherwise nothing is done.
> 
> >> 
> >> > +
> >> > +            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 )
> >> > @@ -420,8 +444,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;
> >> 
> >> How do you manage to get away without storing the value the guest
> >> attempted to write?
> > 
> > In the case of MSR_IA32_DS_AREA the value is stored some lines later
> > core2_vpmu_save_msr_context(v, type, index, msr_content);
> > in an internal data structure.
> > The values of this structure are loaded - core2_vpmu_load() - and
stored
> > - core2_vpmu_save() - on context switch.
> 
> Okay, I must have missed that part then.
> 
> However, in the context of the above the simple is_canonical_address()
> check here clearly isn''t enough.
As described above, the access to this buffer is only done while running the
domU.

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

Jan Beulich

2012-Feb-14 14:50 UTC

head link

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

>>> On 14.02.12 at 15:30, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> Am Dienstag 14 Februar 2012, 13:27:08 schrieb Jan Beulich:
>> Plus enforcing the buffer requirements to avoid CPU deadlock
>> (contiguous present pages, alignment). Failure to do so can hang the
>> CPU, and hence would represent a DoS vulnerability.
> 
> I''m not sure what you mean here. Are you speaking about the DS
buffer?
> If yes, this is no problem, because the DS buffer addressm must be a domU
> virtual address. The processor only writes data into the buffer, if the
> domU is running so in the worst case the domU gets triggered a page fault
> or what I testet a triple fault occurs and the domU gets rebootet.
This certainly can be CPU model dependent, but on raw hardware
I know that not meeting the buffer constraints can hang (not triple
fault, perhaps a live lock) a CPU. Therefore, unless you can prove
this is impossible when running in VMX non-root, you will have to add
provisions for this (and this was the major reason keeping me from
trying to add DS support a year or two ago). At the very minimum
the whole functionality would otherwise need to be disabled by
default, and when enabled a prominent warning be issued (along
the lines of that of sync_console).

Jan

Dietmar Hahn

2012-Feb-15 10:18 UTC

head link

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

Am Dienstag 14 Februar 2012, 14:50:30 schrieb Jan
Beulich:> >>> On 14.02.12 at 15:30, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> > Am Dienstag 14 Februar 2012, 13:27:08 schrieb Jan Beulich:
> >> Plus enforcing the buffer requirements to avoid CPU deadlock
> >> (contiguous present pages, alignment). Failure to do so can hang
the
> >> CPU, and hence would represent a DoS vulnerability.
> > 
> > I''m not sure what you mean here. Are you speaking about the
DS buffer?
> > If yes, this is no problem, because the DS buffer addressm must be a
domU
> > virtual address. The processor only writes data into the buffer, if
the
> > domU is running so in the worst case the domU gets triggered a page
fault
> > or what I testet a triple fault occurs and the domU gets rebootet.
> 
> This certainly can be CPU model dependent, but on raw hardware
> I know that not meeting the buffer constraints can hang (not triple
> fault, perhaps a live lock) a CPU. Therefore, unless you can prove
> this is impossible when running in VMX non-root, you will have to add
> provisions for this (and this was the major reason keeping me from
> trying to add DS support a year or two ago). At the very minimum
> the whole functionality would otherwise need to be disabled by
> default, and when enabled a prominent warning be issued (along
> the lines of that of sync_console).
Of course I can''t prove anything.
While I experimented with this I couldn''t find any statement in the cpu
specs
about this buffer stuff and what the cpu does with wrong addresses. I found
that writing a non canonical address into the MSR_IA32_DS_AREA leads to a
general protection fault in the hypervisor. This was the cause for the check of
is_canonical_address(msr_content).
But with this check I tried different addresses, also wrong addresses within
the buffer and the hypervisor didn''t crash anymore but the domU always
failed
with the:
hvm.c:1141:d10 Triple fault on VCPU0 - invoking HVM system reset.
But of course there may be other critical pathes.

Currently we have the vpmu boot flag. So by default this is disabled.
The question is, should the BTS suff get an own flag or should we change the
vpmu flag to a string with multiple meanings? If the warning should be printed
only for the BTS stuff then an own BTS flag is needed.

Dietmar.


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

Jan Beulich

2012-Feb-15 10:29 UTC

head link

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

>>> On 15.02.12 at 11:18, Dietmar Hahn
<dietmar.hahn@ts.fujitsu.com> wrote:
> Currently we have the vpmu boot flag. So by default this is disabled.
> The question is, should the BTS suff get an own flag or should we change
the
> vpmu flag to a string with multiple meanings? If the warning should be 
> printed only for the BTS stuff then an own BTS flag is needed.
I''d recommend "vpmu=bts" to enable vPMU including BTS, while
the
normal boolean values (parse_bool()) would enable the "safe" parts
of vPMU code only.

Jan

Xen devel - Feb 2012 - [PATCH 2 of 2] vpmu: Add the BTS extension

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

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

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

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

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

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

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

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