lkml.org 
[lkml]   [2023]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH kernel v5 5/6] KVM: SEV: Enable data breakpoints in SEV-ES
From
Sean, ping?

I wonder if this sev-es-not-singlestepping is a showstopper or it is
alright to repost this patchset without it? Thanks,


On 30/5/23 18:57, Alexey Kardashevskiy wrote:
>
>
> On 27/5/23 00:39, Sean Christopherson wrote:
>> On Fri, May 26, 2023, Alexey Kardashevskiy wrote:
>>>
>>> On 24/5/23 01:44, Sean Christopherson wrote:
>>>> On Tue, May 23, 2023, Alexey Kardashevskiy wrote:
>>>>>> Actually, can't disabling #DB interception for DebugSwap SEV-ES
>>>>>> guests be a
>>>>>> separate patch?  KVM can still inject #DBs for SEV-ES guests, no?
>>>>>
>>>>> Sorry for my ignorance but what is the point of injecting #DB if
>>>>> there is no
>>>>> way of changing the guest's DR7?
>>>>
>>>> Well, _injecting_ the #DB is necessary for correctness from the
>>>> guest's perspective.
>>>> "What's the point of _intercepting_ #DB" is the real question.  And
>>>> for SEV-ES guests
>>>> with DebugSwap, there is no point, which is why I agree that KVM
>>>> should disable
>>>> interception in that case.  What I'm calling out is that disabling
>>>> #Db interception
>>>> isn't _necessary_ for correctness (unless I'm missing something),
>>>> which means that
>>>> it can and should go in a separate patch.
>>>
>>>
>>> About this. Instead of sev_es_init_vmcb(), I can toggle the #DB
>>> intercept
>>> when toggling guest_debug, see below. This
>>> kvm_x86_ops::update_exception_bitmap hook is called on vcpu reset and
>>> kvm_arch_vcpu_ioctl_set_guest_debug (which skips this call if
>>> guest_state_protected = true).
>>
>> KVM also intercepts #DB when single-stepping over IRET to find an NMI
>> window, so
>> you'd also have to factor in nmi_singlestep, and update
>> svm_enable_nmi_window()
>> and disable_nmi_singlestep() to call svm_update_exception_bitmap().
>
> Uff. New can of worms for me :-/
>
>
>>> Is there any downside?
>>
>> Complexity is the main one.  The complexity is quite low, but the
>> benefit to the
>> guest is likely even lower.  A #DB in the guest isn't likely to be
>> performance
>> sensitive.  And on the flip side, opening an NMI window would be a
>> tiny bit more
>> expensive, though I doubt that would be meaningful either.
>>
>> All in all, I think it makes sense to just keep intercepting #DB for
>> non-SEV-ES
>> guests.
>>
>> Side topic, isn't there an existing bug regarding SEV-ES NMI windows?
>> KVM can't
>> actually single-step an SEV-ES guest, but tries to set RFLAGS.TF anyways.
>
> Why is it a "bug" and what does the patch fix? Sound to me as it is
> pointless and the guest won't do single stepping and instead will run
> till it exits somehow, what do I miss?
>
>> Blech,
>> and suppressing EFER.SVME in efer_trap() is a bit gross,
>
> Why suppressed? svm_set_efer() sets it eventually anyway.
>
>> but I suppose since the
>> GHCB doesn't allow for CLGI or STGI it's "fine".
>
> GHCB does not mention this, instead these are always intercepted in
> init_vmcb().
>
>> E.g. shouldn't KVM do this?
>
> It sure can and I am happy to include this into the series, the commit
> log is what I am struggling with :)
>
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index ca32389f3c36..4e4a49031efe 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3784,6 +3784,16 @@ static void svm_enable_nmi_window(struct
>> kvm_vcpu *vcpu)
>>          if (svm_get_nmi_mask(vcpu) && !svm->awaiting_iret_completion)
>>                  return; /* IRET will cause a vm exit */
>> +       /*
>> +        * KVM can't single-step SEV-ES guests and instead assumes
>> that IRET
>> +        * in the guest will always succeed,
>
> It relies on GHCB's NMI_COMPLETE (which SVM than handles is it was IRET):
>
>         case SVM_VMGEXIT_NMI_COMPLETE:
>                 ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_IRET);
>                 break;
>
>
>> i.e. clears NMI masking on the
>> +        * next VM-Exit.  Note, GIF is guaranteed to be '1' for SEV-ES
>> guests
>> +        * as the GHCB doesn't allow for CLGI or STGI (and KVM suppresses
>> +        * EFER.SVME for good measure, see efer_trap()).
>
> SVM KVM seems to not enforce EFER.SVME, the guest does what it wants and
> KVM is only told the new value via EFER_WRITE_TRAP. And "writes by
> SEV-ES guests to EFER.SVME are always ignored by hardware" says the APM.
> I must be missing the point here...
>
>
>> +        */
>> +       if (sev_es_guest(vcpu->kvm))
>> +               return;
>> +
>>          if (!gif_set(svm)) {
>>                  if (vgif)
>>                          svm_set_intercept(svm, INTERCEPT_STGI);
>

--
Alexey

\
 
 \ /
  Last update: 2023-06-02 01:32    [W:0.606 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site