Re: [Xen-devel] [PATCH v6 12/19] x86/VPMU: Add support for PMU register handling on PV guests

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/22/2014 10:50 AM, Jan Beulich wrote:
> > @@ -868,8 +869,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
> >           __clear_bit(X86_FEATURE_TOPOEXT % 32, &c);
> >           break;
> >   
> > +    case 0x0000000a: /* Architectural Performance Monitor Features (Intel) */
> > +        break;
> > +
> >       case 0x00000005: /* MONITOR/MWAIT */
> > -    case 0x0000000a: /* Architectural Performance Monitor Features */
> Is there actually anything wrong with leaving this as it was, i.e.
> clearing a, b, c, and d?

Since AMD's PMU-related CPUID is 0x80000001 (and is not used currently 
anyway as there is no do_cpuid op in AMD's vpmu) I think I'll just move 
vpmu_do_cpuid() into 0x0000000a case.
> > @@ -885,6 +888,8 @@ void pv_cpuid(struct cpu_user_regs *regs)
> >       }
> >   
> >    out:
> > +    vpmu_do_cpuid(regs->eax, &a, &b, &c, &d);
> This seems incomplete without passing in regs->ecx. And without at
> least a brief comment it also looks misplaced at the first glance.

vpmu_cpuid() doesn't use indexed CPUIDs (but I can see how ecx could 
have been added for consistency if I kept the call where it is now.)

> > @@ -2515,7 +2520,14 @@ static int emulate_privileged_op(struct cpu_user_regs 
> > *regs)
> >               if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK )
> >                   wrmsrl(regs->_ecx, msr_content);
> >               break;
> > -
> > +        case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1:
> > +        case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1:
> > +        case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
> > +        case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> > +        case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
> > +            if ( !vpmu_do_wrmsr(regs->ecx, msr_content) )
> > +                goto invalid;
> > +            break;
> Can you really handle both Intel and AMD ones as a group here,
> without consideration whose CPU you're actually running on? I
> think for forward compatibility you should be making the call only
> for Intel MSRs on Intel CPUs, and respectively for AMD.

The vendor-specific paths are taken in vpmu_do_wrmsr() (and rdmsr). Not 
sure if splitting this into two cases would be better but if you feel it 
adds to clarity I can do this.

-boris

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