Re: [Xen-devel] [PATCH v2 03/14] x86/cpu/vpmu: Add Hygon Dhyana and AMD Zen support for vPMU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.19 at 14:47, <puwen@xxxxxxxx> wrote:
> On 2019/3/19 20:58, Jan Beulich wrote:
>> On 19.03.19 at 12:32, <puwen@xxxxxxxx> wrote:
>>> On 2019/3/18 16:59, Jan Beulich wrote:
>>>> On 16.03.19 at 11:11, <puwen@xxxxxxxx> wrote:
>>>>> On 2019/3/15 20:41, Jan Beulich wrote:
>>>>>> On 21.02.19 at 10:50, <puwen@xxxxxxxx> wrote:
>>>>>>> --- a/xen/arch/x86/cpu/vpmu_amd.c
>>>>>>> +++ b/xen/arch/x86/cpu/vpmu_amd.c
>>>>>>> @@ -545,6 +545,8 @@ int __init amd_vpmu_init(void)
>>>>>>>         switch ( current_cpu_data.x86 )
>>>>>>>         {
>>>>>>>         case 0x15:
>>>>>>> +    case 0x17:
>>>>>>> +    case 0x18:
>>>>>>>             num_counters = F15H_NUM_COUNTERS;
>>>>>>>             counters = AMD_F15H_COUNTERS;
>>>>>>>             ctrls = AMD_F15H_CTRLS;
>>>>>>
>>>>>> Unless you know what AMD Fam18 will look like, you can't do it
>>>>>> like this. Fam18 really needs to be further qualified by a vendor
>>>>>> check at this point in time.
>>>>>
>>>>> Hygon will negotiate with AMD to make sure that only Hygon should use
>>>>> Fam18h.
>>>>
>>>> In the success case of which please state this in the description.
>>>> Until those negotiations have succeeded I'm afraid I'm going to
>>>> insist to see the extra check added.
>>>
>>> How to check vendor? Maybe like this:
>>>       case 0x15:
>>>       case 0x17:
>>>       case 0x18:
>>>           if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
>>>               boot_cpu_data.x86 == 0x18)
>>>               return -EINVAL;
>>>
>>>           num_counters = F15H_NUM_COUNTERS;
>>>           counters = AMD_F15H_COUNTERS;
>>>           ctrls = AMD_F15H_CTRLS;
>>>
>>> or just add Hygon support at beginning of amd_vpmu_init():
>>>       if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>>>           num_counters = F15H_NUM_COUNTERS;
>>>           counters = AMD_F15H_COUNTERS;
>>>           ctrls = AMD_F15H_CTRLS;
>>>           k7_counters_mirrored = 1;
>>>       }
>> 
>> A suitable variant of the latter or
>> 
>> int __init amd_vpmu_init(void)
>> {
>>      unsigned int i, fam = current_cpu_data.x86
>> 
>>      /* <suitable comment> */
>>      if ( current_cpu_data.x86_vendor == X86_VENDOR_HYGON && fam == 0x18 )
>>         fam = 0x17;
> 
> This is the minimum change, I think it's better.
> 
>> 
>>      switch ( fam )
>>      ...
>> 
>> or perhaps even better would be two separate switch()-es, one for
>> AMD and one for Hygon. Possibly even a separate hygon_vpmu_init().
> 
> A separate hygon_vpmu_init() is also fine except that the last part of
> the function can be shared.

So perhaps split that part out into a static _vpmu_init() or common_init()?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel