On 2019/3/27 0:10, Jan Beulich wrote: > On 25.03.19 at 14:30, <pu...@hygon.cn> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel