Currently SVM setup is done sequentially in
init_vmcb() -> sev_init_vmcb() -> sev_es_init_vmcb()
and tries keeping SVM/SEV/SEV-ES bits separated. One of the exceptions
is DR intercepts which is for SEV-ES before sev_es_init_vmcb() runs.
Move the SEV-ES intercept setup to sev_es_init_vmcb(). From now on
set_dr_intercepts()/clr_dr_intercepts() handle SVM/SEV only.
Extend the comment about intercepting DR7 which is to prevent the CPU
from getting stuck in an infinite #DB loop as described in
https://bugzilla.redhat.com/show_bug.cgi?id=1278496
No functional change intended.
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Alexey Kardashevskiy <[email protected]>
Reviewed-by: Santosh Shukla <[email protected]>
Reviewed-by: Tom Lendacky <[email protected]>
---
Changes:
v6:
* updated the commit log
* updated the DR7 intercept comment in the code
v5:
* updated the comments
* removed sev_es_guest() checks from set_dr_intercepts()/clr_dr_intercepts()
* removed remaining intercepts from clr_dr_intercepts()
---
arch/x86/kvm/svm/sev.c | 11 ++++++
arch/x86/kvm/svm/svm.c | 37 ++++++++------------
2 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 981286359b72..744bcc2e6a05 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2948,6 +2948,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
static void sev_es_init_vmcb(struct vcpu_svm *svm)
{
+ struct vmcb *vmcb = svm->vmcb01.ptr;
struct kvm_vcpu *vcpu = &svm->vcpu;
svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_SEV_ES_ENABLE;
@@ -2976,6 +2977,16 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
svm_set_intercept(svm, TRAP_CR4_WRITE);
svm_set_intercept(svm, TRAP_CR8_WRITE);
+ /*
+ * DR7 access must remain intercepted for an SEV-ES guest to disallow
+ * the guest kernel set up a #DB on memory that's needed to vector a #DB
+ * as otherwise the CPU gets stuck in an infinite #DB loop.
+ */
+ vmcb->control.intercepts[INTERCEPT_DR] = 0;
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE);
+ recalc_intercepts(svm);
+
/* Can't intercept XSETBV, HV can't modify XCR0 directly */
svm_clr_intercept(svm, INTERCEPT_XSETBV);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index bec6fb82f494..1df99e9f8655 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -694,23 +694,20 @@ static void set_dr_intercepts(struct vcpu_svm *svm)
{
struct vmcb *vmcb = svm->vmcb01.ptr;
- if (!sev_es_guest(svm->vcpu.kvm)) {
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR0_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR1_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR2_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR3_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR4_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR5_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR6_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR0_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR1_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR2_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR3_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR4_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR5_WRITE);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR6_WRITE);
- }
-
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR0_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR1_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR2_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR3_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR4_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR5_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR6_READ);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR0_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR1_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR2_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR3_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR4_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR5_WRITE);
+ vmcb_set_intercept(&vmcb->control, INTERCEPT_DR6_WRITE);
vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ);
vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE);
@@ -723,12 +720,6 @@ static void clr_dr_intercepts(struct vcpu_svm *svm)
vmcb->control.intercepts[INTERCEPT_DR] = 0;
- /* DR7 access must remain intercepted for an SEV-ES guest */
- if (sev_es_guest(svm->vcpu.kvm)) {
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_READ);
- vmcb_set_intercept(&vmcb->control, INTERCEPT_DR7_WRITE);
- }
-
recalc_intercepts(svm);
}
--
2.40.1
On Thu, Jun 15, 2023, Alexey Kardashevskiy wrote:
> @@ -2976,6 +2977,16 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
> svm_set_intercept(svm, TRAP_CR4_WRITE);
> svm_set_intercept(svm, TRAP_CR8_WRITE);
>
> + /*
> + * DR7 access must remain intercepted for an SEV-ES guest to disallow
> + * the guest kernel set up a #DB on memory that's needed to vector a #DB
> + * as otherwise the CPU gets stuck in an infinite #DB loop.
> + */
This isn't correct. Letting the guest configuring breakpoints would be weird
and nonsensical, but it wouldn't lead to infinite #DBs so long as KVM intercepts
#DB.
KVM intercepts DR7 when DebugSwap isn't enabled because otherwise KVM has no way
of context switching DR[0-3] for the guest. At least, I assume that's the case,
AFAICT the APM never actually says what happens with DR[0-3] when DebugSwap is
disabled.
On 1/7/23 07:49, Sean Christopherson wrote:
> On Thu, Jun 15, 2023, Alexey Kardashevskiy wrote:
>> @@ -2976,6 +2977,16 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>> svm_set_intercept(svm, TRAP_CR4_WRITE);
>> svm_set_intercept(svm, TRAP_CR8_WRITE);
>>
>> + /*
>> + * DR7 access must remain intercepted for an SEV-ES guest to disallow
>> + * the guest kernel set up a #DB on memory that's needed to vector a #DB
>> + * as otherwise the CPU gets stuck in an infinite #DB loop.
>> + */
>
> This isn't correct. Letting the guest configuring breakpoints would be weird
> and nonsensical, but it wouldn't lead to infinite #DBs so long as KVM intercepts
> #DB.
True. OTOH not intercepting #DB and intercepting only DR7 would do the
same thing.
> KVM intercepts DR7 when DebugSwap isn't enabled because otherwise KVM has no way
> of context switching DR[0-3] for the guest. At least, I assume that's the case,
> AFAICT the APM never actually says what happens with DR[0-3] when DebugSwap is
> disabled.
This is the SEV-ES code, no DR[0-3] context switching anyway, is not it?
The actual immediate reason for intercepting DR7 is "SEV-ES GHCB" but
this does not really explain it to me :-/
4.5 Debug Register Support
Currently, hardware debug traps are not supported for an SEV-ES guest.
The hypervisor must set the intercept for both read and write of the
debug control register (DR7). With the intercepts in place, the #VC
handler will be invoked when the guest accesses DR7. For a write to DR7,
the #VC handler should perform Standard VMGExit processing. The #VC
handler must not update the actual DR7 register, but rather it should
cache the DR7 value being written. For a read of DR7, the #VC handler
should return the cached value of the DR7 register.
--
Alexey