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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.02.12 at 13:59, Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx> wrote:
> Am Dienstag 14 Februar 2012, 11:51:39 schrieb Jan Beulich:
>> >>> On 13.02.12 at 14:01, Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel