Re: [Xen-devel] [PATCH v5 2/9] x86/intel_pstate: APERF/MPERF feature detect

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.15 at 04:32, <wei.w.wang@xxxxxxxxx> wrote:
>  changes in v5:
>  1) define macros for 0x1 and CPUID leaf5;
>  2) add a statement stating that this patch is independent of the
>     previous ones.

This statement doesn't belong in the commit message, i.e. should go
after the first ---.

> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -50,7 +50,6 @@ enum {
>  };
>  
>  #define INTEL_MSR_RANGE         (0xffffull)
> -#define CPUID_6_ECX_APERFMPERF_CAPABILITY       (0x1)
>  
>  struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
>  
> @@ -351,10 +350,9 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>  static void feature_detect(void *info)
>  {
>      struct cpufreq_policy *policy = info;
> -    unsigned int eax, ecx;
> +    unsigned int eax;
>  
> -    ecx = cpuid_ecx(6);
> -    if (ecx & CPUID_6_ECX_APERFMPERF_CAPABILITY) {
> +    if ( cpu_has_aperfmperf ) {

You should have fully fixed coding style here.

> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -239,6 +239,10 @@ static void __cpuinit generic_identify(struct 
> cpuinfo_x86 *c)
>       if ( cpu_has(c, X86_FEATURE_CLFLSH) )
>               c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
>  
> +     if ( (c->cpuid_level > CPUID_PM_LEAF) &&
> +             (cpuid_ecx(CPUID_PM_LEAF) & CPUID6_ECX_APERFMPERF_CAPABILITY) )

Indentation.

> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -69,6 +69,7 @@
>  #define X86_FEATURE_XTOPOLOGY    (3*32+13) /* cpu topology enum extensions */
>  #define X86_FEATURE_CPUID_FAULTING (3*32+14) /* cpuid faulting */
>  #define X86_FEATURE_CLFLUSH_MONITOR (3*32+15) /* clflush reqd with monitor */
> +#define X86_FEATURE_APERFMPERF (3*32+28) /* APERFMPERF */

Why 28 instead of the consecutive 16?

> @@ -190,6 +194,7 @@
>  #define cpu_has_page1gb              boot_cpu_has(X86_FEATURE_PAGE1GB)
>  #define cpu_has_efer         1
>  #define cpu_has_fsgsbase     boot_cpu_has(X86_FEATURE_FSGSBASE)
> +#define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)

Should use tab for aligning the right side to match the lines right
above.

I've taken care of all of these for you, but I would really
appreciate you taking care of such going forward.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel