Re: [Xen-devel] [PATCH v9 13/20] x86/VPMU: When handling MSR accesses, leave fault injection to callers

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/12/2014 08:45 AM, Jan Beulich wrote:
> > > > On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> > @@ -458,13 +458,13 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> > msr_content)
> >               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 ( !(msr_content & supported ) ||
> > +                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> >               {
> > -                if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> > -                    return 1;
> > -                gdprintk(XENLOG_WARNING, "Debug Store is not supported on this 
> > cpu\n");
> > -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > -                return 0;
> > +                gdprintk(XENLOG_WARNING,
> > +                         "Debug Store is not supported on this cpu\n");
> > +                return 1;
> The code here (and in particular the #GP injection) was wrong anyway;
> see the small series I sent earlier today. Please base your on top of that.
>
> > @@ -476,18 +476,17 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> > msr_content)
> >       {
> >       case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> >           core2_vpmu_cxt->global_status &= ~msr_content;
> > -        return 1;
> > +        return 0;
> >       case MSR_CORE_PERF_GLOBAL_STATUS:
> >           gdprintk(XENLOG_INFO, "Can not write readonly MSR: "
> >                    "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
> > -        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >           return 1;
> Suspicious: Most return values get flipped, but this one (and at least
> one more below) doesn't. Any such inconsistencies that are being
> corrected (assuming this is intentional) as you go should be spelled
> out in the description. And then the question of course is whether
> it's really necessary to flip the meaning of this and some similar SVM
> function's return values anyway.
The return value of 1 of vpmu_do_msr() will now indicate that the 
routine encountered a fault as opposed to indicating whether the MSR 
access was to a VPMU register.
I will make a comment to that effect.

> > @@ -541,45 +539,43 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> > msr_content)
> >           }
> >       }
> >   
> > -    if ( (global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0) )
> > -        vpmu_set(vpmu, VPMU_RUNNING);
> > -    else
> > -        vpmu_reset(vpmu, VPMU_RUNNING);
> The moving off this code is - again without at least a brief comment -
> also not obviously correct; at least to me it looks like this slightly
> alters behavior (perhaps towards the better, but anyway).
Right. Because we don't want to change VPMU state if a fault is 
encountered (which is possible with current code).
Come think of it, this is still not enough --- we may leave registers 
updated when fault happens. I should fix that too.
-boris

> > @@ -607,19 +603,14 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t 
> > *msr_content)
> >               rdmsrl(msr, *msr_content);
> >           }
> >       }
> > -    else
> > +    else if ( msr == MSR_IA32_MISC_ENABLE )
> >       {
> >           /* 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;
> Again a case where the return value stays the same even if in
> general it appears to get flipped in this function.
>
> > +        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> > +            *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
> >       }
> >   
> > -    return 1;
> > +    return 0;
> >   }
> Jan

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