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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.05.14 at 19:16, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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.

Iirc this wouldn't work, as the function as it is right now (without any
of your patches, and I don't think they remove that code) wants to
alter leaf 1.

>>> @@ -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.

The fear I'm having is that eventually there might be MSR index
clashes. Doing it properly now saves us from future headache.

Jan


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