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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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()?

> @@ -565,24 +589,25 @@ int __init amd_vpmu_init(void)
>          return -EINVAL;
>      }
>  
> -    if ( sizeof(struct xen_pmu_data) +
> -         2 * sizeof(uint64_t) * num_counters > PAGE_SIZE )
> -    {
> -        printk(XENLOG_WARNING
> -               "VPMU: Register bank does not fit into VPMU shared page\n");
> -        counters = ctrls = NULL;
> -        num_counters = 0;
> -        return -ENOSPC;
> -    }
> +    return _vpmu_init();
> +}
>  
> -    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.

Jan



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