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

[Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>]

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