Page MenuHomeFreeBSD

THUNDERX ERRATA: Apply erratum for mrs ICC_IAR1_EL1
ClosedPublic

Authored by wma_semihalf.com on Jul 24 2015, 12:05 PM.

Details

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wma_semihalf.com retitled this revision from to THUNDERX ERRATA: Apply erratum for mrs ICC_IAR1_EL1 speculative execution.
wma_semihalf.com updated this object.
wma_semihalf.com edited the test plan for this revision. (Show Details)
wma_semihalf.com added reviewers: zbb, andrew, emaste.
wma_semihalf.com set the repository for this revision to rS FreeBSD src repository.
andrew requested changes to this revision.Jul 24 2015, 12:16 PM
andrew edited edge metadata.
andrew added inline comments.
sys/arm64/arm64/gic_v3.c
234–244 ↗(On Diff #7252)

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.

This revision now requires changes to proceed.Jul 24 2015, 12:16 PM
wma_semihalf.com updated this object.
wma_semihalf.com edited edge metadata.
wma_semihalf.com added inline comments.
sys/arm64/arm64/gic_v3.c
234–244 ↗(On Diff #7260)

Fixed, comments added. Unfortunately, these are the only information than can go public.

emaste added inline comments.Jul 24 2015, 2:09 PM
sys/arm64/arm64/gic_v3.c
234–244 ↗(On Diff #7260)

Is it possible to expand on the meaning of Pass 1.0/1.1?

wma_semihalf.com retitled this revision from THUNDERX ERRATA: Apply erratum for mrs ICC_IAR1_EL1 speculative execution to THUNDERX ERRATA: Apply erratum for mrs ICC_IAR1_EL1.
wma_semihalf.com edited edge metadata.
andrew requested changes to this revision.Jul 27 2015, 3:22 PM
andrew edited edge metadata.

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.

This revision now requires changes to proceed.Jul 27 2015, 3:22 PM
emaste edited edge metadata.Jul 27 2015, 3:32 PM
In D3184#64706, @andrew wrote:

What is Pass 1.0 and Pass 1.1? Someone reading this code that doesn't know the ThunderX would have no idea.

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.

wma_semihalf.com edited edge metadata.
wma_semihalf.com removed rS FreeBSD src repository as the repository for this revision.

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.

In D3184#64711, @emaste wrote:
In D3184#64706, @andrew wrote:

What is Pass 1.0 and Pass 1.1? Someone reading this code that doesn't know the ThunderX would have no idea.

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.

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.

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.

In D3184#64915, @andrew wrote:

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.

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.

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.

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.

In D3184#64956, @emaste wrote:
In D3184#64915, @andrew wrote:

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.

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.

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.

wma_semihalf.com edited edge metadata.

Added current info about Cavium revisions. Let me know if that's enough.

Added current info about Cavium revisions. Let me know if that's enough.

The descriptions look fine to me.

imp added inline comments.Jul 28 2015, 4:12 PM
sys/arm64/arm64/gic_v3.c
238 ↗(On Diff #7434)

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 ↗(On Diff #7434)

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.

emaste added inline comments.Jul 28 2015, 7:55 PM
sys/arm64/arm64/gic_v3.c
238 ↗(On Diff #7434)

As far as we know right now, yes.

sys/arm64/include/cpu.h
122 ↗(On Diff #7434)

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.
I can narrow down the condition to be sensitive only to 1.0/1.1, then the define name should be more consistent with what's actually doing.

wma_semihalf.com set the repository for this revision to rS FreeBSD src repository.
sys/arm64/include/cpu.h
125 ↗(On Diff #7450)

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.

emaste added inline comments.Jul 30 2015, 7:04 PM
sys/arm64/include/cpu.h
125 ↗(On Diff #7450)

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.

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.

imp added a comment.Jul 30 2015, 7:14 PM

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.

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.

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.

imp accepted this revision.Jul 31 2015, 3:32 AM
imp added a reviewer: imp.
In D3184#65657, @andrew wrote:

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.

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.

This revision was automatically updated to reflect the committed changes.