Re: [Xen-devel] [PATCH v2 1/9] x86/acpi: add a common interface for x86 cpu matching

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 09:49, <wei.w.wang@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -45,6 +45,45 @@ unsigned int paddr_bits __read_mostly = 36;
>   */
>  u64 host_pat = 0x050100070406;
>  
> +/*
> + * x86_match_cpu - match the current CPU against an array of
> + * x86_cpu_ids
> + * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
> + *         {}.
> + * Return the entry if the current CPU matches the entries in the
> + * passed x86_cpu_id match table. Otherwise NULL.  The match table
> + * contains vendor (X86_VENDOR_*), family, model and feature bits or
> + * respective wildcard entries.
> + *
> + * A typical table entry would be to match a specific CPU
> + * { X86_VENDOR_INTEL, 6, 0x12 }
> + * or to match a specific CPU feature
> + * { X86_FEATURE_MATCH(X86_FEATURE_FOOBAR) }
> + *
> + * This always matches against the boot cpu, assuming models and+features are
> + * consistent over all CPUs.
> + */
> +const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)

const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[])

> +{
> +    const struct x86_cpu_id *m;
> +    struct cpuinfo_x86 *c = &boot_cpu_data;

const

> +    for (m = match; m->vendor | m->family | m->model | m->feature; m++) {
> +        if (c->x86_vendor != m->vendor)
> +            continue;
> +        if (c->x86 != m->family)
> +            continue;
> +        if (c->x86_model != m->model)
> +            continue;
> +        if (!cpu_has(c, m->feature))
> +            continue;
> +        return m;
> +    }
> +    return NULL;
> +}

Please honor tab indentation used in the rest of this file.

> +EXPORT_SYMBOL(x86_match_cpu);

???

> static unsigned int __cpuinitdata cleared_caps[NCAPINTS];

I also dislike the function being placed between two data definitions;
I'd rather see this go (much) farther down in the file.

> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -59,6 +59,8 @@
>  #include <asm/hpet.h>
>  #include <asm/mwait.h>
>  #include <asm/msr.h>
> +#include <asm/processor.h>
> +#include <asm/cpufeature.h>

The former alone would be sufficient.

> @@ -656,12 +658,11 @@ static const struct idle_cpu idle_cpu_avn = {
>       .disable_promotion_to_c1e = 1,
>  };
>  
> -#define ICPU(model, cpu) { 6, model, &idle_cpu_##cpu }
> +#define ICPU(model, cpu) \
> +    { X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT, \
> +        (unsigned long)&idle_cpu_##cpu}
>  
> -static struct intel_idle_id {
> -     unsigned int family, model;
> -     const struct idle_cpu *data;
> -} intel_idle_ids[] __initdata = {
> +static const struct x86_cpu_id intel_idle_ids[] = {

__initconst

> @@ -722,23 +723,18 @@ static void __init mwait_idle_state_table_update(void)
>  static int __init mwait_idle_probe(void)
>  {
>       unsigned int eax, ebx, ecx;
> -     const struct intel_idle_id *id;
> +     const struct x86_cpu_id *id;
>  
> -     if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> -         !boot_cpu_has(X86_FEATURE_MWAIT) ||
> -         boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF)
> -             return -ENODEV;
> -
> -     for (id = intel_idle_ids; id->family; ++id)
> -             if (id->family == boot_cpu_data.x86 &&
> -                 id->model == boot_cpu_data.x86_model)
> -                     break;
> -     if (!id->family) {
> +        id = x86_match_cpu(intel_idle_ids);
> +        if (!id) {

Please don't corrupt indentation.

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -163,6 +163,14 @@ struct vcpu;
>      pc;                                             \
>  })
>  
> +struct x86_cpu_id {
> +    __u16 vendor;
> +    __u16 family;
> +    __u16 model;
> +    __u16 feature;   /* bit index */

In order of preference we'd like to use uintNN_t, uNN, and only
then __uNN (even if pre-existing code may violate this).

> +    __u64 driver_data;

void * please, avoiding casts.

> @@ -204,6 +212,8 @@ extern u32 cpuid_ext_features;
>  /* Maximum width of physical addresses supported by the hardware */
>  extern unsigned int paddr_bits;
>  
> +extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id 
> *match);

extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id table[]);

Jan


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