On 24/3/23 03:39, Sean Christopherson wrote:
On Fri, Feb 03, 2023, Alexey Kardashevskiy wrote:
Follow-up question: does KVM _have_ to wait until KVM_SEV_LAUNCH_UPDATE_VMSA to
set the flag?
Nope. Will repost soon as a reply to this mail.
Please, please do not post new versions In-Reply-To the previous version, and
especially not In-Reply-To a random mail buried deep in the thread. b4, which
is imperfect but makes my life sooo much easier, gets confused by all the threading
and partial rerolls. The next version also buries _this_ reply, which is partly
why I haven't responded until now. I simply missed this the below questions because
I saw v4 and assumed all my feedback was addressed, i.e. that I could handle this
in the context of 6.4 and not earlier.
Ok, noted. Sorry for the mess.
Continuing on that topic, please do not post a new version until open questions
from the previous version are resolved. Posting a new version when there are
unresolved questions might feel like it helps things move faster, but more often
than not it has the comlete opposite effect.
Well I thought keeping the history in one place/thread is helping. Oh
well...
+/* enable/disable SEV-ES DebugSwap support */
+static bool sev_es_debug_swap_enabled = true;
+module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644);
Module param needs 0444 permissions, i.e. shouldn't be writable after KVM is
loaded. Though I don't know that providing a module param is warranted in this
case.
KVM provides module params for SEV and SEV-ES because there are legitimate
reasons to turn them off, but at a glance, I don't see why we'd want that for this
feature.
/me confused. You suggested this in the first place for (I think) for the
case if the feature is found to be broken later on so admins can disable it.
Hrm, so I did. Right, IIUC, this has guest visible effects, i.e. guest can
read/write DRs, and so the admin might want the ability to disable the feature.
Speaking of past me, no one answered my question about how this will interact
with SNP, where the VM can maniuplate the VMSA.
: > @@ -604,6 +607,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
: > save->xss = svm->vcpu.arch.ia32_xss;
: > save->dr6 = svm->vcpu.arch.dr6;
: >
: > + if (sev->debug_swap)
: > + save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP;
:
: Resurrecting my objection to "AP Creation NAE event"[*], what happens if a hypervisor
: supports GHCB_HV_FT_SNP_AP_CREATION but not DebugSwap? IIUC, a guest can corrupt
: host DRs by enabling DebugSwap in the VMSA of an AP vCPU, e.g. the CPU will load
: zeros on VM-Exit if the host hasn't stuffed the host save area fields.
I was convinced it was answered (by Tom). (...10min later...) now I can
see it was an internal email :-/
The host will have to save DRs and masks to the host save area for SNP
and non-SNP guests (until we get the hw which allows masking features).
Which patchset will do this - it depends on what gets accepted first -
this or the SNP support.
: KVM can obviously just make sure to save its DRs if hardware supports DebugSwap,
: but what if DebugSwap is buggy and needs to be disabled? And what about the next
: feature that can apparently be enabled by the guest?
:
: [*] https://lore.kernel.org/all/YWnbfCet84Vup6q9@xxxxxxxxxx
And I was using it to verify "x86/debug: Fix stack recursion caused by DR7
accesses" which is convenient but it is a minor thing.
...
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 60c7c880266b..6c54a3c9d442 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1190,7 +1190,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
set_exception_intercept(svm, UD_VECTOR);
set_exception_intercept(svm, MC_VECTOR);
set_exception_intercept(svm, AC_VECTOR);
- set_exception_intercept(svm, DB_VECTOR);
+ if (!sev_es_is_debug_swap_enabled())
+ set_exception_intercept(svm, DB_VECTOR);
This is wrong. KVM needs to intercept #DBs when debugging non-SEV-ES VMs.
Sorry, not following. The #DB intercept for non-SEV-ES is enabled here.
The helper in this version (v3) just queries whether or not the feature is enabled,
it doesn't differentiate between SEV-ES and other VM types. I.e. loading KVM with
SEV-ES and DebugSwap enabled would break non-SEV-ES VMs running on the same host.
+bool sev_es_is_debug_swap_enabled(void)
+{
+ return sev_es_debug_swap_enabled;
+}
Looks like this was fixed in v4.
Right. I'll try addressing the comments from the other big reply of
yours and send the patches. Thanks for all comments!
This _could_ be tied to X86_FEATURE_NO_NESTED_DATA_BP, but the KVM would need to
toggle the intercept depending on whether or not userspace wants to debug the
guest.
Similar to the DR7 interception, can this check sev_features directly?
--
Alexey
