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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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