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

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

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