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

[Pu Wen <puwen@xxxxxxxx>]

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.

-- 
Regards,
Pu Wen

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