Messages in this thread | | | Date | Thu, 31 Mar 2016 14:12:31 +0100 | From | Mark Rutland <> | Subject | Re: [RFC] [PATCH] arm64: survive after access to unimplemented register |
| |
On Thu, Mar 31, 2016 at 03:28:59PM +0300, Yury Norov wrote: > Hi Mark, > > On Thu, Mar 31, 2016 at 11:05:48AM +0100, Mark Rutland wrote: > > On Thu, Mar 31, 2016 at 05:27:03AM +0300, Yury Norov wrote: > > > Not all vendors implement all the system registers ARM specifies. > > > > The ID registers in question are precisely documented in the ARM ARM > > (see table C5-6 in ARM DDI 0487A.i). Specifically, the ID space > > ID_AA64MMFR2_EL1 now falls in to is listed as RAZ. > > > > Any deviation from this is an erratum, and needs to be handled as such > > (e.g. listing in silicon-errata.txt). > > > > Does the issue affect ThunderX natively? > > Yes, Thunder is involved, but I cannot tell more due to NDA. > And this error is not in silicon-errata.txt. > I'll ask permission to share more details.
Ok. Regardless of how this is solved, we need to know the details of the erratum (and need an entry in silicon-errata.txt).
> > > So access them causes undefined instruction abort and then kernel > > > panic. There are 3 ways to handle it we can figure out: > > > - use conditional compilation and erratas; > > > - use kernel patching; > > > - inline fixups resolving the abort. > > > > > > Last option is more robust as it does not require additional efforts > > > to support targers. It is looking reasonable because in many cases > > > optional registers should be RAZ if not implemented. Special cases may > > > be handled by underlying __read_cpuid() when needed. > > > > I don't think we should do this if the only affected implementations are > > software emulators which can be patched (and have already been, in the > > case of QEMU). > > > > In future it's very likely that early assembly code (potentially in > > hypervisor context) will need to access ID registers which are currently > > reserved/RAZ, and it will be rather painful to fix up accesses to this. > > So we will not fix. This one fixes el1 only, and don't pretend for more.
At some point, it's practically guaranteed that we will have to access reserved/RAZ ID registers in other cases, so we _will_ need workarounds that cater for those sooner or later.
We need to consider how we can handle those, in case it implies constraints on our solution elsewhere, or requires a more complex, but more general solution (which we can implement part of today).
For example:
* The sanity checks code will perform many back-to-back register accesses. Trapping lots of these could be expensive, so not performing the MRS at all when known to be unsafe may be preferable.
* Some registers may be read in a hot/critical path, or potentially in a context where we cannot handle trapping (e.g. early boot code or parts of KVM). In some cases, patching may be preferable to an MRS that only gets executed depending on a branch condition.
Before we can do any of this, we need to know the conditions of the erratum, however.
> > Additionally, this workaround will silently mask other bugs in this area > > (e.g. if registers like ID_AA64MMFR0_EL1 were to trap for some reason on > > an implementation), which doesn't seem good. > > We can mask it less silently, for example, print message to dmesg. > > Initially I was thinking about erratas as well, but Arnd suggested > this approach, and now think it's better. From consumer point of view, > it's much better to have a warning line in dmesg, instead of bricked > device, after another kernel or driver update.
Having some warning is certainly better, though I think we need to scream _very loudly_ for cases we do not expect, as non-fatal warnings are easily/often ignored, and can later turn out to be more critical than previously believed.
Thanks, Mark.
| |