| Message ID | 20221129193717.513824-7-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM: vNMI (with my fixes) | expand |
On 11/30/2022 1:07 AM, Maxim Levitsky wrote: > SEV-ES guests don't use IRET interception for the detection of > an end of a NMI. > > Therefore it makes sense to create a wrapper to avoid repeating > the check for the SEV-ES. > > No functional change is intended. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/svm.c | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 512b2aa21137e2..cfed6ab29c839a 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) > has_error_code, error_code); > } > > +static void svm_disable_iret_interception(struct vcpu_svm *svm) > +{ > + if (!sev_es_guest(svm->vcpu.kvm)) > + svm_clr_intercept(svm, INTERCEPT_IRET); > +} > + > +static void svm_enable_iret_interception(struct vcpu_svm *svm) > +{ > + if (!sev_es_guest(svm->vcpu.kvm)) > + svm_set_intercept(svm, INTERCEPT_IRET); > +} > + nits: s/_iret_interception / _iret_intercept does that make sense? Thanks, Santosh > static int iret_interception(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > > ++vcpu->stat.nmi_window_exits; > svm->awaiting_iret_completion = true; > - if (!sev_es_guest(vcpu->kvm)) { > - svm_clr_intercept(svm, INTERCEPT_IRET); > + > + svm_disable_iret_interception(svm); > + if (!sev_es_guest(vcpu->kvm)) > svm->nmi_iret_rip = kvm_rip_read(vcpu); > - } > + > kvm_make_request(KVM_REQ_EVENT, vcpu); > return 1; > } > @@ -3470,8 +3483,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu) > return; > > svm->nmi_masked = true; > - if (!sev_es_guest(vcpu->kvm)) > - svm_set_intercept(svm, INTERCEPT_IRET); > + svm_enable_iret_interception(svm); > ++vcpu->stat.nmi_injections; > } > > @@ -3614,12 +3626,10 @@ static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) > > if (masked) { > svm->nmi_masked = true; > - if (!sev_es_guest(vcpu->kvm)) > - svm_set_intercept(svm, INTERCEPT_IRET); > + svm_enable_iret_interception(svm); > } else { > svm->nmi_masked = false; > - if (!sev_es_guest(vcpu->kvm)) > - svm_clr_intercept(svm, INTERCEPT_IRET); > + svm_disable_iret_interception(svm); > } > } >
On Mon, 2022-12-05 at 21:11 +0530, Santosh Shukla wrote: > On 11/30/2022 1:07 AM, Maxim Levitsky wrote: > > SEV-ES guests don't use IRET interception for the detection of > > an end of a NMI. > > > > Therefore it makes sense to create a wrapper to avoid repeating > > the check for the SEV-ES. > > > > No functional change is intended. > > > > Suggested-by: Sean Christopherson <seanjc@google.com> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/svm/svm.c | 28 +++++++++++++++++++--------- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > index 512b2aa21137e2..cfed6ab29c839a 100644 > > --- a/arch/x86/kvm/svm/svm.c > > +++ b/arch/x86/kvm/svm/svm.c > > @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) > > has_error_code, error_code); > > } > > > > +static void svm_disable_iret_interception(struct vcpu_svm *svm) > > +{ > > + if (!sev_es_guest(svm->vcpu.kvm)) > > + svm_clr_intercept(svm, INTERCEPT_IRET); > > +} > > + > > +static void svm_enable_iret_interception(struct vcpu_svm *svm) > > +{ > > + if (!sev_es_guest(svm->vcpu.kvm)) > > + svm_set_intercept(svm, INTERCEPT_IRET); > > +} > > + > > nits: > s/_iret_interception / _iret_intercept > does that make sense? Makes sense. I can also move this to svm.h near the svm_set_intercept(), I think it better a better place for this function there if no objections. Best regards, Maxim Levitsky > > Thanks, > Santosh > > > static int iret_interception(struct kvm_vcpu *vcpu) > > { > > struct vcpu_svm *svm = to_svm(vcpu); > > > > ++vcpu->stat.nmi_window_exits; > > svm->awaiting_iret_completion = true; > > - if (!sev_es_guest(vcpu->kvm)) { > > - svm_clr_intercept(svm, INTERCEPT_IRET); > > + > > + svm_disable_iret_interception(svm); > > + if (!sev_es_guest(vcpu->kvm)) > > svm->nmi_iret_rip = kvm_rip_read(vcpu); > > - } > > + > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > return 1; > > } > > @@ -3470,8 +3483,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu) > > return; > > > > svm->nmi_masked = true; > > - if (!sev_es_guest(vcpu->kvm)) > > - svm_set_intercept(svm, INTERCEPT_IRET); > > + svm_enable_iret_interception(svm); > > ++vcpu->stat.nmi_injections; > > } > > > > @@ -3614,12 +3626,10 @@ static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) > > > > if (masked) { > > svm->nmi_masked = true; > > - if (!sev_es_guest(vcpu->kvm)) > > - svm_set_intercept(svm, INTERCEPT_IRET); > > + svm_enable_iret_interception(svm); > > } else { > > svm->nmi_masked = false; > > - if (!sev_es_guest(vcpu->kvm)) > > - svm_clr_intercept(svm, INTERCEPT_IRET); > > + svm_disable_iret_interception(svm); > > } > > } > >
On 12/6/2022 5:44 PM, Maxim Levitsky wrote: > On Mon, 2022-12-05 at 21:11 +0530, Santosh Shukla wrote: >> On 11/30/2022 1:07 AM, Maxim Levitsky wrote: >>> SEV-ES guests don't use IRET interception for the detection of >>> an end of a NMI. >>> >>> Therefore it makes sense to create a wrapper to avoid repeating >>> the check for the SEV-ES. >>> >>> No functional change is intended. >>> >>> Suggested-by: Sean Christopherson <seanjc@google.com> >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> arch/x86/kvm/svm/svm.c | 28 +++++++++++++++++++--------- >>> 1 file changed, 19 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c >>> index 512b2aa21137e2..cfed6ab29c839a 100644 >>> --- a/arch/x86/kvm/svm/svm.c >>> +++ b/arch/x86/kvm/svm/svm.c >>> @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) >>> has_error_code, error_code); >>> } >>> >>> +static void svm_disable_iret_interception(struct vcpu_svm *svm) >>> +{ >>> + if (!sev_es_guest(svm->vcpu.kvm)) >>> + svm_clr_intercept(svm, INTERCEPT_IRET); >>> +} >>> + >>> +static void svm_enable_iret_interception(struct vcpu_svm *svm) >>> +{ >>> + if (!sev_es_guest(svm->vcpu.kvm)) >>> + svm_set_intercept(svm, INTERCEPT_IRET); >>> +} >>> + >> >> nits: >> s/_iret_interception / _iret_intercept >> does that make sense? > > Makes sense. I can also move this to svm.h near the svm_set_intercept(), I think > it better a better place for this function there if no objections. > I think current approach is fine since function used in svm.c only. but I have no strong opinion on moving to svm.h either ways. Thanks, Santosh > Best regards, > Maxim Levitsky >> >> Thanks, >> Santosh >> >>> static int iret_interception(struct kvm_vcpu *vcpu) >>> { >>> struct vcpu_svm *svm = to_svm(vcpu); >>> >>> ++vcpu->stat.nmi_window_exits; >>> svm->awaiting_iret_completion = true; >>> - if (!sev_es_guest(vcpu->kvm)) { >>> - svm_clr_intercept(svm, INTERCEPT_IRET); >>> + >>> + svm_disable_iret_interception(svm); >>> + if (!sev_es_guest(vcpu->kvm)) >>> svm->nmi_iret_rip = kvm_rip_read(vcpu); >>> - } >>> + >>> kvm_make_request(KVM_REQ_EVENT, vcpu); >>> return 1; >>> } >>> @@ -3470,8 +3483,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu) >>> return; >>> >>> svm->nmi_masked = true; >>> - if (!sev_es_guest(vcpu->kvm)) >>> - svm_set_intercept(svm, INTERCEPT_IRET); >>> + svm_enable_iret_interception(svm); >>> ++vcpu->stat.nmi_injections; >>> } >>> >>> @@ -3614,12 +3626,10 @@ static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) >>> >>> if (masked) { >>> svm->nmi_masked = true; >>> - if (!sev_es_guest(vcpu->kvm)) >>> - svm_set_intercept(svm, INTERCEPT_IRET); >>> + svm_enable_iret_interception(svm); >>> } else { >>> svm->nmi_masked = false; >>> - if (!sev_es_guest(vcpu->kvm)) >>> - svm_clr_intercept(svm, INTERCEPT_IRET); >>> + svm_disable_iret_interception(svm); >>> } >>> } >>> > >
On Thu, 2022-12-08 at 17:39 +0530, Santosh Shukla wrote: > > On 12/6/2022 5:44 PM, Maxim Levitsky wrote: > > On Mon, 2022-12-05 at 21:11 +0530, Santosh Shukla wrote: > > > On 11/30/2022 1:07 AM, Maxim Levitsky wrote: > > > > SEV-ES guests don't use IRET interception for the detection of > > > > an end of a NMI. > > > > > > > > Therefore it makes sense to create a wrapper to avoid repeating > > > > the check for the SEV-ES. > > > > > > > > No functional change is intended. > > > > > > > > Suggested-by: Sean Christopherson <seanjc@google.com> > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > --- > > > > arch/x86/kvm/svm/svm.c | 28 +++++++++++++++++++--------- > > > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > > > index 512b2aa21137e2..cfed6ab29c839a 100644 > > > > --- a/arch/x86/kvm/svm/svm.c > > > > +++ b/arch/x86/kvm/svm/svm.c > > > > @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) > > > > has_error_code, error_code); > > > > } > > > > > > > > +static void svm_disable_iret_interception(struct vcpu_svm *svm) > > > > +{ > > > > + if (!sev_es_guest(svm->vcpu.kvm)) > > > > + svm_clr_intercept(svm, INTERCEPT_IRET); > > > > +} > > > > + > > > > +static void svm_enable_iret_interception(struct vcpu_svm *svm) > > > > +{ > > > > + if (!sev_es_guest(svm->vcpu.kvm)) > > > > + svm_set_intercept(svm, INTERCEPT_IRET); > > > > +} > > > > + > > > > > > nits: > > > s/_iret_interception / _iret_intercept > > > does that make sense? > > > > Makes sense. I can also move this to svm.h near the svm_set_intercept(), I think > > it better a better place for this function there if no objections. > > > I think current approach is fine since function used in svm.c only. but I have > no strong opinion on moving to svm.h either ways. I also think so, just noticed something in case there are any objections. Best regards, Maxim Levitsky > > Thanks, > Santosh > > > Best regards, > > Maxim Levitsky > > > Thanks, > > > Santosh > > > > > > > static int iret_interception(struct kvm_vcpu *vcpu) > > > > { > > > > struct vcpu_svm *svm = to_svm(vcpu); > > > > > > > > ++vcpu->stat.nmi_window_exits; > > > > svm->awaiting_iret_completion = true; > > > > - if (!sev_es_guest(vcpu->kvm)) { > > > > - svm_clr_intercept(svm, INTERCEPT_IRET); > > > > + > > > > + svm_disable_iret_interception(svm); > > > > + if (!sev_es_guest(vcpu->kvm)) > > > > svm->nmi_iret_rip = kvm_rip_read(vcpu); > > > > - } > > > > + > > > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > > > return 1; > > > > } > > > > @@ -3470,8 +3483,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu) > > > > return; > > > > > > > > svm->nmi_masked = true; > > > > - if (!sev_es_guest(vcpu->kvm)) > > > > - svm_set_intercept(svm, INTERCEPT_IRET); > > > > + svm_enable_iret_interception(svm); > > > > ++vcpu->stat.nmi_injections; > > > > } > > > > > > > > @@ -3614,12 +3626,10 @@ static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) > > > > > > > > if (masked) { > > > > svm->nmi_masked = true; > > > > - if (!sev_es_guest(vcpu->kvm)) > > > > - svm_set_intercept(svm, INTERCEPT_IRET); > > > > + svm_enable_iret_interception(svm); > > > > } else { > > > > svm->nmi_masked = false; > > > > - if (!sev_es_guest(vcpu->kvm)) > > > > - svm_clr_intercept(svm, INTERCEPT_IRET); > > > > + svm_disable_iret_interception(svm); > > > > } > > > > } > > > >
On Thu, Dec 08, 2022, Maxim Levitsky wrote: > On Thu, 2022-12-08 at 17:39 +0530, Santosh Shukla wrote: > > > > On 12/6/2022 5:44 PM, Maxim Levitsky wrote: > > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > > > > index 512b2aa21137e2..cfed6ab29c839a 100644 > > > > > --- a/arch/x86/kvm/svm/svm.c > > > > > +++ b/arch/x86/kvm/svm/svm.c > > > > > @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) > > > > > has_error_code, error_code); > > > > > } > > > > > > > > > > +static void svm_disable_iret_interception(struct vcpu_svm *svm) > > > > > +{ > > > > > + if (!sev_es_guest(svm->vcpu.kvm)) > > > > > + svm_clr_intercept(svm, INTERCEPT_IRET); > > > > > +} > > > > > + > > > > > +static void svm_enable_iret_interception(struct vcpu_svm *svm) > > > > > +{ > > > > > + if (!sev_es_guest(svm->vcpu.kvm)) > > > > > + svm_set_intercept(svm, INTERCEPT_IRET); > > > > > +} > > > > > + > > > > > > > > nits: > > > > s/_iret_interception / _iret_intercept > > > > does that make sense? > > > > > > Makes sense. I would rather go with svm_{clr,set}_iret_intercept(). I don't particularly like the SVM naming scheme, but I really dislike inconsistent naming. If we want to clean up naming, I would love unify VMX and SVM nomenclature for things like this. > > > I can also move this to svm.h near the svm_set_intercept(), I think > > > it better a better place for this function there if no objections. > > > > > I think current approach is fine since function used in svm.c only. but I have > > no strong opinion on moving to svm.h either ways. > > I also think so, just noticed something in case there are any objections. My vote is to keep it in svm.c unless we anticipate usage outside of svm.h. Keeping the implementation close to the usage makes it easer to understand what's going on, especially for something like this where there's a bit of "hidden" logic for SEV-ES.
On 2/1/2023 2:37 AM, Sean Christopherson wrote: > On Thu, Dec 08, 2022, Maxim Levitsky wrote: >> On Thu, 2022-12-08 at 17:39 +0530, Santosh Shukla wrote: >>> >>> On 12/6/2022 5:44 PM, Maxim Levitsky wrote: >>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c >>>>>> index 512b2aa21137e2..cfed6ab29c839a 100644 >>>>>> --- a/arch/x86/kvm/svm/svm.c >>>>>> +++ b/arch/x86/kvm/svm/svm.c >>>>>> @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) >>>>>> has_error_code, error_code); >>>>>> } >>>>>> >>>>>> +static void svm_disable_iret_interception(struct vcpu_svm *svm) >>>>>> +{ >>>>>> + if (!sev_es_guest(svm->vcpu.kvm)) >>>>>> + svm_clr_intercept(svm, INTERCEPT_IRET); >>>>>> +} >>>>>> + >>>>>> +static void svm_enable_iret_interception(struct vcpu_svm *svm) >>>>>> +{ >>>>>> + if (!sev_es_guest(svm->vcpu.kvm)) >>>>>> + svm_set_intercept(svm, INTERCEPT_IRET); >>>>>> +} >>>>>> + >>>>> >>>>> nits: >>>>> s/_iret_interception / _iret_intercept >>>>> does that make sense? >>>> >>>> Makes sense. > > I would rather go with svm_{clr,set}_iret_intercept(). I don't particularly like ok. > the SVM naming scheme, but I really dislike inconsistent naming. If we want to > clean up naming, I would love unify VMX and SVM nomenclature for things like this. > >>>> I can also move this to svm.h near the svm_set_intercept(), I think >>>> it better a better place for this function there if no objections. >>>> >>> I think current approach is fine since function used in svm.c only. but I have >>> no strong opinion on moving to svm.h either ways. >> >> I also think so, just noticed something in case there are any objections. > > My vote is to keep it in svm.c unless we anticipate usage outside of svm.h. Keeping ok. Thanks, Santosh > the implementation close to the usage makes it easer to understand what's going on, > especially for something like this where there's a bit of "hidden" logic for SEV-ES.
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 512b2aa21137e2..cfed6ab29c839a 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -2468,16 +2468,29 @@ static int task_switch_interception(struct kvm_vcpu *vcpu) has_error_code, error_code); } +static void svm_disable_iret_interception(struct vcpu_svm *svm) +{ + if (!sev_es_guest(svm->vcpu.kvm)) + svm_clr_intercept(svm, INTERCEPT_IRET); +} + +static void svm_enable_iret_interception(struct vcpu_svm *svm) +{ + if (!sev_es_guest(svm->vcpu.kvm)) + svm_set_intercept(svm, INTERCEPT_IRET); +} + static int iret_interception(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); ++vcpu->stat.nmi_window_exits; svm->awaiting_iret_completion = true; - if (!sev_es_guest(vcpu->kvm)) { - svm_clr_intercept(svm, INTERCEPT_IRET); + + svm_disable_iret_interception(svm); + if (!sev_es_guest(vcpu->kvm)) svm->nmi_iret_rip = kvm_rip_read(vcpu); - } + kvm_make_request(KVM_REQ_EVENT, vcpu); return 1; } @@ -3470,8 +3483,7 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu) return; svm->nmi_masked = true; - if (!sev_es_guest(vcpu->kvm)) - svm_set_intercept(svm, INTERCEPT_IRET); + svm_enable_iret_interception(svm); ++vcpu->stat.nmi_injections; } @@ -3614,12 +3626,10 @@ static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked) if (masked) { svm->nmi_masked = true; - if (!sev_es_guest(vcpu->kvm)) - svm_set_intercept(svm, INTERCEPT_IRET); + svm_enable_iret_interception(svm); } else { svm->nmi_masked = false; - if (!sev_es_guest(vcpu->kvm)) - svm_clr_intercept(svm, INTERCEPT_IRET); + svm_disable_iret_interception(svm); } }
SEV-ES guests don't use IRET interception for the detection of an end of a NMI. Therefore it makes sense to create a wrapper to avoid repeating the check for the SEV-ES. No functional change is intended. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/svm.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)