[PATCH 1/1] Speedfreq-SMI call clobbers ECX - Kernel

This is a discussion on [PATCH 1/1] Speedfreq-SMI call clobbers ECX - Kernel ; Dear Dave, I have found that using SMI to change the cpu's frequency on my DELL Latitude L400 clobbers the ECX register in speedstep_set_state, causing unneccessary retries because the "state" variable has changed silently (GCC assumes it is still present ...

+ Reply to Thread
Results 1 to 13 of 13

Thread: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

  1. [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Dear Dave,
    I have found that using SMI to change the cpu's frequency on my DELL
    Latitude L400 clobbers the ECX register in speedstep_set_state, causing
    unneccessary retries because the "state" variable has changed silently (GCC
    assumes it is still present in ECX).

    The patch is simple: Introduce temporary output "clobber_ecx" operand that
    consumes the clobbered value.

    Cheers,
    Stephan

    Signed-off: Stephan Diestelhorst

    --

    --- linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c.orig 2008-02-26
    01:20:20.000000000 +0100
    +++ linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c 2008-03-05
    15:56:42.000000000 +0100
    @@ -160,7 +160,7 @@ static int speedstep_get_state (void)
    */
    static void speedstep_set_state (unsigned int state)
    {
    - unsigned int result = 0, command, new_state;
    + unsigned int result = 0, command, new_state, ecx_clobber;
    unsigned long flags;
    unsigned int function=SET_SPEEDSTEP_STATE;
    unsigned int retry = 0;
    @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
    __asm__ __volatile__(
    "movl $0, %%edi\n"
    "out %%al, (%%dx)\n"
    - : "=b" (new_state), "=D" (result)
    + : "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
    : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
    );
    } while ((new_state != state) && (retry <= SMI_TRIES));
    @@ -195,7 +195,7 @@ static void speedstep_set_state (unsigne
    if (new_state == state) {
    dprintk("change to %u MHz succeeded after %u tries with result %u\n",
    (speedstep_freqs[new_state].frequency / 1000), retry, result);
    } else {
    - printk(KERN_ERR "cpufreq: change failed with new_state %u and result %u\n",
    new_state, result);
    + printk(KERN_ERR "cpufreq: change to state %u failed with new_state %u and
    result %u\n", state, new_state, result);
    }

    return;
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX


    * Stephan Diestelhorst wrote:

    > @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
    > __asm__ __volatile__(
    > "movl $0, %%edi\n"
    > "out %%al, (%%dx)\n"
    > - : "=b" (new_state), "=D" (result)
    > + : "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
    > : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
    > );


    stupid suggestion: why not do a pusha/popa around those instructions, to
    make sure everything is restored? This isnt a fastpath and being
    conservative about SMI side-effects cannot hurt ...

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Ingo Molnar wrote:
    > * Stephan Diestelhorst wrote:
    >
    >> @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
    >> __asm__ __volatile__(
    >> "movl $0, %%edi\n"
    >> "out %%al, (%%dx)\n"
    >> - : "=b" (new_state), "=D" (result)
    >> + : "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
    >> : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
    >> );

    >
    > stupid suggestion: why not do a pusha/popa around those instructions, to
    > make sure everything is restored? This isnt a fastpath and being
    > conservative about SMI side-effects cannot hurt ...
    >


    You can't pusha/popa if you expect a result. You can, of course, push
    and pop individual registers.

    It's also kind of odd to do "movl $0,%%edi" instead of just setting EDI
    as an input.

    -hpa
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    On, March 5th 2008 16:35:20 Ingo Molnar wrote:
    > * Stephan Diestelhorst wrote:
    > > @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
    > > __asm__ __volatile__(
    > > "movl $0, %%edi\n"
    > > "out %%al, (%%dx)\n"
    > > - : "=b" (new_state), "=D" (result)
    > > + : "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
    > >
    > > : "a" (command), "b" (function), "c" (state), "d" (smi_port),
    > > : "S" (0)
    > >
    > > );

    >
    > stupid suggestion: why not do a pusha/popa around those
    > instructions, to make sure everything is restored? This isnt a
    > fastpath and being conservative about SMI side-effects cannot hurt


    That sounds like a sane thing to do to me. Should I provide a 'patch'?
    Or leave that (and the decision about it) to the maintainer?

    Regards,
    Stephan
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    > On, March 5th 2008 16:35:20 Ingo Molnar wrote:
    > > * Stephan Diestelhorst wrote:
    > > > @@ -184,7 +184,7 @@ static void speedstep_set_state (unsigne
    > > > __asm__ __volatile__(
    > > > "movl $0, %%edi\n"
    > > > "out %%al, (%%dx)\n"
    > > > - : "=b" (new_state), "=D" (result)
    > > > + : "=b" (new_state), "=D" (result), "=c" (ecx_clobber)
    > > >
    > > > : "a" (command), "b" (function), "c" (state), "d"
    > > > : (smi_port), "S" (0)
    > > >
    > > > );

    > >
    > > stupid suggestion: why not do a pusha/popa around those
    > > instructions, to make sure everything is restored? This isnt a
    > > fastpath and being conservative about SMI side-effects cannot
    > > hurt


    Stephan Diestelhorst wrote:
    > That sounds like a sane thing to do to me. Should I provide a
    > 'patch'? Or leave that (and the decision about it) to the
    > maintainer?


    H. Peter Anvin wrote:
    > You can't pusha/popa if you expect a result. You can, of course,
    > push and pop individual registers.
    >
    > It's also kind of odd to do "movl $0,%%edi" instead of just setting
    > EDI as an input.


    Whoops, HPA is correct, of course. Manually pushing / popping the
    registers is ugly, how about a larger clobber-list? Let the compiler
    figure out what it wants to save/restore. Only thing to worry about
    is EBP then.

    Again, should I provide these patches? This thing just annoyed me for
    a while as I have been patching it in my personal kernels for too
    long.

    Regards,
    Stephan

    PS: I'm not on LKML, please CC me at your discretion.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX


    * Stephan Diestelhorst wrote:

    > Again, should I provide these patches? This thing just annoyed me for
    > a while as I have been patching it in my personal kernels for too
    > long.


    yes, please do keep sending them (and any other patches you might have)
    - it's a real issue on real hardware so we want this fix upstream.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Ingo Molnar wrote:
    > > Again, should I provide these patches? This thing just annoyed me for
    > > a while as I have been patching it in my personal kernels for too
    > > long.

    >
    > yes, please do keep sending them (and any other patches you might have)
    > - it's a real issue on real hardware so we want this fix upstream.


    New attempt with full clobbers, note that I deliberatly did not change
    the order of the output registers. Real output operands still preceede
    outputs used for potential clobbering.

    I'm not too sure about the EBP push/pop frame, but as folks pointed
    out already, we should not trust the SMI code too much.

    Regards,
    Stephan

    --
    Signed-off by:

    --- linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c.orig 2008-02-26 01:20:20.000000000 +0100
    +++ linux-2.6.24.3/arch/x86/kernel/cpu/cpufreq/speedstep-smi.c 2008-03-10 16:02:51.000000000 +0100
    @@ -63,7 +63,7 @@ static struct cpufreq_frequency_table sp
    */
    static int speedstep_smi_ownership (void)
    {
    - u32 command, result, magic;
    + u32 command, result, magic, dummy;
    u32 function = GET_SPEEDSTEP_OWNER;
    unsigned char magic_data[] = "Copyright (c) 1999 Intel Corporation";

    @@ -73,8 +73,11 @@ static int speedstep_smi_ownership (void
    dprintk("trying to obtain ownership with command %x at port %x\n", command, smi_port);

    __asm__ __volatile__(
    + "push %%ebp\n"
    "out %%al, (%%dx)\n"
    - : "=D" (result)
    + "pop %%ebp\n"
    + : "=D" (result), "=a" (dummy), "=b" (dummy),"=c" (dummy),"=d" (dummy),
    + "=S" (dummy)
    : "a" (command), "b" (function), "c" (0), "d" (smi_port),
    "D" (0), "S" (magic)
    : "memory"
    @@ -96,7 +99,7 @@ static int speedstep_smi_ownership (void
    */
    static int speedstep_smi_get_freqs (unsigned int *low, unsigned int *high)
    {
    - u32 command, result = 0, edi, high_mhz, low_mhz;
    + u32 command, result = 0, edi, high_mhz, low_mhz, dummy;
    u32 state=0;
    u32 function = GET_SPEEDSTEP_FREQS;

    @@ -109,10 +112,12 @@ static int speedstep_smi_get_freqs (unsi

    dprintk("trying to determine frequencies with command %x at port %x\n", command, smi_port);

    - __asm__ __volatile__("movl $0, %%edi\n"
    + __asm__ __volatile__(
    + "push %%ebp\n"
    "out %%al, (%%dx)\n"
    - : "=a" (result), "=b" (high_mhz), "=c" (low_mhz), "=d" (state), "=D" (edi)
    - : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
    + "pop %%ebp"
    + : "=a" (result), "=b" (high_mhz), "=c" (low_mhz), "=d" (state), "=D" (edi), "=S" (dummy)
    + : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0), "D" (0)
    );

    dprintk("result %x, low_freq %u, high_freq %u\n", result, low_mhz, high_mhz);
    @@ -135,16 +140,18 @@ static int speedstep_smi_get_freqs (unsi
    static int speedstep_get_state (void)
    {
    u32 function=GET_SPEEDSTEP_STATE;
    - u32 result, state, edi, command;
    + u32 result, state, edi, command, dummy;

    command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);

    dprintk("trying to determine current setting with command %x at port %x\n", command, smi_port);

    - __asm__ __volatile__("movl $0, %%edi\n"
    + __asm__ __volatile__(
    + "push %%ebp\n"
    "out %%al, (%%dx)\n"
    - : "=a" (result), "=b" (state), "=D" (edi)
    - : "a" (command), "b" (function), "c" (0), "d" (smi_port), "S" (0)
    + "pop %%ebp\n"
    + : "=a" (result), "=b" (state), "=D" (edi), "=c" (dummy), "=d" (dummy), "=S" (dummy)
    + : "a" (command), "b" (function), "c" (0), "d" (smi_port), "S" (0), "D" (0)
    );

    dprintk("state is %x, result is %x\n", state, result);
    @@ -160,7 +167,7 @@ static int speedstep_get_state (void)
    */
    static void speedstep_set_state (unsigned int state)
    {
    - unsigned int result = 0, command, new_state;
    + unsigned int result = 0, command, new_state, dummy;
    unsigned long flags;
    unsigned int function=SET_SPEEDSTEP_STATE;
    unsigned int retry = 0;
    @@ -182,10 +189,12 @@ static void speedstep_set_state (unsigne
    }
    retry++;
    __asm__ __volatile__(
    - "movl $0, %%edi\n"
    + "push %%ebp\n"
    "out %%al, (%%dx)\n"
    - : "=b" (new_state), "=D" (result)
    - : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0)
    + "pop %%ebp"
    + : "=b" (new_state), "=D" (result), "=c" (dummy), "=a" (dummy),
    + "=d" (dummy), "=S" (dummy)
    + : "a" (command), "b" (function), "c" (state), "d" (smi_port), "S" (0), "D" (0)
    );
    } while ((new_state != state) && (retry <= SMI_TRIES));

    @@ -195,7 +204,7 @@ static void speedstep_set_state (unsigne
    if (new_state == state) {
    dprintk("change to %u MHz succeeded after %u tries with result %u\n", (speedstep_freqs[new_state].frequency / 1000), retry, result);
    } else {
    - printk(KERN_ERR "cpufreq: change failed with new_state %u and result %u\n", new_state, result);
    + printk(KERN_ERR "cpufreq: change to state %u failed with new_state %u and result %u\n", state, new_state, result);
    }

    return;
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Stephan Diestelhorst writes:
    >
    > New attempt with full clobbers, note that I deliberatly did not change
    > the order of the output registers. Real output operands still preceede
    > outputs used for potential clobbering.
    >
    > I'm not too sure about the EBP push/pop frame, but as folks pointed
    > out already, we should not trust the SMI code too much.


    Be careful -- older gcc versions tend to abort for inline asm
    that clobbers too many registers. Especially when the register
    is already used (like ebp in a frame pointer enabled kernel)

    Make sure it at least works on the oldest supported gcc version
    (gcc 3.2) and with frame pointer on.

    For asms with so many clobbers explicit push/pop is usually safer.

    -Andi
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Andi Kleen wrote:
    > Stephan Diestelhorst writes:
    > >
    > > New attempt with full clobbers, note that I deliberatly did not change
    > > the order of the output registers. Real output operands still preceede
    > > outputs used for potential clobbering.
    > >
    > > I'm not too sure about the EBP push/pop frame, but as folks pointed
    > > out already, we should not trust the SMI code too much.

    >
    > Be careful -- older gcc versions tend to abort for inline asm
    > that clobbers too many registers. Especially when the register
    > is already used (like ebp in a frame pointer enabled kernel)


    AFAIK clobbering ebp is silently ignored on the GCCs I've tried it
    on (regardles of frame-pointer ommission). Hence there is a hard-coded
    push & pop for that register.

    Please also note that these are not clobbers in the strict inline asm
    syntax, but rather dummy output values that correspond to actual
    input parameters. I'd consider this a serious compiler flaw, if not
    bug, if these would not work. But let's not get into GCC vs.
    Fancy-inline-asm-hacker flames, like the mplayer folks do ;-)

    > Make sure it at least works on the oldest supported gcc version
    > (gcc 3.2) and with frame pointer on.


    As I've said, I do not expect this to be problematic, but will test,
    just to be sure!

    > For asms with so many clobbers explicit push/pop is usually safer.


    Yes, but it is unneccessary overhead as the compiler can (should!)
    find more elegant ways to get those registers back if needed.

    Regards,
    Stephan
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    > Please also note that these are not clobbers in the strict inline asm
    > syntax, but rather dummy output values that correspond to actual


    Outputs/Inputs are as bad as clobbers in triggering gcc reload ICEs.

    > input parameters. I'd consider this a serious compiler flaw, if not
    > bug, if these would not work. But let's not get into GCC vs.


    It is, and it's (mostly) fixed in newer versions, but we're stuck with
    supporting the older compiler versions unfortunately.

    > > For asms with so many clobbers explicit push/pop is usually safer.

    >
    > Yes, but it is unneccessary overhead as the compiler can (should!)
    > find more elegant ways to get those registers back if needed.


    This code is not performance critical at all. Safer beats faster
    any time in such cases.

    -Andi

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Stephan Diestelhorst wrote:
    > Andi Kleen wrote:
    > > Stephan Diestelhorst writes:
    > > >
    > > > New attempt with full clobbers, note that I deliberatly did not change
    > > > the order of the output registers. Real output operands still preceede
    > > > outputs used for potential clobbering.
    > > >
    > > > I'm not too sure about the EBP push/pop frame, but as folks pointed
    > > > out already, we should not trust the SMI code too much.

    > >
    > > Be careful -- older gcc versions tend to abort for inline asm
    > > that clobbers too many registers. Especially when the register
    > > is already used (like ebp in a frame pointer enabled kernel)

    >
    > > Make sure it at least works on the oldest supported gcc version
    > > (gcc 3.2) and with frame pointer on.

    >
    > As I've said, I do not expect this to be problematic, but will test,
    > just to be sure!


    I've tried it on the following GCCs: 3.3, 3.4, 4.0, 4.1, all with and
    without frame-pointer ommission.
    Result: As expected. Worked w/o problems, warnings anything.

    Apologies for not testing gcc-3.2, but compiling it from source did
    not work with libtool complaining about tags in libmath. I'd be
    grateful if someone with working gcc-3.2 could try this out.

    Cheers,
    Stephan
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX


    * Stephan Diestelhorst wrote:

    > Please also note that these are not clobbers in the strict inline asm
    > syntax, but rather dummy output values that correspond to actual input
    > parameters. I'd consider this a serious compiler flaw, if not bug, if
    > these would not work. But let's not get into GCC vs.
    > Fancy-inline-asm-hacker flames, like the mplayer folks do ;-)


    yep, that's my experience as well - output constraints are pretty robust
    in general, but if one tries the same effect via the clobber list it's
    easy to run into gcc internal errors. I've applied your patch to
    x86.git. Do you think it's 2.6.25 material? I'm inclined to have it in
    the 2.6.26 bucket. We could do your first, minimal fix in 2.6.25 and do
    this second, full fix in 2.6.26.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: [PATCH 1/1] Speedfreq-SMI call clobbers ECX

    Ingo Molnar wrote:
    >
    > I've applied your patch to x86.git.


    Many thanks!

    > Do you think it's 2.6.25 material? I'm inclined to have it in
    > the 2.6.26 bucket. We could do your first, minimal fix in 2.6.25 and do
    > this second, full fix in 2.6.26.


    I am not familiar with the policies regarding the time-frame for
    inclusion. So I guess the decision is up to you. I feel that the
    changes are fairly small (at least for the first patch) so perhaps
    splitting the release as you suggested allows people to get more
    confidence in the larger patches with the clobbers and manual ebp
    push/pop.

    Doing this one step after the other has the additional benefit of
    havinga fix for the core problem in fairly soon, so I can continue
    using prebuilt distro kernels again, soon. :-) ( I know. I'm just lazy
    sometimes. )

    Apart from that: Feel free to do as you like, after all this code is
    GPL.

    Kind regards,
    Stephan
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread