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...
if (vcpu->arch.dr6 != DR6_RESERVED)
set_debugreg(DR6_RESERVED, 6);
