Re: [Xen-devel] [PATCH v24 08/15] x86/VPMU: When handling MSR accesses, leave fault injection to callers

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/15/2015 11:06 AM, Jan Beulich wrote:
> > > > On 10.06.15 at 17:04, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> > @@ -454,36 +454,41 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> > msr_content,
> >                                IA32_DEBUGCTLMSR_BTS_OFF_USR;
> >               if ( !(msr_content & ~supported) &&
> >                    vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> > -                return 1;
> > +                return 0;
> >               if ( (msr_content & supported) &&
> >                    !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> >                   printk(XENLOG_G_WARNING
> >                          "%pv: Debug Store unsupported on this CPU\n",
> >                          current);
> >           }
> > -        return 0;
> > +        return -EINVAL;
> >       }
> >   
> >       ASSERT(!supported);
> >   
> > +    if ( type == MSR_TYPE_COUNTER &&
> > +         (msr_content &
> > +          ~((1ull << core2_get_bitwidth_fix_count()) - 1)) )
> > +        /* Writing unsupported bits to a fixed counter */
> > +        return -EINVAL;
> > +
> >       core2_vpmu_cxt = vpmu->context;
> >       enabled_cntrs = vpmu->priv_context;
> >       switch ( msr )
> >       {
> >       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;
> > +        return -EINVAL;
> Is it intentional that you convert a success to a failure here? If so,
> this should be mentioned (with reason) in the commit message. If
> not, this should be adjusted to there's no behavioral change here.
Yes, this is intentional. Until now return value indicated whether 
access was to a PMU register. This worked for HVM guests since they can 
do hvm_inject_trap() at any time. For PV guests we are called from 
emulate_privileged_op() and we need to know whether access was 
successful or not. This way emulate_privileged_op() will take care of 
fault injection by returning 0.
-boris



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