Hi,
On Wed, Oct 17, 2018 at 02:29:17PM -0700, Jim Mattson wrote:
> On Wed, Oct 17, 2018 at 1:26 PM, Christian Ehrhardt <lk@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 16, 2018 at 02:18:25PM -0700, Jim Mattson wrote:
> >> On Tue, Oct 16, 2018 at 1:37 PM, Christian Ehrhardt <lk@xxxxxxx> wrote:
> >>
> >> > - set_debugreg(0, 6);
> >> > + if (vcpu->arch.dr6 & ~DR6_RESERVED)
> >> > + set_debugreg(0, 6);
> >>
> >> This will skip setting %dr6 when DR6.RTM is clear. That seems
> >> incorrect, since DR6.RTM is active-low. Should this be:
> >>
> >> if (vcpu->arch.dr6 != DR6_RESERVED)
> >> set_debugreg(0, 6);
> >
> > Oh, bummer. I didn't realize that there is a non-reserved bit that
> > defaults to 1 and not to zero, now. Thanks for pointing this out.
> > Currently, DR6_RESERVED includes %dr6.RTM, i.e. the check above
> > ignores the current value of %dr6.RTM.
> >
> > Thus technically the code is ok both on machines that support
> > RTM and on machines that don't. However, DR6_RESERVED should not
> > contain bit 16 anymore.
> >
> > From this point there are two options:
> > - Ignore the value of the RTM bit along with the reserved bits
> > and continue to write zero to %dr6. This is equivalent to the
> > current code but I'd make the fact that RTM is included in the
> > mask explicit.
> > - Do not ignore the value of dr6.RTM but expect it to be set
> > by default. In this case the reset value changes from zero to
> > DB_RTM (0x10000).
> >
> > What do you think? regards Christian
>
> We want to deactivate all bits in %dr6 if any are active, right? So...
Not quite. Actually, we don't really care about the actual value
that we write to dr6.RTM as Intel-SDM states that RTM is explicitly
set on every debug exception.
The purpose of my change is to avoid the dr6 write here whenever
possible as it causes a VMEXIT if we run as an L1 guest.
> if (vcpu->arch.dr6 != DR6_RESERVED)
> set_debugreg(DR6_RESERVED, 6);
I don't like this idea. It works as long as DR6_RESERVED
includes a non-reserved bit (RTM). However, someone is bound to change
that in the future. And when DR6_RESERVED does not include RTM
anymore (as its name suggests), the if(...) above will always return
false on machines without RTM support (as RTM will always be set in
vcpu->arch.dr6).
regards Christian
