Re: [PATCH kernel v3 2/3] KVM: SEV: Enable data breakpoints in SEV-ES

[Alexey Kardashevskiy <aik@amd.com>]

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@google.com
> 
>> 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