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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.19 at 09:16, <puwen@xxxxxxxx> wrote:
> On 2019/3/27 0:10, Jan Beulich wrote:
>> On 25.03.19 at 14:30, <puwen@xxxxxxxx> wrote:
>>> -    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?

Well, personally I'd prefer to keep the switch(), as that what's
going to be needed once you introduce a second model, but I
won't insist.

Jan



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