On Tue, Nov 13, 2018 at 05:36:37PM +0000, Marc Zyngier wrote: > On Tue, 13 Nov 2018 15:16:03 +0000, > David Long <dave.long@xxxxxxxxxx> wrote: > > > > On 11/13/18 9:23 AM, Marc Zyngier wrote: > > > Russell, > > > > > > On Mon, 12 Nov 2018 16:54:10 +0000, > > > Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> wrote: > > >> > > >> Marc, > > >> > > >> Can you please ack this to say that you are now happy with it after > > >> your comments on version 1, so we can move forward and have Greg > > >> merge it. > > >> > > >> Thanks. > > >> > > >> On Wed, Nov 07, 2018 at 11:43:47AM -0500, David Long wrote: > > >>> From: Russell King <rmk+kernel@xxxxxxxxxxxxxxx> > > >>> > > >>> Commit 10115105cb3aa17b5da1cb726ae8dd5f6854bd93 upstream. > > >>> Commit 6282e916f774e37845c65d1eae9f8c649004f033 upstream. > > >>> > > >>> Add firmware based hardening for cores that require more complex > > >>> handling in firmware. > > >>> > > >>> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx> > > >>> Boot-tested-by: Tony Lindgren <tony@xxxxxxxxxxx> > > >>> Reviewed-by: Tony Lindgren <tony@xxxxxxxxxxx> > > >>> Reviewed-by: Marc Zyngier <marc.zyngier@xxxxxxx> > > >>> Signed-off-by: David A. Long <dave.long@xxxxxxxxxx> > > > > > > Sure. Feel free to add my > > > > > > Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx> > > > > > > I assume someone has tested these patches (I haven't, and I'm unlikely > > > to do so in the near future as I'm travelling). I'm not sure Tony's > > > "Boot-tested-by" is still valid, and Florian's earlier set of tests > > > didn't show the issues of the initial backport. > > > > > > Thanks, > > > > > > M. > > > > > > > I tested the patch set through kernelci and (belatedly) > > kvm-unit-tests, the latter of which revealed the problem in V1 #11/24. > > I have to assume Florian didn't specifically test kvm, something I > > myself had originally assumed would be covered by kernelci. > > > > I didn't scrub any of the ack/tested/reviewed lines from the original > > patches. I've always assumed this is the correct way to do this but > > maybe it's not? > > Leaving the tags is absolutely fine, they indicate that the original > patch was actually tested. > > I'm more worried of potential regressions: we've already found two > problems, and although I cannot spot any other, it is fairly obvious > that there has only been a limited amount of testing. It may not be a > problem, but I'd rather be cautious. The biggest problem is getting people to actually test the damn patches, and not relying on the autobuilders/autobooters to do the work for you. The autobooters are hopeless when it comes to identifying problems (as I've now said multiple times since the regressions were first spotted.) I'm of the opinion that the autobooters actually give _misleading_ results, and have been doing every since they were setup. They claim that a platform which boots to a shell prompt, but which the kernel log contains a stack trace is a "pass" result. When you receive an email that says that nothing failed, you don't bother going to (eg) kernelci to read the boot logs, you assume there's no problem. So the failures (eg, due to WARN_ON() or worse, an oops that doesn't prevent reaching a shell prompt) will go unnoticed. That's exactly what has happened with the big.Little changes. The ENDPROC() issue is much harder, none of the autobooters cover the case of running a kernel under KVM afaik. So that path never gets executed - it's worse than that, because even if we _did_ have that, we need to have both an ARM kernel and Thumb2 kernel exercised. We really can't rely on "eyes" spotting it, because as has been shown many times, we have lots of cases where things get missed in review. The claim that "open source is better because you have many eyes looking at the source" seems to be provably false. The same goes for the mess up in vfp_preserve_user_clear_hwstate(), no one spotted that either, and it was only when some other compiler issued a warning that it was found. We keep asking people to test, but I don't think it does any good in the long run other than delaying patches. Unfortunately, the point at which people notice problems is only _after_ the patches have been merged into mainline and they are effectively forced via that method to test the merged result. Again, there's plenty of evidence to this. For example, I've been posting asking people to test the big.Little Spectre patches that caused problems last time around. I get the impression I'm completely wasting my time, because I'm getting zero replies on that, not even from the individual who reported the problem last time around. I might as well have merged the patches last week and get it over with, rather than delaying them until Friday. At least we'll find any problems quicker that way, because, as I've said above, it _forces_ people to test. I'm afraid that our "ask people to test" model just doesn't work anymore now that we have autobuilders - I guess people end up thinking that the autobuilders will do the work for them... :( In summary, what I'm saying is that getting patches tested so we can have confidence that they are correct is damn hard to nigh on impossible. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
