KVM: nVMX: Clear reserved bits of #DB exit qualification
diff mbox series

Message ID 20180920230243.65915-1-jmattson@google.com
State New
Headers show
Series
  • KVM: nVMX: Clear reserved bits of #DB exit qualification
Related show

Commit Message

Jim Mattson Sept. 20, 2018, 11:02 p.m. UTC
Per volume 2 of the SDM, bits 63:15 and 12:4 of the exit qualification
field for debug exceptions are reserved (cleared to 0).

There is still an issue with stale DR6 bits potentially being
misreported for the current debug exception.  DR6 should not have been
modified before vectoring the #DB exception, and the "new DR6 bits"
should be available somewhere, but it was and they aren't.

Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/vmx.c              | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Sept. 21, 2018, 12:07 a.m. UTC | #1
On Thu, Sep 20, 2018 at 04:02:43PM -0700, Jim Mattson wrote:
> Per volume 2 of the SDM, bits 63:15 and 12:4 of the exit qualification
> field for debug exceptions are reserved (cleared to 0).

Ugh, I suspect we have a documentation bug regarding bit 16 (DR6.BTM),
and maybe bit 15 (DR6.BT) as well.  I'm pretty sure DR6.BTM should be
preserved if either HLE or RTM is supported.  DR6.BT will never be set,
but that's because it's impossible to take a task-switch #DB in guest
due to task switches unconditionally taking VMExit and not because the
bit is explicitly cleared.  DR6.BT doesn't really matter but I'll ask
since I need to bug someone about DR6.BTM anyways.

Maybe just explicitly clear DR6_FIXED_1 for now?  DR6_BT should be a
non-issue.  DR6.BTM is weird because of its inverted behavior, not
sure what the safest approach for that bit would be.

P.S. I assume you meant Volume 3?

