ERRATUM: 22978, 23154
PASS: 1.0/1.1
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/arm64/arm64/gic_v3.c | ||
|---|---|---|
| 234–244 | Quoting https://lkml.org/lkml/2015/7/6/247 If you need an instruction sequence to be consecutive, then it _must_ be one single asm(). I would also like a comment explaining why all the nops are needed, and on what hardware the errata applies to. There have been a few places with other architectures where they needed a collection of nops, but nobody can remember why, or knew if they are still needed. | |
| sys/arm64/arm64/gic_v3.c | ||
|---|---|---|
| 234–244 | Fixed, comments added. Unfortunately, these are the only information than can go public. | |
| sys/arm64/arm64/gic_v3.c | ||
|---|---|---|
| 234–244 | Is it possible to expand on the meaning of Pass 1.0/1.1? | |
Comment Actions
What is Pass 1.0 and Pass 1.1? Someone reading this code that doesn't know the ThunderX would have no idea.
I think it would also be useful to have a kernel config to toggle this, I'm guessing later ThunderX hardware will be fixed, and non-ThunderX SoCs won't need this. If someone is building a custom kernel for their hardware they may wish to disable unneeded workarounds.
Comment Actions
It now says Chip revision: Pass 1.0, Pass 1.1 which I think is sufficient if that's what Cavium calls them. In other contexts (other mfgrs) I've seen e.g. A1 stepping or B0.
I think it would also be useful to have a kernel config to toggle this, I'm guessing later ThunderX hardware will be fixed, and non-ThunderX SoCs won't need this. If someone is building a custom kernel for their hardware they may wish to disable unneeded workarounds.
Hmm, that seems reasonable - see e.g. NO_F00F_HACK in i386 (although it has the wrong sense).
I suggest:
- Add THUNDERX_PASS1_ERRATA option
- Put the workarounds in #ifdef THUNDERX_PASS1_ERRATA
- Add it to GENERIC
As an aside I emailed Robert Richter (author of the Linux workaround in the LKML post referenced above), and he will let us know if/when public details are available.
Comment Actions
I mostly dislike an idea of having errata-specific ifdefs inside the code. This one is the first, but there are still few more I'd like to submit and would like to avoid causing mess within generic drivers.
In this revision I've added global errata-matcher for thunderx p1.0&p1.1. Global kernel option can enable/disable that define and let the compiler evaluate 'if' statements and automatically remove any unnecessary code.
Comment Actions
How difficult will it be for someone in 5 years time to know if this workaround is still needed? The important part to know is whether the workaround is needed in production silicon. There are too many places in the arm and mips code where we are unsure why something is needed, and if it is still the case.
The names A1 and B0 don't tell us if it's production ready or not, there are cases where A0 is good, where for other chips D1 may be unstable.
Comment Actions
Andrew, would you mind explaining what is the problem here?
At first you disliked an idea of having any vendor-specific defines in kernel config (option SOC_CAV_THUNDERX) so I developed a method for resolving it dynamically in runtime. That approach is really optimal and gives us only 5-cycle penalty when the cpu is not thunderx (reading data from pcpu takes 3-cycles if is in L1 cache, value comparison and near jump are another 2-cycles). That's way better than workarounds in Linux, and, what's more, marginal when you compare with cycles spent in the PIC interrupt handler routine.
Now we are adding magical errata option to kernconf and creating mess, which we'd like to avoid... But that's fine if that's what you want. My primary goal is to have thunderx support done.
Please also elaborate on what's wrong with "Chip revision: Pass 1.0, Pass 1.1". I've got no idea how can I be more specific on this. These names are an official Cavium revisions and are printed in the bootloader console, EFI... they are even on a sticker! Just not sure how anyone who has a contact with thunderx can miss it.
This workaround is one of the official Cavium patchsets required for supporting their ThunderX product line. Pass 1.0 is an early version (the one available in Sentex and the one another clients are testing as well), where the Pass 1.1 is planned to be the first one with a long-term support. Both of them are "production ready" and officially supported by Cavm, but only workarounds affecting p1.1 (and automatically p1.0 also) are disclosed to the open-source communities., without any technical details covered by the NDA.
Comment Actions
It will be easier in 5 years than now - we don't yet know if Pass 1.1 is production silicon or not. Probably the best that we can do is add a comment marker to check again with Cavium in 3 or 6 months.
Comment Actions
Where have I said I don't like vendor specific options? I don't like vendor specific kernels, but this is a different issue. My preference is for a dynamic check that can be compiled out when building a cut-down kernel when needed. I would be happy with this behind option SOC_CAV_THUNDERX, or whatever the option gets named.
That approach is really optimal and gives us only 5-cycle penalty when the cpu is not thunderx (reading data from pcpu takes 3-cycles if is in L1 cache, value comparison and near jump are another 2-cycles). That's way better than workarounds in Linux, and, what's more, marginal when you compare with cycles spent in the PIC interrupt handler routine.
Now we are adding magical errata option to kernconf and creating mess, which we'd like to avoid... But that's fine if that's what you want. My primary goal is to have thunderx support done.Please also elaborate on what's wrong with "Chip revision: Pass 1.0, Pass 1.1". I've got no idea how can I be more specific on this. These names are an official Cavium revisions and are printed in the bootloader console, EFI... they are even on a sticker! Just not sure how anyone who has a contact with thunderx can miss it.
The problem with "Chip revision: Pass 1.0, Pass 1.1" is it is missing the information on if the hardware is expected to be in general use and we will need to care about it in 5 years. Assume a new programmer is coming along without knowledge of the Cavium hardware, they won't know if Pass 1.0 is hardware they need to care about or not.
This workaround is one of the official Cavium patchsets required for supporting their ThunderX product line. Pass 1.0 is an early version (the one available in Sentex and the one another clients are testing as well), where the Pass 1.1 is planned to be the first one with a long-term support. Both of them are "production ready" and officially supported by Cavm, but only workarounds affecting p1.1 (and automatically p1.0 also) are disclosed to the open-source communities., without any technical details covered by the NDA.
Then say something like:
Revision: Pass 1.0 (early access), Pass 1.1 (general release)
This will tell the next programmer we expect Pass 1.1 to be in general use, but not the Pass 1.0. This can then be revised later if we find the Pass 1.1 is, for example, a limited general release.
It will only be easier if there is someone around with knowledge of the hardware, we have had cases where the people with this knowledge have come and gone and we loose this knowledge.
Comment Actions
It will be easier in 5 years than now - we don't yet know if Pass 1.1 is production silicon or not. Probably the best that we can do is add a comment marker to check again with Cavium in 3 or 6 months.
It will only be easier if there is someone around with knowledge of the hardware, we have had cases where the people with this knowledge have come and gone and we loose this knowledge.
Well, that's why I said 3 or 6 months -- while we have good contacts at Cavium and regular dialogue. Adding early access and general release annotations is probably fine, but we should still check in 3 or 6 months to see how things have developed.
| sys/arm64/arm64/gic_v3.c | ||
|---|---|---|
| 238 | So both Pass 1.0 and 1.1 have this errata. Is Cavium really going to market with Pass 1.1? that likely means we'd have to keep this around for a long time. If that's just the first wave and there will be a Pass 2.0 which fixes this, that would be better. | |
| sys/arm64/include/cpu.h | ||
| 122 | How does this only match pass 1.1 and earlier? And what happens when Pass 2.0 comes out that has only some of the Errata from 1.1 fixed? This is currently only used in one place, so maybe it isn't a huge deal. But the name is suboptimal and will likely need to change in the future if it goes in unaltered. | |
| sys/arm64/arm64/gic_v3.c | ||
|---|---|---|
| 238 | As far as we know right now, yes. | |
| sys/arm64/include/cpu.h | ||
|---|---|---|
| 122 | As Cavium states, all erratas will be fixed in 2.0, so no vendor-specific workaround will be necessary. However, I'm not aware of any dates describing when that hardware will be released, probably not soon, and even then there will be clients using 1.1. | |
| sys/arm64/include/cpu.h | ||
|---|---|---|
| 122 | One more comment... If Cavium releases pass2.0 which somehow require any erratas, I'd rather expect to have a separate set of defines, like thunderx_pass_2_0_errata which can be enabled/disabled independently from all p1.1 stuff. | |
| sys/arm64/include/cpu.h | ||
|---|---|---|
| 122 |
Right, although hopefully there are none. And if it turns out that no pass 1.1 silicon ends up remaining in production we'll be able to pull these back out -- but I don't think we know how likely that is to happen at this point. | |
Comment Actions
Given my experience with Octeon and work arounds, I'd like us to think about a more scalable way to do them than having a separate #define for each one. Given the time constraints we have today, that doesn't need to happen today, but over time the Octeon work arounds really got out of hand (both in our in-tree stuff, in the SDK and in some out of tree stuff I was involved with). We should be thinking about this.
Comment Actions
a more scalable way to do them than having a separate #define for each one
Just to be clear, we're talking here about #defines for each silicon revision (or group of them), not each workaround.
Comment Actions
I have no problem with the workarounds being enabled on a per SoC basis, so this one would be enabled for all ThunderX hardware. This would mean one could update the code and have an old kernel config still work with new hardware, assuming this has a different set of workarounds.
Comment Actions
Like I said, I'm cool with what we have for the moment. It won't scale, but we're not at scale yet. Testing for different passes based on a define will work for now, so doesn't need to be fixed right away. But in a year or two, assuming we're wildly successful, we'll start to hit the point where we're starting to feel scale issues.
Honestly, I'd be happier with a bit array of work arounds that gets initialized on a per SoC / revision basis that a boot-loader tunable could override if necessary for new silicon that appears in the field. But I understand the issues with that, not least that you can only have 64 work arounds per SoC, but that should be plenty, right :)
Anyway, what's here's fine.