On Wed, 30 May 2007 10:00:33 -0400
Mathieu Desnoyers <[EMAIL PROTECTED]> wrote:

> Use the faster conditional calls for F00F bug handling in do_page_fault.
> 

I guess this means that CONDCALL will be enabled on pretty much all i386,
in which case making the whole feature Kconfigurable is starting to look
marginal.

Perhaps a better approach would have to made this change dependent upon
CONDCALL, rather than forcing it on.

> @@ -1084,6 +1085,7 @@
>        */
>       idt_descr.address = fix_to_virt(FIX_F00F_IDT);
>       load_idt(&idt_descr);
> +     BUG_ON(cond_call_arm("fix_f00f"));

It is generally poor C style to do

        assert(something_which_has_side_effects())

because people can legitimately expect to do

#define assert() /*nothing*/

for production code.

The kernel doesn't actually do the right thing here when CONFIG_BUG=n,
sadly.  But still, children might be watching, so the better and preferred
style is

        if (cond_call_arm("fix_f00f"))
                BUG();

>  }
>  #endif
>  
> Index: linux-2.6-lttng/arch/i386/mm/fault.c
> ===================================================================
> --- linux-2.6-lttng.orig/arch/i386/mm/fault.c 2007-05-29 11:05:48.000000000 
> -0400
> +++ linux-2.6-lttng/arch/i386/mm/fault.c      2007-05-29 11:13:16.000000000 
> -0400
> @@ -25,6 +25,7 @@
>  #include <linux/kprobes.h>
>  #include <linux/uaccess.h>
>  #include <linux/kdebug.h>
> +#include <linux/condcall.h>
>  
>  #include <asm/system.h>
>  #include <asm/desc.h>
> @@ -221,6 +222,25 @@
>  
>  fastcall void do_invalid_op(struct pt_regs *, unsigned long);
>  
> +#ifdef CONFIG_X86_F00F_BUG
> +/*
> + * Pentium F0 0F C7 C8 bug workaround.
> + */
> +static inline int do_f00f_workaround(struct pt_regs *regs,
> +             unsigned long address)
> +{
> +     unsigned long nr;
> +
> +     nr = (address - idt_descr.address) >> 3;
> +
> +     if (nr == 6) {
> +             do_invalid_op(regs, 0);
> +             return 1;
> +     }
> +     return 0;
> +}
> +#endif
> +
>  static inline pmd_t *vmalloc_sync_one(pgd_t *pgd, unsigned long address)
>  {
>       unsigned index = pgd_index(address);
> @@ -474,19 +494,8 @@
>       }
>  
>  #ifdef CONFIG_X86_F00F_BUG
> -     /*
> -      * Pentium F0 0F C7 C8 bug workaround.
> -      */
> -     if (boot_cpu_data.f00f_bug) {
> -             unsigned long nr;
> -             
> -             nr = (address - idt_descr.address) >> 3;
> -
> -             if (nr == 6) {
> -                     do_invalid_op(regs, 0);
> -                     return;
> -             }
> -     }
> +     if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
> +             return;

We do a cond_call() to an inlined function?  That's a bit weird, isn't it?

>  #endif
>  
>  no_context:
> Index: linux-2.6-lttng/arch/i386/Kconfig.cpu
> ===================================================================
> --- linux-2.6-lttng.orig/arch/i386/Kconfig.cpu        2007-05-29 
> 11:51:46.000000000 -0400
> +++ linux-2.6-lttng/arch/i386/Kconfig.cpu     2007-05-29 11:52:08.000000000 
> -0400
> @@ -275,6 +275,7 @@
>  config X86_F00F_BUG
>       bool
>       depends on M586MMX || M586TSC || M586 || M486 || M386
> +     select COND_CALL

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

Reply via email to