lkml.org 
[lkml]   [2009]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [git-pull -tip] x86: cpu_debug patches

    * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:

    > On Wed, 2009-04-29 at 12:50 +0200, Ingo Molnar wrote:
    > > * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
    > >
    > > > On Tue, 2009-04-28 at 19:28 +0200, Ingo Molnar wrote:
    > > > > * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
    > > > >
    > > > > > @@ -850,10 +903,10 @@ static int cpu_init_cpu(void)
    > > > > > cpui = &cpu_data(cpu);
    > > > > > if (!cpu_has(cpui, X86_FEATURE_MSR))
    > > > > > continue;
    > > > > > - per_cpu(cpu_model, cpu) = ((cpui->x86_vendor << 16) |
    > > > > > - (cpui->x86 << 8) |
    > > > > > - (cpui->x86_model));
    > > > > > - per_cpu(cpu_modelflag, cpu) = get_cpu_modelflag(cpu);
    > > > > > + per_cpu(cpu_modelflag, cpu) = get_cpu_flag(cpui);
    > > > > > + if (!per_cpu(cpu_modelflag, cpu))
    > > > > > + send_report(per_cpu(cpu_priv_count, cpu), cpui);
    > > > >
    > > > > This means that if the CPU is not enumerated in the model table
    > > > > explicitly, we'll fall back to some really minimal output, right?
    > > > >
    > > >
    > > > Yes.
    > >
    > > That's a bug really: it means that for every new CPU type that comes
    > > around we need to update this code. I.e. precisely for those CPUs
    > > where we might need the most help from such a debug facility, we
    > > wont have much info to look at ... New CPUs generally support all
    > > the CPU features that are displayed here, in a compatible manner.
    > >
    > > So that needs to be improved/changed to not be tied to such a static
    > > 'cpu model' enumeration but instead be CPU feature flags driven. See
    > > all the existing cpu_has_*() tests we have.
    > >
    >
    > cpu_has_*() is based on boot_cpu. So you mean cpu_has tests. right ?

    Correct.

    > We can use cpu_has tests for unknown processors but 'cpu model' is
    > accurate and cover all range.

    So is cpu_has. If it's not accurate then various other pieces of x86
    code might break as well.

    > cpu_has does not cover following registers:
    > 1. platform
    > 2. poweron
    > 3. control
    > 4. bios
    > 5. freq
    > 6. cache
    > 7. misc
    > 8. base
    > 9. ver
    > 10. conf
    >
    > So is this looks OK to you:
    >
    > 1. first check for 'cpu model' if CPU not supported then goto 2
    > 2. check for cpu_has tests

    No, please get rid of the 'cpu model' based feature tests altogether
    and extend the _existing_ feature flags with the above details, when
    needed.

    That way we reuse the existing cpu_has checks and extend them.
    Instead of implementing a private facility in cpu_debug.c.

    Ingo


    \
     
     \ /
      Last update: 2009-04-29 14:33    [W:0.044 / U:0.008 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site