Re: Removing dbregs

[Maxime Villard <max%m00nbsd.net@localhost>]

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.

> 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.

> 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.

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