| Message ID | 20180920230243.65915-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Clear reserved bits of #DB exit qualification | expand |
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 >
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 >>
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 >>>
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.
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.
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;
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(-)