On 04/18/2018 12:54 PM, Borislav Petkov wrote: > On Mon, Apr 16, 2018 at 04:59:02PM -0500, Alexandru Gagniuc wrote: >> Firmware is evil: >> - ACPI was created to "try and make the 'ACPI' extensions somehow >> Windows specific" in order to "work well with NT and not the others >> even if they are open" >> - EFI was created to hide "secret" registers from the OS. >> - UEFI was created to allow compromising an otherwise secure OS. >> >> Never has firmware been created to solve a problem or simplify an >> otherwise cumbersome process. It is of no surprise then, that >> firmware nowadays intentionally crashes an OS. > > I don't believe I'm saying this but, get rid of that rant. Even though I > agree, it doesn't belong in a commit message. Of course. (snip)> Well, Tyler touched that AER error severity handling recently and we had > it all nicely documented in the comment above ghes_handle_aer(). > > Your ghes_handle_aer_irqsafe() graft basically bypasses > ghes_handle_aer() instead of incorporating in it. > > If all you wanna say is, the severity computation should go through all > the sections and look at each error's severity before making a decision, > then add that to ghes_severity() instead of doing that "deferrable" > severity dance. ghes_severity() is a one-to-one mapping from a set of unsorted severities to monotonically increasing numbers. The "one-to-one" mapping part of the sentence is obvious from the function name. To change it to parse the entire GHES would completely destroy this, and I think it would apply policy in the wrong place. Should I do that, I might have to call it something like ghes_parse_and_apply_policy_to_severity(). But that misses the whole point if these changes. I would like to get to the handlers first, and then decide if things are okay or not, but the ARM guys didn't exactly like this approach. It seems there are quite some per-error-type considerations. The logical step is to associate these considerations with the specific error type they apply to, rather than hide them as a decision under an innocent ghes_severity(). > And add the changes to the policy to the comment above > ghes_handle_aer(). I don't want any changes from people coming and going > and leaving us scratching heads why we did it this way. > > And no need for those handlers and so on - make it simple first - then we > can talk more complex handling. I don't want to leave people scratching their heads, but I also don't want to make AER a special case without having a generic way to handle these cases. People are just as susceptible to scratch their heads wondering why AER is a special case and everything else crashes. Maybe it's better move the AER handling to NMI/IRQ context, since ghes_handle_aer() is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things. Alex -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
