Commit-ID: 48327dd572aaf9924c3dc4f8ad3189d506b11390
Gitweb: http://git.kernel.org/tip/48327dd572aaf9924c3dc4f8ad3189d506b11390
Author: Hans Rosenfeld <[email protected]>
AuthorDate: Wed, 16 Jun 2010 11:48:52 +0200
Committer: H. Peter Anvin <[email protected]>
CommitDate: Wed, 16 Jun 2010 17:28:04 -0700
x86, cpu: AMD errata checking framework
Errata are defined using the AMD_LEGACY_ERRATUM() or AMD_OSVW_ERRATUM()
macros. The latter is intended for newer errata that have an OSVW id
assigned, which it takes as first argument. Both take a variable number
of family-specific model-stepping ranges created by AMD_MODEL_RANGE().
Iff an erratum has an OSVW id, OSVW is available on the CPU, and the
OSVW id is known to the hardware, it is used to determine whether an
erratum is present. Otherwise, the model-stepping ranges are matched
against the boot CPU to find out whether the erratum applies.
For certain special errata, the code using this framework might have to
conduct further checks to make sure an erratum is really (not) present.
Signed-off-by: Hans Rosenfeld <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/include/asm/processor.h | 30 ++++++++++++++++++++++
arch/x86/kernel/cpu/amd.c | 51 ++++++++++++++++++++++++++++++++++++++
2 files changed, 81 insertions(+), 0 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 7e5c6a6..09fb3a1 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -1025,4 +1025,34 @@ unsigned long calc_aperfmperf_ratio(struct aperfmperf *old,
return ratio;
}
+/*
+ * AMD errata checking
+ */
+#ifdef CONFIG_CPU_SUP_AMD
+extern bool cpu_has_amd_erratum(const struct cpuinfo_x86 *, bool, ...);
+
+/*
+ * Errata are defined using the AMD_LEGACY_ERRATUM() or AMD_OSVW_ERRATUM()
+ * macros. The latter is intended for newer errata that have an OSVW id
+ * assigned, which it takes as first argument. Both take a variable number
+ * of family-specific model-stepping ranges created by AMD_MODEL_RANGE().
+ *
+ * Example:
+ *
+ * #define AMD_ERRATUM_319 \
+ * AMD_LEGACY_ERRATUM(AMD_MODEL_RANGE(0x10, 0x2, 0x1, 0x4, 0x2), \
+ * AMD_MODEL_RANGE(0x10, 0x8, 0x0, 0x8, 0x0), \
+ * AMD_MODEL_RANGE(0x10, 0x9, 0x0, 0x9, 0x0))
+ */
+
+#define AMD_LEGACY_ERRATUM(...) false, __VA_ARGS__, 0
+#define AMD_OSVW_ERRATUM(osvw_id, ...) true, osvw_id, __VA_ARGS__, 0
+#define AMD_MODEL_RANGE(f, m_start, s_start, m_end, s_end) \
+ ((f << 24) | (m_start << 16) | (s_start << 12) | (m_end << 4) | (s_end))
+#define AMD_MODEL_RANGE_FAMILY(range) (((range) >> 24) & 0xff)
+#define AMD_MODEL_RANGE_START(range) (((range) >> 12) & 0xfff)
+#define AMD_MODEL_RANGE_END(range) ((range) & 0xfff)
+
+#endif /* CONFIG_CPU_SUP_AMD */
+
#endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 12b9cff..07bdfe9 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -14,6 +14,8 @@
# include <asm/cacheflush.h>
#endif
+#include <stdarg.h>
+
#include "cpu.h"
#ifdef CONFIG_X86_32
@@ -609,3 +611,52 @@ static const struct cpu_dev __cpuinitconst amd_cpu_dev = {
};
cpu_dev_register(amd_cpu_dev);
+
+
+/*
+ * Check for the presence of an AMD erratum.
+ * Arguments are defined in processor.h for each known erratum.
+ */
+bool cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, bool osvw, ...)
+{
+ va_list ap;
+ u32 range;
+ u32 ms;
+
+ if (cpu->x86_vendor != X86_VENDOR_AMD)
+ return false;
+
+ va_start(ap, osvw);
+
+ if (osvw) {
+ u16 osvw_id = va_arg(ap, int);
+
+ if (cpu_has(cpu, X86_FEATURE_OSVW)) {
+ u64 osvw_len;
+ rdmsrl(MSR_AMD64_OSVW_ID_LENGTH, osvw_len);
+
+ if (osvw_id < osvw_len) {
+ u64 osvw_bits;
+ rdmsrl(MSR_AMD64_OSVW_STATUS + (osvw_id >> 6),
+ osvw_bits);
+
+ va_end(ap);
+ return osvw_bits & (1ULL << (osvw_id & 0x3f));
+ }
+ }
+ }
+
+ /* OSVW unavailable or ID unknown, match family-model-stepping range */
+ ms = (cpu->x86_model << 8) | cpu->x86_mask;
+ while ((range = va_arg(ap, int))) {
+ if ((cpu->x86 == AMD_MODEL_RANGE_FAMILY(range)) &&
+ (ms >= AMD_MODEL_RANGE_START(range)) &&
+ (ms <= AMD_MODEL_RANGE_END(range))) {
+ va_end(ap);
+ return true;
+ }
+ }
+
+ va_end(ap);
+ return false;
+}
Hi there,
This code has been excluded since it was added, because it doesn't
compile if CONFIG_CPU_SUP_AMD is not defined. I took a quick look over
it to see if it could quickly be fixed (which it can -- just don't
exclude the macro definitions and define a dummy version of
cpu_has_amd_erratum() which simply returns false), however, on looking
closer at the code I have to say it really is pretty broken, and as such
I would rather you refreshed the patch.
First of all, you're passing a boolean flag which only is used to tell
if there is a single integer immediately following it. This could just
as easily, and much more cleanly, be done by passing -1 in the case
there is no OSVW ID. However, a *much* bigger issue is the fact that
this will manifest a potentially very large memory structure into code
at every calling point, but it is not at all obvious to the programmer
that that will happen. As such, I'm going to insist that the individual
errata definitions move out of line into a memory structure -- using
more or less your existing macros you can simply make it an array of
type u32 -- and then have in your header file:
extern const u32 amd_erratum_400[];
... and at the point of call ...
cpu_has_amd_erratum(amd_erratum_400)
Note that I haven't included a pointer to the cpuinfo_x86 structure. The
reason why is that although you pass a pointer to an arbitrary
cpuinfo_x86 structure, you also do rdmsrl() on the local processor and
expect them to work. This isn't really ideal; it's better, then, to
make it specific to the currently executing processor and just use
current_cpu_data.
I have flushed these two commits, and am looking forward to an updated
version.
-hpa
On 06/17/2010 10:06 AM, tip-bot for Hans Rosenfeld wrote:
> Commit-ID: 48327dd572aaf9924c3dc4f8ad3189d506b11390
> Gitweb: http://git.kernel.org/tip/48327dd572aaf9924c3dc4f8ad3189d506b11390
> Author: Hans Rosenfeld <[email protected]>
> AuthorDate: Wed, 16 Jun 2010 11:48:52 +0200
> Committer: H. Peter Anvin <[email protected]>
> CommitDate: Wed, 16 Jun 2010 17:28:04 -0700
>
> x86, cpu: AMD errata checking framework
>
> Errata are defined using the AMD_LEGACY_ERRATUM() or AMD_OSVW_ERRATUM()
> macros. The latter is intended for newer errata that have an OSVW id
> assigned, which it takes as first argument. Both take a variable number
> of family-specific model-stepping ranges created by AMD_MODEL_RANGE().
>
> Iff an erratum has an OSVW id, OSVW is available on the CPU, and the
> OSVW id is known to the hardware, it is used to determine whether an
> erratum is present. Otherwise, the model-stepping ranges are matched
> against the boot CPU to find out whether the erratum applies.
>
> For certain special errata, the code using this framework might have to
> conduct further checks to make sure an erratum is really (not) present.
>
> Signed-off-by: Hans Rosenfeld <[email protected]>
> LKML-Reference: <[email protected]>
> Signed-off-by: H. Peter Anvin <[email protected]>
> ---
> arch/x86/include/asm/processor.h | 30 ++++++++++++++++++++++
> arch/x86/kernel/cpu/amd.c | 51 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 7e5c6a6..09fb3a1 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -1025,4 +1025,34 @@ unsigned long calc_aperfmperf_ratio(struct aperfmperf *old,
> return ratio;
> }
>
> +/*
> + * AMD errata checking
> + */
> +#ifdef CONFIG_CPU_SUP_AMD
> +extern bool cpu_has_amd_erratum(const struct cpuinfo_x86 *, bool, ...);
> +
> +/*
> + * Errata are defined using the AMD_LEGACY_ERRATUM() or AMD_OSVW_ERRATUM()
> + * macros. The latter is intended for newer errata that have an OSVW id
> + * assigned, which it takes as first argument. Both take a variable number
> + * of family-specific model-stepping ranges created by AMD_MODEL_RANGE().
> + *
> + * Example:
> + *
> + * #define AMD_ERRATUM_319 \
> + * AMD_LEGACY_ERRATUM(AMD_MODEL_RANGE(0x10, 0x2, 0x1, 0x4, 0x2), \
> + * AMD_MODEL_RANGE(0x10, 0x8, 0x0, 0x8, 0x0), \
> + * AMD_MODEL_RANGE(0x10, 0x9, 0x0, 0x9, 0x0))
> + */
> +
> +#define AMD_LEGACY_ERRATUM(...) false, __VA_ARGS__, 0
> +#define AMD_OSVW_ERRATUM(osvw_id, ...) true, osvw_id, __VA_ARGS__, 0
> +#define AMD_MODEL_RANGE(f, m_start, s_start, m_end, s_end) \
> + ((f << 24) | (m_start << 16) | (s_start << 12) | (m_end << 4) | (s_end))
> +#define AMD_MODEL_RANGE_FAMILY(range) (((range) >> 24) & 0xff)
> +#define AMD_MODEL_RANGE_START(range) (((range) >> 12) & 0xfff)
> +#define AMD_MODEL_RANGE_END(range) ((range) & 0xfff)
> +
> +#endif /* CONFIG_CPU_SUP_AMD */
> +
> #endif /* _ASM_X86_PROCESSOR_H */
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 12b9cff..07bdfe9 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -14,6 +14,8 @@
> # include <asm/cacheflush.h>
> #endif
>
> +#include <stdarg.h>
> +
> #include "cpu.h"
>
> #ifdef CONFIG_X86_32
> @@ -609,3 +611,52 @@ static const struct cpu_dev __cpuinitconst amd_cpu_dev = {
> };
>
> cpu_dev_register(amd_cpu_dev);
> +
> +
> +/*
> + * Check for the presence of an AMD erratum.
> + * Arguments are defined in processor.h for each known erratum.
> + */
> +bool cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, bool osvw, ...)
> +{
> + va_list ap;
> + u32 range;
> + u32 ms;
> +
> + if (cpu->x86_vendor != X86_VENDOR_AMD)
> + return false;
> +
> + va_start(ap, osvw);
> +
> + if (osvw) {
> + u16 osvw_id = va_arg(ap, int);
> +
> + if (cpu_has(cpu, X86_FEATURE_OSVW)) {
> + u64 osvw_len;
> + rdmsrl(MSR_AMD64_OSVW_ID_LENGTH, osvw_len);
> +
> + if (osvw_id < osvw_len) {
> + u64 osvw_bits;
> + rdmsrl(MSR_AMD64_OSVW_STATUS + (osvw_id >> 6),
> + osvw_bits);
> +
> + va_end(ap);
> + return osvw_bits & (1ULL << (osvw_id & 0x3f));
> + }
> + }
> + }
> +
> + /* OSVW unavailable or ID unknown, match family-model-stepping range */
> + ms = (cpu->x86_model << 8) | cpu->x86_mask;
> + while ((range = va_arg(ap, int))) {
> + if ((cpu->x86 == AMD_MODEL_RANGE_FAMILY(range)) &&
> + (ms >= AMD_MODEL_RANGE_START(range)) &&
> + (ms <= AMD_MODEL_RANGE_END(range))) {
> + va_end(ap);
> + return true;
> + }
> + }
> +
> + va_end(ap);
> + return false;
> +}
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
Hi,
On Wed, Jul 07, 2010 at 01:13:50PM -0400, H. Peter Anvin wrote:
> This code has been excluded since it was added, because it doesn't
> compile if CONFIG_CPU_SUP_AMD is not defined. I took a quick look over
> it to see if it could quickly be fixed (which it can -- just don't
> exclude the macro definitions and define a dummy version of
> cpu_has_amd_erratum() which simply returns false), however, on looking
> closer at the code I have to say it really is pretty broken, and as such
> I would rather you refreshed the patch.
Great that finally someone cared to take a closer look at it, after
only 4 weeks :)
I have looked into the issue and I think the main problem is not in
those patches, although either of the two patches could have fixed it
one way or another. Adding a dummy cpu_has_amd_erratum() would be one
way to do it, but I don't think it would be right.
I think that my patches uncovered a latent bug in the handling of AMD
erratum 400. The obviously correct fix is to completely wrap the
AMD-specific erratum 400 workaround in arch/x86/kernel/process.c in
#ifdef CONFIG_CPU_SUP_AMD. There is no reason why the code for an
AMD-specific workaround should be in the kernel if AMD support is
completely disabled. This makes me believe that while
CONFIG_CPU_SUP_AMD etc. exist, they are probably not used as widely as
they should be.
> First of all, you're passing a boolean flag which only is used to tell
> if there is a single integer immediately following it. This could just
> as easily, and much more cleanly, be done by passing -1 in the case
> there is no OSVW ID.
You are right in that this would be more concise, but is it really
"cleaner"? The bool makes it pretty obvious whats going on, without
introducing any ambiguity about what OSVW IDs are actually legal or
not. Also, I don't see how passing an extra bool would hurt anyone,
especially since this is not exactly code thats called very often.
> However, a *much* bigger issue is the fact that
> this will manifest a potentially very large memory structure into code
> at every calling point, but it is not at all obvious to the programmer
> that that will happen.
I don't really see a problem here, for basically two reasons.
First, it is very, very, very unlikely that there will ever be a
very large argument list for an erratum. Errata that never get fixed
in a family only have one model-stepping range covering the whole
family, while errata that get fixed eventually apply only to a couple
few model-stepping ranges. While working on this I spent a lot of time
studying the various AMD errata that could possibly make use of this
framework, and except for one all could be handled by three ranges or
less. The exception was a family 0xf erratum, which needed about 8
ranges because of the strange way the family 0xf models/steppings were
assigned.
Second, the cpu_has_amd_erratum() function is supposed to be called
only once for each erratum by initialization code. You should never
ever call it repeatedly in loops or interrupt handlers. If you need to
check for an erratum in such a place, cache the result in a local
variable. That would even be advisable without specifying the erratum
in the argument list.
The way I had implemented it would automatically make sure that the
definition of an erratum is embedded only in the code that checks for
it. An erratum that is never checked for (because the code is
disabled, or the module that contains it is not loaded) will never
need any space in the kernel.
> As such, I'm going to insist that the individual
> errata definitions move out of line into a memory structure -- using
> more or less your existing macros you can simply make it an array of
> type u32 -- and then have in your header file:
>
>
> extern const u32 amd_erratum_400[];
>
>
>
> ... and at the point of call ...
>
> cpu_has_amd_erratum(amd_erratum_400)
Thats exactly the kind of thing I wanted to avoid. This means carrying
around all errata definitions in memory all the time instead of just
when theres actually code thats using them.
It also requires more than one change to more than one file when
you're defining a new erratum. It was one of the design goals to avoid
just that.
Really, I don't see what this change would gain us, but it would
certainly make it harder to maintain.
> Note that I haven't included a pointer to the cpuinfo_x86 structure. The
> reason why is that although you pass a pointer to an arbitrary
> cpuinfo_x86 structure, you also do rdmsrl() on the local processor and
> expect them to work. This isn't really ideal; it's better, then, to
> make it specific to the currently executing processor and just use
> current_cpu_data.
Yes, that makes sense. I will rework that part.
Hans
--
%SYSTEM-F-ANARCHISM, The operating system has been overthrown
On 07/08/2010 07:18 AM, Hans Rosenfeld wrote:
> Hi,
>
> On Wed, Jul 07, 2010 at 01:13:50PM -0400, H. Peter Anvin wrote:
>> This code has been excluded since it was added, because it doesn't
>> compile if CONFIG_CPU_SUP_AMD is not defined. I took a quick look over
>> it to see if it could quickly be fixed (which it can -- just don't
>> exclude the macro definitions and define a dummy version of
>> cpu_has_amd_erratum() which simply returns false), however, on looking
>> closer at the code I have to say it really is pretty broken, and as such
>> I would rather you refreshed the patch.
>
> Great that finally someone cared to take a closer look at it, after
> only 4 weeks :)
>
> I have looked into the issue and I think the main problem is not in
> those patches, although either of the two patches could have fixed it
> one way or another. Adding a dummy cpu_has_amd_erratum() would be one
> way to do it, but I don't think it would be right.
>
It works well, and gcc will then remove the associated code.
>> However, a *much* bigger issue is the fact that
>> this will manifest a potentially very large memory structure into code
>> at every calling point, but it is not at all obvious to the programmer
>> that that will happen.
>
> I don't really see a problem here, for basically two reasons.
>
> First, it is very, very, very unlikely that there will ever be a
> very large argument list for an erratum. Errata that never get fixed
> in a family only have one model-stepping range covering the whole
> family, while errata that get fixed eventually apply only to a couple
> few model-stepping ranges. While working on this I spent a lot of time
> studying the various AMD errata that could possibly make use of this
> framework, and except for one all could be handled by three ranges or
> less. The exception was a family 0xf erratum, which needed about 8
> ranges because of the strange way the family 0xf models/steppings were
> assigned.
>
> Second, the cpu_has_amd_erratum() function is supposed to be called
> only once for each erratum by initialization code. You should never
> ever call it repeatedly in loops or interrupt handlers. If you need to
> check for an erratum in such a place, cache the result in a local
> variable. That would even be advisable without specifying the erratum
> in the argument list.
There is absolutely no reason to believe that that is actually the
case... and even if it was, it could get changed by gcc behind the
programmer's back. This assertion is insane.
> Really, I don't see what this change would gain us, but it would
> certainly make it harder to maintain.
Centralization and abstraction.
>> Note that I haven't included a pointer to the cpuinfo_x86 structure. The
>> reason why is that although you pass a pointer to an arbitrary
>> cpuinfo_x86 structure, you also do rdmsrl() on the local processor and
>> expect them to work. This isn't really ideal; it's better, then, to
>> make it specific to the currently executing processor and just use
>> current_cpu_data.
>
> Yes, that makes sense. I will rework that part.
Perhaps I wasn't making myself clear enough. If you submit the same
style of code again, I will veto it.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
Something else you might want to consider is exporting this information
in /proc/cpuinfo or similar.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On 07/08/2010 07:18 AM, Hans Rosenfeld wrote:
>
> Thats exactly the kind of thing I wanted to avoid. This means carrying
> around all errata definitions in memory all the time instead of just
> when theres actually code thats using them.
>
On a note on that... by passing the definition as arguments you're doing
it asas *code*, which is seriously bloated over a memory structure. The
size of memory structures should be insignificant, and would be cold in
memory anyway.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Thu, Jul 08, 2010 at 10:59:50AM -0400, H. Peter Anvin wrote:
> > I have looked into the issue and I think the main problem is not in
> > those patches, although either of the two patches could have fixed it
> > one way or another. Adding a dummy cpu_has_amd_erratum() would be one
> > way to do it, but I don't think it would be right.
> >
>
> It works well, and gcc will then remove the associated code.
Maybe it removes the code directly associated with the check, but the
rest of the stuff is still there. So when you build without AMD
support, you get whole lot of dead AMD-specific code. Well, I
personally don't care.
> > Second, the cpu_has_amd_erratum() function is supposed to be called
> > only once for each erratum by initialization code. You should never
> > ever call it repeatedly in loops or interrupt handlers. If you need to
> > check for an erratum in such a place, cache the result in a local
> > variable. That would even be advisable without specifying the erratum
> > in the argument list.
>
> There is absolutely no reason to believe that that is actually the
> case... and even if it was, it could get changed by gcc behind the
> programmer's back. This assertion is insane.
Maybe I'm getting something wrong here, but I highly doubt that gcc is
buggy enough to reorder function calls into loops or random unrelated
code. Also, having a function for a special purpose that requires
special handling is not such an uncommon thing, and documenting that
wouldn't pose much of a problem.
> > Really, I don't see what this change would gain us, but it would
> > certainly make it harder to maintain.
>
> Centralization and abstraction.
Yes, thats a generally a good thing, in source code at least. Thats
why I added the abstraction for errata definitions in _one_ central
place, not in two. Where it ends up in memory once it's been compiled
isn't really of interest there.
>From your other mail,
> On a note on that... by passing the definition as arguments you're doing
> it asas *code*, which is seriously bloated over a memory structure. The
> size of memory structures should be insignificant, and would be cold in
> memory anyway.
In case of the erratum 400 code (using 2 ranges), passing it as
arguments uses 4 bytes less. An erratum using 3 ranges will use 2
bytes more. While I don't think it matters at all, it is certainly not
"seriously bloated".
> Perhaps I wasn't making myself clear enough. If you submit the same
> style of code again, I will veto it.
That is probably the first convincing argument in this discussion.
Hans
--
%SYSTEM-F-ANARCHISM, The operating system has been overthrown