> There is still an issue with stale DR6 bits potentially being
> misreported for the current debug exception.  DR6 should not have been
> modified before vectoring the #DB exception, and the "new DR6 bits"
> should be available somewhere, but it was and they aren't.
> 
> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/vmx.c              | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8e90488c3d56..234f7320ba59 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -177,6 +177,7 @@ enum {
>  
>  #define DR6_BD		(1 << 13)
>  #define DR6_BS		(1 << 14)
> +#define DR6_BT		(1 << 15)
>  #define DR6_RTM		(1 << 16)
>  #define DR6_FIXED_1	0xfffe0ff0
>  #define DR6_INIT	0xffff0ff0
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 533a327372c8..01f729830fd9 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3293,7 +3293,8 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
>  	} else {
>  		if (vmcs12->exception_bitmap & (1u << nr)) {
>  			if (nr == DB_VECTOR)
> -				*exit_qual = vcpu->arch.dr6;
> +				*exit_qual = vcpu->arch.dr6 &
> +					~(DR6_FIXED_1 | DR6_BT | DR6_RTM);
>
>  			else
>  				*exit_qual = 0;
>  			return 1;
> -- 
> 2.19.0.444.g18242da7ef-goog
>
Jim Mattson Sept. 21, 2018, 4 p.m. UTC | #2
Even if the documentation is stale, backwards compatibility requires
that bit 16 of the exit qualification (corresponding to DR6.RTM, per
Figure 17-1 of the SDM, volume 3) must have the opposite polarity of
DR6.RTM (just as bit 16 of the pending debug exceptions does). If
DR6.RTM is set, then bit 16 of the exit qualification must be clear.
Certainly, on hardware without TSX, it works this way. Now, perhaps if
DR6.RTM is clear, then bit 16 of the exit qualification should be set.
I can write a kvm-unit-test to find out, or you can ask around.

DR6.BT actually is an issue. As you point out, it will never be set in
the exit qualification for a #DB exception. However, it may be set in
DR6. Hence, when we try to infer the exit qualification from a
prematurely modified DR6, bit 15 should be masked off.

On Thu, Sep 20, 2018 at 5:07 PM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> On Thu, Sep 20, 2018 at 04:02:43PM -0700, Jim Mattson wrote:
>> Per volume 2 of the SDM, bits 63:15 and 12:4 of the exit qualification
>> field for debug exceptions are reserved (cleared to 0).
>
> Ugh, I suspect we have a documentation bug regarding bit 16 (DR6.BTM),
> and maybe bit 15 (DR6.BT) as well.  I'm pretty sure DR6.BTM should be
> preserved if either HLE or RTM is supported.  DR6.BT will never be set,
> but that's because it's impossible to take a task-switch #DB in guest
> due to task switches unconditionally taking VMExit and not because the
> bit is explicitly cleared.  DR6.BT doesn't really matter but I'll ask
> since I need to bug someone about DR6.BTM anyways.
>
> Maybe just explicitly clear DR6_FIXED_1 for now?  DR6_BT should be a
> non-issue.  DR6.BTM is weird because of its inverted behavior, not
> sure what the safest approach for that bit would be.
>
> P.S. I assume you meant Volume 3?
>
>> There is still an issue with stale DR6 bits potentially being
>> misreported for the current debug exception.  DR6 should not have been
>> modified before vectoring the #DB exception, and the "new DR6 bits"
>> should be available somewhere, but it was and they aren't.
>>
>> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h | 1 +
>>  arch/x86/kvm/vmx.c              | 3 ++-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 8e90488c3d56..234f7320ba59 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -177,6 +177,7 @@ enum {
>>
>>  #define DR6_BD               (1 << 13)
>>  #define DR6_BS               (1 << 14)
>> +#define DR6_BT               (1 << 15)
>>  #define DR6_RTM              (1 << 16)
>>  #define DR6_FIXED_1  0xfffe0ff0
>>  #define DR6_INIT     0xffff0ff0
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 533a327372c8..01f729830fd9 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -3293,7 +3293,8 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
>>       } else {
>>               if (vmcs12->exception_bitmap & (1u << nr)) {
>>                       if (nr == DB_VECTOR)
>> -                             *exit_qual = vcpu->arch.dr6;
>> +                             *exit_qual = vcpu->arch.dr6 &
>> +                                     ~(DR6_FIXED_1 | DR6_BT | DR6_RTM);
>>
>>                       else
>>                               *exit_qual = 0;
>>                       return 1;
>> --
>> 2.19.0.444.g18242da7ef-goog
>>
Jim Mattson Sept. 21, 2018, 5:14 p.m. UTC | #3
Yes, a kvm-unit-test shows that bit 16 of the exit qualification for
#DB should be *set* if a debug exception (#DB) or a breakpoint
exception (#BP) occurred inside an RTM region while advanced debugging
of RTM transactional regions was enabled. This is the opposite of
DR6.RTM. I'll post the test shortly.

Sean: Can you get Table 27-1 of the SDM (volume 3) corrected?

On Fri, Sep 21, 2018 at 9:00 AM, Jim Mattson <jmattson@google.com> wrote:
> Even if the documentation is stale, backwards compatibility requires
> that bit 16 of the exit qualification (corresponding to DR6.RTM, per
> Figure 17-1 of the SDM, volume 3) must have the opposite polarity of
> DR6.RTM (just as bit 16 of the pending debug exceptions does). If
> DR6.RTM is set, then bit 16 of the exit qualification must be clear.
> Certainly, on hardware without TSX, it works this way. Now, perhaps if
> DR6.RTM is clear, then bit 16 of the exit qualification should be set.
> I can write a kvm-unit-test to find out, or you can ask around.
>
> DR6.BT actually is an issue. As you point out, it will never be set in
> the exit qualification for a #DB exception. However, it may be set in
> DR6. Hence, when we try to infer the exit qualification from a
> prematurely modified DR6, bit 15 should be masked off.
>
> On Thu, Sep 20, 2018 at 5:07 PM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
>> On Thu, Sep 20, 2018 at 04:02:43PM -0700, Jim Mattson wrote:
>>> Per volume 2 of the SDM, bits 63:15 and 12:4 of the exit qualification
>>> field for debug exceptions are reserved (cleared to 0).
>>
>> Ugh, I suspect we have a documentation bug regarding bit 16 (DR6.BTM),
>> and maybe bit 15 (DR6.BT) as well.  I'm pretty sure DR6.BTM should be
>> preserved if either HLE or RTM is supported.  DR6.BT will never be set,
>> but that's because it's impossible to take a task-switch #DB in guest
>> due to task switches unconditionally taking VMExit and not because the
>> bit is explicitly cleared.  DR6.BT doesn't really matter but I'll ask
>> since I need to bug someone about DR6.BTM anyways.
>>
>> Maybe just explicitly clear DR6_FIXED_1 for now?  DR6_BT should be a
>> non-issue.  DR6.BTM is weird because of its inverted behavior, not
>> sure what the safest approach for that bit would be.
>>
>> P.S. I assume you meant Volume 3?
>>
>>> There is still an issue with stale DR6 bits potentially being
>>> misreported for the current debug exception.  DR6 should not have been
>>> modified before vectoring the #DB exception, and the "new DR6 bits"
>>> should be available somewhere, but it was and they aren't.
>>>
>>> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>> ---
>>>  arch/x86/include/asm/kvm_host.h | 1 +
>>>  arch/x86/kvm/vmx.c              | 3 ++-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>> index 8e90488c3d56..234f7320ba59 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -177,6 +177,7 @@ enum {
>>>
>>>  #define DR6_BD               (1 << 13)
>>>  #define DR6_BS               (1 << 14)
>>> +#define DR6_BT               (1 << 15)
>>>  #define DR6_RTM              (1 << 16)
>>>  #define DR6_FIXED_1  0xfffe0ff0
>>>  #define DR6_INIT     0xffff0ff0
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 533a327372c8..01f729830fd9 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -3293,7 +3293,8 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
>>>       } else {
>>>               if (vmcs12->exception_bitmap & (1u << nr)) {
>>>                       if (nr == DB_VECTOR)
>>> -                             *exit_qual = vcpu->arch.dr6;
>>> +                             *exit_qual = vcpu->arch.dr6 &
>>> +                                     ~(DR6_FIXED_1 | DR6_BT | DR6_RTM);
>>>
>>>                       else
>>>                               *exit_qual = 0;
>>>                       return 1;
>>> --
>>> 2.19.0.444.g18242da7ef-goog
>>>
Sean Christopherson Sept. 21, 2018, 5:47 p.m. UTC | #4
On Fri, Sep 21, 2018 at 09:00:50AM -0700, Jim Mattson wrote:
> Even if the documentation is stale, backwards compatibility requires
> that bit 16 of the exit qualification (corresponding to DR6.RTM, per
> Figure 17-1 of the SDM, volume 3) must have the opposite polarity of
> DR6.RTM (just as bit 16 of the pending debug exceptions does). If
> DR6.RTM is set, then bit 16 of the exit qualification must be clear.
> Certainly, on hardware without TSX, it works this way. Now, perhaps if
> DR6.RTM is clear, then bit 16 of the exit qualification should be set.
> I can write a kvm-unit-test to find out, or you can ask around.
> 
> DR6.BT actually is an issue. As you point out, it will never be set in
> the exit qualification for a #DB exception. However, it may be set in
> DR6. Hence, when we try to infer the exit qualification from a
> prematurely modified DR6, bit 15 should be masked off.

Ah, right.  And after digging a bit more I'm pretty sure DR6.BT truly
is reserved-to-zero in exit qual from an architectural perspective,
though I have a sneaky suspicion that ucode doesn't explicitly zero
out the bit while transferring its internal state to exit qual.

Anyways, I agree that explicitly clearing DR6.BT is correct.
Sean Christopherson Sept. 21, 2018, 5:49 p.m. UTC | #5
On Fri, Sep 21, 2018 at 10:14:44AM -0700, Jim Mattson wrote:
> Yes, a kvm-unit-test shows that bit 16 of the exit qualification for
> #DB should be *set* if a debug exception (#DB) or a breakpoint
> exception (#BP) occurred inside an RTM region while advanced debugging
> of RTM transactional regions was enabled. This is the opposite of
> DR6.RTM. I'll post the test shortly.
> 
> Sean: Can you get Table 27-1 of the SDM (volume 3) corrected?

Will do.

Patch
diff mbox series

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8e90488c3d56..234f7320ba59 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -177,6 +177,7 @@  enum {
 
 #define DR6_BD		(1 << 13)
 #define DR6_BS		(1 << 14)
+#define DR6_BT		(1 << 15)
 #define DR6_RTM		(1 << 16)
 #define DR6_FIXED_1	0xfffe0ff0
 #define DR6_INIT	0xffff0ff0
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 533a327372c8..01f729830fd9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3293,7 +3293,8 @@  static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
 	} else {
 		if (vmcs12->exception_bitmap & (1u << nr)) {
 			if (nr == DB_VECTOR)
-				*exit_qual = vcpu->arch.dr6;
+				*exit_qual = vcpu->arch.dr6 &
+					~(DR6_FIXED_1 | DR6_BT | DR6_RTM);
 			else
 				*exit_qual = 0;
 			return 1;