linux-kernel - Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d3f2da59b5afd300531ae428174c1f91d731e655.camel@redhat.com>
Date:   Tue, 07 Jun 2022 16:07:13 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Santosh Shukla <santosh.shukla@....com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Tom Lendacky <thomas.lendacky@....com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> VMCB intr_ctrl bit12 (V_NMI_MASK) is set by the processor when handling
> NMI in guest and is cleared after the NMI is handled. Treat V_NMI_MASK as
> read-only in the hypervisor and do not populate set accessors.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@....com>
> ---
>  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 860f28c668bd..d67a54517d95 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
>         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
>  }
>  
> +static bool is_vnmi_enabled(struct vmcb *vmcb)
> +{
> +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
> +}

Following Paolo's suggestion I recently removed vgif_enabled(),
based on the logic that vgif_enabled == vgif, because
we always enable vGIF for L1 as long as 'vgif' module param is set,
which is set unless either hardware or user cleared it.

Note that here vmcb is the current vmcb, which can be vmcb02,
and it might be wrong

> +
> +static bool is_vnmi_mask_set(struct vmcb *vmcb)
> +{
> +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
> +}
> +
>  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3502,13 +3512,21 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu, bool for_injection)
>  
>  static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
>  {
> -       return !!(vcpu->arch.hflags & HF_NMI_MASK);
> +       struct vcpu_svm *svm = to_svm(vcpu);
> +
> +       if (is_vnmi_enabled(svm->vmcb))
> +               return is_vnmi_mask_set(svm->vmcb);
> +       else
> +               return !!(vcpu->arch.hflags & HF_NMI_MASK);
>  }
>  
>  static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
>  
> +       if (is_vnmi_enabled(svm->vmcb))
> +               return;

What if the KVM wants to mask NMI, shoudn't we update the 
V_NMI_MASK value in int_ctl instead of doing nothing?

Best regards,
	Maxim Levitsky


> +
>         if (masked) {
>                 vcpu->arch.hflags |= HF_NMI_MASK;
>                 if (!sev_es_guest(vcpu->kvm))