| Message ID | 20170324144632.5896-4-opendmb@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: > This commit adds support for minimal handling of SError aborts and > allows them to be hooked by a driver or other part of the kernel to > install a custom SError abort handler. The hook function returns > the previously registered handler so that handlers may be chained if > desired. > > The handler should return the value 0 if the error has been handled, > otherwise the handler should either call the next handler in the > chain or return a non-zero value. ... so the order these get calls is completely dependent on probe order... > Since the Instruction Specific Syndrome value for SError aborts is > implementation specific the registerred handlers must implement > their own parsing of the syndrome. ... and drivers have to be intimately familiar with the CPU, in order to be able to parse its IMPLEMENTATION DEFINED ESR_ELx.ISS value. Even then, there's no guarantee there's anything useful there, since it is IMPLEMENTATION DEFINED and could simply be RES0 or UNKNOWN in all cases. I do not think it is a good idea to allow arbitrary drivers to hook this fault in this manner. > + .align 6 > +el0_error: > + kernel_entry 0 > +el0_error_naked: > + mrs x25, esr_el1 // read the syndrome register > + lsr x24, x25, #ESR_ELx_EC_SHIFT // exception class > + cmp x24, #ESR_ELx_EC_SERROR // SError exception in EL0 > + b.ne el0_error_inv > +el0_serr: > + mrs x26, far_el1 > + // enable interrupts before calling the main handler > + enable_dbg_and_irq ... why? We don't do this for inv_entry today. > + ct_user_exit > + bic x0, x26, #(0xff << 56) > + mov x1, x25 > + mov x2, sp > + bl do_serr_abort > + b ret_to_user > +el0_error_inv: > + enable_dbg > + mov x0, sp > + mov x1, #BAD_ERROR > + mov x2, x25 > + b bad_mode > +ENDPROC(el0_error) Clearly you expect these to be delivered at arbitrary times during execution. What if a KVM guest is executing at the time the SError is delivered? To be quite frank, I don't believe that we can reliably and safely handle this misfeature in the kernel, and this infrastructure only provides the illusion that we can. I do not think it makes sense to do this. Thanks, Mark.
On 03/24/2017 08:16 AM, Mark Rutland wrote: > On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: >> This commit adds support for minimal handling of SError aborts and >> allows them to be hooked by a driver or other part of the kernel to >> install a custom SError abort handler. The hook function returns >> the previously registered handler so that handlers may be chained if >> desired. >> >> The handler should return the value 0 if the error has been handled, >> otherwise the handler should either call the next handler in the >> chain or return a non-zero value. > > ... so the order these get calls is completely dependent on probe > order... Yes, but this was an attempt to keep some flexibility in handling a very ambiguous event. > >> Since the Instruction Specific Syndrome value for SError aborts is >> implementation specific the registerred handlers must implement >> their own parsing of the syndrome. > > ... and drivers have to be intimately familiar with the CPU, in order to > be able to parse its IMPLEMENTATION DEFINED ESR_ELx.ISS value. > > Even then, there's no guarantee there's anything useful there, since it > is IMPLEMENTATION DEFINED and could simply be RES0 or UNKNOWN in all > cases. > > I do not think it is a good idea to allow arbitrary drivers to hook > this fault in this manner. > I agree. It should really be resolved in the fault handling code like it is for the ARM architecture, but the IMPLEMENTATION DEFINED nature of the event for ARM64 makes this unmanageable but for the most specific use cases, which is what is attempted here. >> + .align 6 >> +el0_error: >> + kernel_entry 0 >> +el0_error_naked: >> + mrs x25, esr_el1 // read the syndrome register >> + lsr x24, x25, #ESR_ELx_EC_SHIFT // exception class >> + cmp x24, #ESR_ELx_EC_SERROR // SError exception in EL0 >> + b.ne el0_error_inv >> +el0_serr: >> + mrs x26, far_el1 >> + // enable interrupts before calling the main handler >> + enable_dbg_and_irq > > ... why? > > We don't do this for inv_entry today. > Yes, my initial downstream implementation modified inv_entry, but after commit 7d9e8f71b989 ("arm64: avoid returning from bad mode") added the user abort handling for el0_inv I tried to follow that approach so user mode errors (i.e. bad writes) wouldn't kill the kernel. >> + ct_user_exit >> + bic x0, x26, #(0xff << 56) >> + mov x1, x25 >> + mov x2, sp >> + bl do_serr_abort >> + b ret_to_user >> +el0_error_inv: >> + enable_dbg >> + mov x0, sp >> + mov x1, #BAD_ERROR >> + mov x2, x25 >> + b bad_mode >> +ENDPROC(el0_error) > > Clearly you expect these to be delivered at arbitrary times during > execution. What if a KVM guest is executing at the time the SError is > delivered? The timing isn't really arbitrary in our particular use case. It is just after the bus interface has moved on from the failing transaction so from the bus interfaces perspective it is asynchronous. The main benefit is to help debug user mode code that accidentally maps a bad address since we would never make such an egregious error in the kernel ;) I'm afraid I'm not fully versed on the implications to KVM here. > > To be quite frank, I don't believe that we can reliably and safely > handle this misfeature in the kernel, and this infrastructure only > provides the illusion that we can. > > I do not think it makes sense to do this. > > Thanks, > Mark. > I understand your position since this was the cleanest approach I came up with and it is admittedly ugly. I would be happy to entertain any better suggestion on how this could be handled more cleanly. If you would consider an alternative implementation where we scrap the SError handler (i.e. maintain the ugliness in our downstream kernel) in favor of a more gentle user mode crash on SError that allows the kernel the opportunity to service the interrupt for diagnostic purposes I could try to repackage that. Thanks for the review! Doug
On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: > On 03/24/2017 08:16 AM, Mark Rutland wrote: > >On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: > If you would consider an alternative implementation where we scrap > the SError handler (i.e. maintain the ugliness in our downstream > kernel) in favor of a more gentle user mode crash on SError that > allows the kernel the opportunity to service the interrupt for > diagnostic purposes I could try to repackage that. If this is just for diagnostic purposes, I believe you can register a panic notifier, which can then read from the bus. The panic will occur, but you'll have the opportunity to log some information to dmesg. Thanks, Mark.
On 03/24/2017 10:35 AM, Mark Rutland wrote: > On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: >> On 03/24/2017 08:16 AM, Mark Rutland wrote: >>> On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: > >> If you would consider an alternative implementation where we scrap >> the SError handler (i.e. maintain the ugliness in our downstream >> kernel) in favor of a more gentle user mode crash on SError that >> allows the kernel the opportunity to service the interrupt for >> diagnostic purposes I could try to repackage that. > > If this is just for diagnostic purposes, I believe you can register a > panic notifier, which can then read from the bus. The panic will occur, > but you'll have the opportunity to log some information to dmesg. And crash the kernel? That sounds awful, FWIW the ARM/Linux kernel is able to recover just fine from user-space accessing e.g: invalid physical addresses in the GISB register space, bringing the same level of functionality to ARM64/Linux sounds reasonable to me.
Hi Florian, On Fri, Mar 24, 2017 at 10:53:48AM -0700, Florian Fainelli wrote: > On 03/24/2017 10:35 AM, Mark Rutland wrote: > > On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: > >> On 03/24/2017 08:16 AM, Mark Rutland wrote: > >>> On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: > > > >> If you would consider an alternative implementation where we scrap > >> the SError handler (i.e. maintain the ugliness in our downstream > >> kernel) in favor of a more gentle user mode crash on SError that > >> allows the kernel the opportunity to service the interrupt for > >> diagnostic purposes I could try to repackage that. > > > > If this is just for diagnostic purposes, I believe you can register a > > panic notifier, which can then read from the bus. The panic will occur, > > but you'll have the opportunity to log some information to dmesg. > > And crash the kernel? That sounds awful, FWIW the ARM/Linux kernel is > able to recover just fine from user-space accessing e.g: invalid > physical addresses in the GISB register space, bringing the same level > of functionality to ARM64/Linux sounds reasonable to me. I disagree, given that: (a) You cannot determine the (HW) origin of the SError in an architecturally portable way. i.e. when you take an SError, you have no way of determining what asynchronous event caused it. (b) SError is effectively an edge-triggered interrupt for fatal system errors (e.g. it may be triggered in resonse to ECC errors, corruption detected in caches, etc). Even if you can determine that the GISB triggered *an* SError, this does not tell you that this was the *only* SError. If you take an SError, something bad has already happened. Your data may already have been corrupted, and worse, you don't know when or where specifically this occurred (nor how many times). (c) You cannot determine the (SW) origin of an SError without relying upon implementation details. This cannot be written in a way that does not rely on microarchitecture, integration, etc, and would need to be updated for every future system with this misfeature. (d) Even if you can determine the (SW) origin of an SError by relying on IMPLEMENTATION DEFINED details, your handler needs to be intimately familiar with the arch in question in order to attempt to recover. For example, the existing code tries to skip an ARM instruction in some cases. For arm64 there are three cases that would need to be handled (AArch64 A64, AArch32 A32/ARM, AArch32 T32/Thumb). Further, it appears to me that the existing code is broken given that it doesn't handle Thumb, and given that it's skipping an instruction in response to an asynchronous event -- i.e. some arbitrary instruction after the one which triggered the abort. For better or worse, SError *must* be treated as fatal. As Doug stated: The main benefit is to help debug user mode code that accidentally maps a bad address since we would never make such an egregious error in the kernel ;) This is just one of many ways a userspace application with direct HW access can bring down the system. I see no reason to treat it any differently, especially given the above points. Thanks, Mark.
On 03/24/2017 11:31 AM, Mark Rutland wrote: > Hi Florian, > > On Fri, Mar 24, 2017 at 10:53:48AM -0700, Florian Fainelli wrote: >> On 03/24/2017 10:35 AM, Mark Rutland wrote: >>> On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: >>>> On 03/24/2017 08:16 AM, Mark Rutland wrote: >>>>> On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: >>> >>>> If you would consider an alternative implementation where we scrap >>>> the SError handler (i.e. maintain the ugliness in our downstream >>>> kernel) in favor of a more gentle user mode crash on SError that >>>> allows the kernel the opportunity to service the interrupt for >>>> diagnostic purposes I could try to repackage that. >>> >>> If this is just for diagnostic purposes, I believe you can register a >>> panic notifier, which can then read from the bus. The panic will occur, >>> but you'll have the opportunity to log some information to dmesg. >> >> And crash the kernel? That sounds awful, FWIW the ARM/Linux kernel is >> able to recover just fine from user-space accessing e.g: invalid >> physical addresses in the GISB register space, bringing the same level >> of functionality to ARM64/Linux sounds reasonable to me. > > I disagree, given that: > > (a) You cannot determine the (HW) origin of the SError in an > architecturally portable way. i.e. when you take an SError, you have > no way of determining what asynchronous event caused it. > > (b) SError is effectively an edge-triggered interrupt for fatal system > errors (e.g. it may be triggered in resonse to ECC errors, > corruption detected in caches, etc). Even if you can determine that > the GISB triggered *an* SError, this does not tell you that this was > the *only* SError. Correct, which is why Doug's changes allow chaining of handlers. > > If you take an SError, something bad has already happened. Your data > may already have been corrupted, and worse, you don't know when or > where specifically this occurred (nor how many times). Sure, but that still allows you to send the correct signal to a faulting application (unless I am missing something here). > > (c) You cannot determine the (SW) origin of an SError without relying > upon implementation details. This cannot be written in a way that > does not rely on microarchitecture, integration, etc, and would need > to be updated for every future system with this misfeature. Which is exactly what is being done here, with the help of platform specific information (we would not load brcmstb_gisb.c if we were not on a platform where it makes sense to use that HW). > > (d) Even if you can determine the (SW) origin of an SError by relying on > IMPLEMENTATION DEFINED details, your handler needs to be intimately > familiar with the arch in question in order to attempt to recover. > > For example, the existing code tries to skip an ARM instruction in > some cases. For arm64 there are three cases that would need to be > handled (AArch64 A64, AArch32 A32/ARM, AArch32 T32/Thumb). > > Further, it appears to me that the existing code is broken given > that it doesn't handle Thumb, and given that it's skipping an > instruction in response to an asynchronous event -- i.e. some > arbitrary instruction after the one which triggered the abort. OK, that could presumably be fixed though. > > For better or worse, SError *must* be treated as fatal. I disagree here, since this is a platform specific SError exception that we can actually handle correctly there is a chance to actually not take down the system on something that can be made non fatal and informative at the same time. > > As Doug stated: > > The main benefit is to help debug user mode code that accidentally > maps a bad address since we would never make such an egregious error > in the kernel ;) > > This is just one of many ways a userspace application with direct HW > access can bring down the system. I see no reason to treat it any > differently, especially given the above points. Partially disagree, in the absence of a way to specifically deal with the exception, I would almost agree, but this is not the case here, we have a piece of HW that can help us locate the problem, display an informative message, and send a SIGBUS to the faulting application. Anyway, I won't argue much further than that, but I certainly don't think taking down an entire system is going to prove itself useful when you need to deploy such a kernel to hundreds of people who have no clue what so ever what their actual problem is in the first place. Taking a SIGBUS and printing a message can at least allow us to say: read more carefully, it say exactly what's wrong.
On Fri, Mar 24 2017 at 07:02:05 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 03/24/2017 11:31 AM, Mark Rutland wrote: >> Hi Florian, >> >> On Fri, Mar 24, 2017 at 10:53:48AM -0700, Florian Fainelli wrote: >>> On 03/24/2017 10:35 AM, Mark Rutland wrote: >>>> On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: >>>>> On 03/24/2017 08:16 AM, Mark Rutland wrote: >>>>>> On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: >>>> >>>>> If you would consider an alternative implementation where we scrap >>>>> the SError handler (i.e. maintain the ugliness in our downstream >>>>> kernel) in favor of a more gentle user mode crash on SError that >>>>> allows the kernel the opportunity to service the interrupt for >>>>> diagnostic purposes I could try to repackage that. >>>> >>>> If this is just for diagnostic purposes, I believe you can register a >>>> panic notifier, which can then read from the bus. The panic will occur, >>>> but you'll have the opportunity to log some information to dmesg. >>> >>> And crash the kernel? That sounds awful, FWIW the ARM/Linux kernel is >>> able to recover just fine from user-space accessing e.g: invalid >>> physical addresses in the GISB register space, bringing the same level >>> of functionality to ARM64/Linux sounds reasonable to me. >> >> I disagree, given that: >> >> (a) You cannot determine the (HW) origin of the SError in an >> architecturally portable way. i.e. when you take an SError, you have >> no way of determining what asynchronous event caused it. >> >> (b) SError is effectively an edge-triggered interrupt for fatal system >> errors (e.g. it may be triggered in resonse to ECC errors, >> corruption detected in caches, etc). Even if you can determine that >> the GISB triggered *an* SError, this does not tell you that this was >> the *only* SError. > > Correct, which is why Doug's changes allow chaining of handlers. > >> >> If you take an SError, something bad has already happened. Your data >> may already have been corrupted, and worse, you don't know when or >> where specifically this occurred (nor how many times). > > Sure, but that still allows you to send the correct signal to a faulting > application (unless I am missing something here). > >> >> (c) You cannot determine the (SW) origin of an SError without relying >> upon implementation details. This cannot be written in a way that >> does not rely on microarchitecture, integration, etc, and would need >> to be updated for every future system with this misfeature. > > Which is exactly what is being done here, with the help of platform > specific information (we would not load brcmstb_gisb.c if we were not on > a platform where it makes sense to use that HW). > >> >> (d) Even if you can determine the (SW) origin of an SError by relying on >> IMPLEMENTATION DEFINED details, your handler needs to be intimately >> familiar with the arch in question in order to attempt to recover. >> >> For example, the existing code tries to skip an ARM instruction in >> some cases. For arm64 there are three cases that would need to be >> handled (AArch64 A64, AArch32 A32/ARM, AArch32 T32/Thumb). >> >> Further, it appears to me that the existing code is broken given >> that it doesn't handle Thumb, and given that it's skipping an >> instruction in response to an asynchronous event -- i.e. some >> arbitrary instruction after the one which triggered the abort. > > OK, that could presumably be fixed though. > >> >> For better or worse, SError *must* be treated as fatal. > > I disagree here, since this is a platform specific SError exception that > we can actually handle correctly there is a chance to actually not take > down the system on something that can be made non fatal and informative > at the same time. > >> >> As Doug stated: >> >> The main benefit is to help debug user mode code that accidentally >> maps a bad address since we would never make such an egregious error >> in the kernel ;) >> >> This is just one of many ways a userspace application with direct HW >> access can bring down the system. I see no reason to treat it any >> differently, especially given the above points. > > Partially disagree, in the absence of a way to specifically deal with > the exception, I would almost agree, but this is not the case here, we > have a piece of HW that can help us locate the problem, display an > informative message, and send a SIGBUS to the faulting application. > > Anyway, I won't argue much further than that, but I certainly don't > think taking down an entire system is going to prove itself useful when > you need to deploy such a kernel to hundreds of people who have no clue > what so ever what their actual problem is in the first place. Taking a > SIGBUS and printing a message can at least allow us to say: read more > carefully, it say exactly what's wrong. I think there is one point that hasn't been made in this discussion. You seem to assume that an SError should be handled the same way on an ARMv8 system as you handle it on your ARMv7 platform. In most cases, Linux on ARMv7 runs (for better or worse) in secure mode, making it the only software agent capable of handling an Asynchronous Abort. On v8, Linux runs in non-secure mode, and relies on secure firmware for a large set of platform specific services (PM, CPU bring-up...). Crucially, error handling is one of these services. It is largely expected that an SError should be first taken to EL3 (SCR_EL3.EA being set), handled there by any platform-specific FW able to triage and log the error, and could even be reported to EL1NS via some standard mechanism. If the FW decides to reinject this SError to the non-secure side, then this is bound to be fatal, because even the FW couldn't handle it. So my view is that you should move that kind of error handling to the place where it actually belongs. It will give you the opportunity to print out your debug messages if necessary, and leave the SError handling in the kernel the way it should be: fatal. Thanks, M.
On 03/25/2017 03:06 AM, Marc Zyngier wrote: > On Fri, Mar 24 2017 at 07:02:05 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 03/24/2017 11:31 AM, Mark Rutland wrote: >>> Hi Florian, >>> >>> On Fri, Mar 24, 2017 at 10:53:48AM -0700, Florian Fainelli wrote: >>>> On 03/24/2017 10:35 AM, Mark Rutland wrote: >>>>> On Fri, Mar 24, 2017 at 09:48:40AM -0700, Doug Berger wrote: >>>>>> On 03/24/2017 08:16 AM, Mark Rutland wrote: >>>>>>> On Fri, Mar 24, 2017 at 07:46:26AM -0700, Doug Berger wrote: >>>>> >>>>>> If you would consider an alternative implementation where we scrap >>>>>> the SError handler (i.e. maintain the ugliness in our downstream >>>>>> kernel) in favor of a more gentle user mode crash on SError that >>>>>> allows the kernel the opportunity to service the interrupt for >>>>>> diagnostic purposes I could try to repackage that. >>>>> >>>>> If this is just for diagnostic purposes, I believe you can register a >>>>> panic notifier, which can then read from the bus. The panic will occur, >>>>> but you'll have the opportunity to log some information to dmesg. >>>> >>>> And crash the kernel? That sounds awful, FWIW the ARM/Linux kernel is >>>> able to recover just fine from user-space accessing e.g: invalid >>>> physical addresses in the GISB register space, bringing the same level >>>> of functionality to ARM64/Linux sounds reasonable to me. >>> >>> I disagree, given that: >>> >>> (a) You cannot determine the (HW) origin of the SError in an >>> architecturally portable way. i.e. when you take an SError, you have >>> no way of determining what asynchronous event caused it. >>> >>> (b) SError is effectively an edge-triggered interrupt for fatal system >>> errors (e.g. it may be triggered in resonse to ECC errors, >>> corruption detected in caches, etc). Even if you can determine that >>> the GISB triggered *an* SError, this does not tell you that this was >>> the *only* SError. >> >> Correct, which is why Doug's changes allow chaining of handlers. >> >>> >>> If you take an SError, something bad has already happened. Your data >>> may already have been corrupted, and worse, you don't know when or >>> where specifically this occurred (nor how many times). >> >> Sure, but that still allows you to send the correct signal to a faulting >> application (unless I am missing something here). >> >>> >>> (c) You cannot determine the (SW) origin of an SError without relying >>> upon implementation details. This cannot be written in a way that >>> does not rely on microarchitecture, integration, etc, and would need >>> to be updated for every future system with this misfeature. >> >> Which is exactly what is being done here, with the help of platform >> specific information (we would not load brcmstb_gisb.c if we were not on >> a platform where it makes sense to use that HW). >> >>> >>> (d) Even if you can determine the (SW) origin of an SError by relying on >>> IMPLEMENTATION DEFINED details, your handler needs to be intimately >>> familiar with the arch in question in order to attempt to recover. >>> >>> For example, the existing code tries to skip an ARM instruction in >>> some cases. For arm64 there are three cases that would need to be >>> handled (AArch64 A64, AArch32 A32/ARM, AArch32 T32/Thumb). >>> >>> Further, it appears to me that the existing code is broken given >>> that it doesn't handle Thumb, and given that it's skipping an >>> instruction in response to an asynchronous event -- i.e. some >>> arbitrary instruction after the one which triggered the abort. >> >> OK, that could presumably be fixed though. >> >>> >>> For better or worse, SError *must* be treated as fatal. >> >> I disagree here, since this is a platform specific SError exception that >> we can actually handle correctly there is a chance to actually not take >> down the system on something that can be made non fatal and informative >> at the same time. >> >>> >>> As Doug stated: >>> >>> The main benefit is to help debug user mode code that accidentally >>> maps a bad address since we would never make such an egregious error >>> in the kernel ;) >>> >>> This is just one of many ways a userspace application with direct HW >>> access can bring down the system. I see no reason to treat it any >>> differently, especially given the above points. >> >> Partially disagree, in the absence of a way to specifically deal with >> the exception, I would almost agree, but this is not the case here, we >> have a piece of HW that can help us locate the problem, display an >> informative message, and send a SIGBUS to the faulting application. >> >> Anyway, I won't argue much further than that, but I certainly don't >> think taking down an entire system is going to prove itself useful when >> you need to deploy such a kernel to hundreds of people who have no clue >> what so ever what their actual problem is in the first place. Taking a >> SIGBUS and printing a message can at least allow us to say: read more >> carefully, it say exactly what's wrong. > > I think there is one point that hasn't been made in this discussion. You > seem to assume that an SError should be handled the same way on an ARMv8 > system as you handle it on your ARMv7 platform. Correct, that is absolutely a conscious (or not) assumption about the platforms being discussed here, and therefore the proposed patches. > > In most cases, Linux on ARMv7 runs (for better or worse) in secure mode, > making it the only software agent capable of handling an Asynchronous > Abort. On v8, Linux runs in non-secure mode, and relies on secure > firmware for a large set of platform specific services (PM, CPU > bring-up...). Crucially, error handling is one of these services. > > It is largely expected that an SError should be first taken to EL3 > (SCR_EL3.EA being set), handled there by any platform-specific FW able > to triage and log the error, and could even be reported to EL1NS via > some standard mechanism. If the FW decides to reinject this SError to > the non-secure side, then this is bound to be fatal, because even the FW > couldn't handle it. > > So my view is that you should move that kind of error handling to the > place where it actually belongs. It will give you the opportunity to > print out your debug messages if necessary, and leave the SError > handling in the kernel the way it should be: fatal. Your point of view is absolutely valid, but does not necessarily match the reality of things as implemented in real life because: - the trusted firmware on some platforms is something that is subject to a different process than a kernel or user-space upgrade and/or security/certification process, and may be challenging to update in the future, so people design it with everything they will ever need: PSCI v0.2 services and that's it - there is what ARM Ltd. provides as guidelines about how a platform should be done (read: must, should?), and there is how implementors end up making (most often uneducated, biased and under time pressure) design decisions about how a platform will be designed, and prototype is as close as you could get to a product in the embedded space That being said, it is definitively something that should be done, so we'll make sure this gets funneled back to the appropriate people so they can think about adding SError handling where it should be done. Thanks for the feedback!
diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h index e05f5b8c7c1c..60ac784ff4e6 100644 --- a/arch/arm64/include/asm/system_misc.h +++ b/arch/arm64/include/asm/system_misc.h @@ -41,6 +41,8 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int, void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *), int sig, int code, const char *name); +void *hook_serror_handler(int (*fn)(unsigned long, unsigned int, + struct pt_regs *)); struct mm_struct; extern void show_pte(struct mm_struct *mm, unsigned long addr); diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 43512d4d7df2..d043d66b390d 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -323,18 +323,18 @@ ENTRY(vectors) ventry el1_sync // Synchronous EL1h ventry el1_irq // IRQ EL1h ventry el1_fiq_invalid // FIQ EL1h - ventry el1_error_invalid // Error EL1h + ventry el1_error // Error EL1h ventry el0_sync // Synchronous 64-bit EL0 ventry el0_irq // IRQ 64-bit EL0 ventry el0_fiq_invalid // FIQ 64-bit EL0 - ventry el0_error_invalid // Error 64-bit EL0 + ventry el0_error // Error 64-bit EL0 #ifdef CONFIG_COMPAT ventry el0_sync_compat // Synchronous 32-bit EL0 ventry el0_irq_compat // IRQ 32-bit EL0 ventry el0_fiq_invalid_compat // FIQ 32-bit EL0 - ventry el0_error_invalid_compat // Error 32-bit EL0 + ventry el0_error_compat // Error 32-bit EL0 #else ventry el0_sync_invalid // Synchronous 32-bit EL0 ventry el0_irq_invalid // IRQ 32-bit EL0 @@ -374,10 +374,6 @@ ENDPROC(el0_error_invalid) el0_fiq_invalid_compat: inv_entry 0, BAD_FIQ, 32 ENDPROC(el0_fiq_invalid_compat) - -el0_error_invalid_compat: - inv_entry 0, BAD_ERROR, 32 -ENDPROC(el0_error_invalid_compat) #endif el1_sync_invalid: @@ -508,6 +504,34 @@ el1_preempt: ret x24 #endif + .align 6 +el1_error: + kernel_entry 1 + mrs x1, esr_el1 // read the syndrome register + lsr x24, x1, #ESR_ELx_EC_SHIFT // exception class + cmp x24, #ESR_ELx_EC_SERROR // SError exception in EL1 + b.ne el1_error_inv +el1_serr: + mrs x0, far_el1 + enable_dbg + // re-enable interrupts if they were enabled in the aborted context + tbnz x23, #7, 1f // PSR_I_BIT + enable_irq +1: + mov x2, sp // struct pt_regs + bl do_serr_abort + + // disable interrupts before pulling preserved data off the stack + disable_irq + kernel_exit 1 +el1_error_inv: + enable_dbg + mov x0, sp + mov x2, x1 + mov x1, #BAD_ERROR + b bad_mode +ENDPROC(el1_error) + /* * EL0 mode handlers. */ @@ -584,6 +608,11 @@ el0_svc_compat: el0_irq_compat: kernel_entry 0, 32 b el0_irq_naked + + .align 6 +el0_error_compat: + kernel_entry 0, 32 + b el0_error_naked #endif el0_da: @@ -705,6 +734,32 @@ el0_irq_naked: b ret_to_user ENDPROC(el0_irq) + .align 6 +el0_error: + kernel_entry 0 +el0_error_naked: + mrs x25, esr_el1 // read the syndrome register + lsr x24, x25, #ESR_ELx_EC_SHIFT // exception class + cmp x24, #ESR_ELx_EC_SERROR // SError exception in EL0 + b.ne el0_error_inv +el0_serr: + mrs x26, far_el1 + // enable interrupts before calling the main handler + enable_dbg_and_irq + ct_user_exit + bic x0, x26, #(0xff << 56) + mov x1, x25 + mov x2, sp + bl do_serr_abort + b ret_to_user +el0_error_inv: + enable_dbg + mov x0, sp + mov x1, #BAD_ERROR + mov x2, x25 + b bad_mode +ENDPROC(el0_error) + /* * Register switch for AArch64. The callee-saved registers need to be saved * and restored. On entry: diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 43319ed58a47..577fecea7c7d 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -705,3 +705,34 @@ int cpu_enable_pan(void *__unused) return 0; } #endif /* CONFIG_ARM64_PAN */ + +static int (*serror_handler)(unsigned long, unsigned int, + struct pt_regs *) __ro_after_init; + +void *__init hook_serror_handler(int (*fn)(unsigned long, unsigned int, + struct pt_regs *)) +{ + void *ret = serror_handler; + + serror_handler = fn; + return ret; +} + +asmlinkage void __exception do_serr_abort(unsigned long addr, unsigned int esr, + struct pt_regs *regs) +{ + struct siginfo info; + + if (serror_handler) + if (!serror_handler(addr, esr, regs)) + return; + + pr_alert("Unhandled SError: (0x%08x) at 0x%016lx\n", esr, addr); + __show_regs(regs); + + info.si_signo = SIGILL; + info.si_errno = 0; + info.si_code = ILL_ILLOPC; + info.si_addr = (void __user *)addr; + arm64_notify_die("", regs, &info, esr); +}
This commit adds support for minimal handling of SError aborts and allows them to be hooked by a driver or other part of the kernel to install a custom SError abort handler. The hook function returns the previously registered handler so that handlers may be chained if desired. The handler should return the value 0 if the error has been handled, otherwise the handler should either call the next handler in the chain or return a non-zero value. Since the Instruction Specific Syndrome value for SError aborts is implementation specific the registerred handlers must implement their own parsing of the syndrome. Signed-off-by: Doug Berger <opendmb@gmail.com> --- arch/arm64/include/asm/system_misc.h | 2 ++ arch/arm64/kernel/entry.S | 69 ++++++++++++++++++++++++++++++++---- arch/arm64/mm/fault.c | 31 ++++++++++++++++ 3 files changed, 95 insertions(+), 7 deletions(-)