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

[Pu Wen <puwen@xxxxxxxx>]

On 2019/3/27 0:10, Jan Beulich wrote:
> On 25.03.19 at 14:30, <puwen@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/cpu/vpmu_amd.c
>> +++ b/xen/arch/x86/cpu/vpmu_amd.c
>> @@ -538,13 +538,37 @@ int svm_vpmu_initialise(struct vcpu *v)
>>       return 0;
>>   }
>>   
>> -int __init amd_vpmu_init(void)
>> +static int _vpmu_init(void)
> 
> Despite it having been me (I think) to have suggested this as
> a possible name, now that I see it in use I don't think it's a
> good choice: We're in vPMU code anyway, so the vpmu_
> prefix is pretty pointless. Simply init() would be too short and
> generic for my taste, so how about common_init() or
> shared_init()?

I prefer common_init() here.

>> -    for ( i = 0; i < num_counters; i++ )
>> +int __init hygon_vpmu_init(void)
>> +{
>> +    switch ( current_cpu_data.x86 )
>>       {
>> -        rdmsrl(ctrls[i], ctrl_rsvd[i]);
>> -        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
>> +    case 0x18:
>> +        num_counters = F15H_NUM_COUNTERS;
>> +        counters = AMD_F15H_COUNTERS;
>> +        ctrls = AMD_F15H_CTRLS;
>> +        k7_counters_mirrored = 1;
>> +        break;
>> +    default:
>> +        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
>> +               current_cpu_data.x86);
>> +        return -EINVAL;
>>       }
> 
> While I'm not going to insist in cases where you add to existing
> switch()-es which lack such blank lines, please add a blank line
> between the case blocks here. Yet then again I wonder whether
> the default case wouldn't better move into the shared function
> as well, keying off of e.g. num_counters still being zero.

I think it's a good idea to move the default case into the shared
function, which would like:
static int common_init(void)
{
     unsigned int i;

     if (!num_counters) {
         printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
                current_cpu_data.x86);
         return -EINVAL;
     }
...

Then as there is only one case in hygon_vpmu_init(), how about remove
switch()-es in this function?

-- 
Regards,
Pu Wen

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