On 13.07.2018 22:44, Maxime Villard wrote: > Le 13/07/2018 à 21:54, Kamil Rytarowski a écrit : >> On 13.07.2018 21:31, Maxime Villard wrote: >>> I don't like the dbregs code. We unconditionally write into %dr6 and >>> %dr7 in >>> userret, that is, during each interruptible kernel->user transition. >>> >>> Some measurements with PMCs show that the loads of dr6/dr7 are one of >>> the >>> first sources of recovery cycles during a build.sh - the cycles the CPU >>> spends >>> recovering from a mispredicted branch or a machine clear event. >>> >>> We lose many cycles down there. The code should have been written to >>> just >>> switch dbregs during context switches, and not on each transition. >>> >>> It could be fixed, but since the PopSS vuln, we actually disabled >>> dbregs by >>> default (they are now privileged), and I don't think we will ever >>> re-enable >>> them, given the risk, even though we do have a mitigation for PopSS. >>> >>> So why not remove the code? >> >> I strongly object to the removal because of the debugging purposes. > > For root only? There is nearly zero use case. You did write dbregs, and of > course you couldn't know that something like PopSS would come out. But now > that we have this bug, the interest of the feature has severely diminished, > to the point where it's just totally pointless. > I disagree. I keep using it. And for everybody there is still an option to read the registers (or rather their cache). The POP SS x86 design flaw is irrelevant to the functionality. >> Their functionality is possible to simulate to some extend and with >> significant performance overhead (like execution in the single step move) >> >> I was benchmarking the existing code with DTrace and the overhead was >> negligible (like: 0,05% DR6 + 0,23% DR7). > > You may have had these results with your test case, but the fact is that > reloading DR6/DR7 all the time for processes that don't even use dbregs > is cycle-consuming, and it must be avoided. > It's a micro-optimization at this point, unless someone is doing dummy context switch benchmarking. >> http://netbsd.org/~kamil/will-it-scale/open1_processes.svg >> >> I've initially started it with the FreeBSD-like approach to set them >> during cpu_switchto(9) but at that time I haven't solved mysterious >> panics. >> >> I still have some intermediate draft patch with the cpu_switch(9) >> implementation: >> >> http://netbsd.org/~kamil/dbg9.txt > > This code seems still wrong. You are reloading DR6/DR7 even if the process > was not using dbregs. We don't want to touch anything if we do a switch > from > a process that did not use dbregs to a process that did not use dbregs > either. > > During the PopSS thing I remember someone told me that several > generations of > CPUs had numerous bugs with DR6 and DR7, and that the erratas were a > headache. I don't have the email handy, but googling the thing, several > erratas indeed seem to exist for different dbregs bugs. > The only errata or buggy implementation I recall is not masking reserved bits. We do mask them. FreeBSD used to have this exposed and NetBSD is safe. > If you really want to keep the code, I suggest we disable it entirely > (via a > #ifdef), until the support is rewritten not to do reloads all the time. > This > can be done later, and won't reduce the functionality meanwhile (which is > already not there since it's privileged...). > > Maxime I disagree with disabling it. The code is not broken, it's covered by tests, it's in use.
Attachment:
signature.asc
Description: OpenPGP digital signature