Re: [PATCH] AMD Opteron Rev. E hack

2009/11/5 Giovanni Trematerra <giovanni.trematerra_at_gmail.com>:
> Hi,
> I have a quick and dirty patch to address the problem as discussed on
> commit r198868 in svn-src-head_at_
> I introduced BROKEN_OPTERON_E kernel option for i386/amd64 arch.
> The patch isn't tested yet, I only successfully compiled on i386.
> Can you let me know if the patch is on the right direction to resolve the issue?
> style(9) tips are welcomed.

> diff -r 75d35d8e7fe1 sys/amd64/amd64/identcpu.c
> --- a/sys/amd64/amd64/identcpu.c        Thu Nov 05 11:18:35 2009 +0100
> +++ b/sys/amd64/amd64/identcpu.c        Thu Nov 05 12:42:35 2009 +0100
> _at__at_ -404,6 +404,10 _at__at_
>
>         if (cpu_vendor_id == CPU_VENDOR_AMD)
>                 print_AMD_info();
> +#if defined(BROKEN_OPTERON_E)
> +       else
> +               printf("BROKEN_OPTERON_E option in your kernel is useless with y
> our CPU\n");
> +#endif
> }
>
>  void
> _at__at_ -620,10 +624,17 _at__at_
>          */
>         if (CPUID_TO_FAMILY(cpu_id) == 0xf && CPUID_TO_MODEL(cpu_id) >= 0x20 &&
>             CPUID_TO_MODEL(cpu_id) <= 0x3f) {
> +#if !defined(BROKEN_OPTERON_E)
>                 printf("WARNING: This architecture revision has known SMP "
>                     "hardware bugs which may cause random instability\n");
> -               printf("WARNING: For details see: "
> -                   "http://bugzilla.kernel.org/show_bug.cgi?id=11305\n");
> +#else
> +               printf("WARNING: options BROKEN_OPTERON_E is in your kernel. "
> +                               "Expect performance penalties\n");
> +       else
> +
> +               printf("WARNING: options BROKEN_OPTERON_E is useless with your C
> PU."
> +                               "Expect performance penalties\n");
> +#endif
>         }
>  }

I would leave the whole logic within print_AMD_info() and not pollute
external code and I would not print a string if the fix is in.

> diff -r 75d35d8e7fe1 sys/amd64/include/atomic.h
> --- a/sys/amd64/include/atomic.h        Thu Nov 05 11:18:35 2009 +0100
> +++ b/sys/amd64/include/atomic.h        Thu Nov 05 12:42:35 2009 +0100
> _at__at_ -36,6 +36,14 _at__at_
>  #define        wmb()   __asm __volatile("sfence;" : : : "memory")
>  #define        rmb()   __asm __volatile("lfence;" : : : "memory")
>
> +#include "opt_cpu.h"
> +
> +#if defined(BROKEN_OPTERON_E) && (defined(SMP) || !defined(_KERNEL))
> +       #define OPTERON_E_HACK() rmb()
> +#else
> +       #define OPTERON_E_HACK()
> +#endif
> +
> /*
>  * Various simple operations on memory, each of which is atomic in the
>  * presence of interrupts and multiple processors.
> _at__at_ -147,6 +155,8 _at__at_
>          "m" (*dst)                    /* 4 */
>         : "memory");
>
> +       OPTERON_E_HACK();
> +
>         return (res);
>  }

You need to override the whole barrier IMHO and not add this new stub.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein