linux-kernel - Re: [PATCH v3 11/12] KVM: SVM: call avic_vcpu_load/avic_vcpu

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5a3b940b-7290-ca37-b3a4-bf01b7c4a6bb@redhat.com>
Date:   Tue, 3 Aug 2021 11:00:10 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org
Cc:     Wanpeng Li <wanpengli@...cent.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Joerg Roedel <joro@...tes.org>, Borislav Petkov <bp@...en8.de>,
        Sean Christopherson <seanjc@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v3 11/12] KVM: SVM: call avic_vcpu_load/avic_vcpu_put when
 enabling/disabling AVIC

On 02/08/21 20:33, Maxim Levitsky wrote:
> @@ -651,6 +673,7 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>   	}
>   	vmcb_mark_dirty(vmcb, VMCB_AVIC);
>   
> +	__avic_set_running(vcpu, activated);
>   	svm_set_pi_irte_mode(vcpu, activated);
>   }
>   

I'd rather have calls to avic_vcpu_load/avic_vcpu_put directly inside 
the "if (activated)", and leaving avic_set_running to its current 
implementation.  That way you don't need __avic_set_running (which is a 
confusing name, because it does more than just setting the running bit).

>  void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>  {
> +	bool activate;
> +
>  	if (!lapic_in_kernel(vcpu))
>  		return;
>  
>  	mutex_lock(&vcpu->kvm->arch.apicv_update_lock);
>  
> -	vcpu->arch.apicv_active = kvm_apicv_activated(vcpu->kvm);
> +	activate = kvm_apicv_activated(vcpu->kvm);
> +	if (vcpu->arch.apicv_active == activate)
> +		goto out;
> +
> +	vcpu->arch.apicv_active = activate;
>  	kvm_apic_update_apicv(vcpu);
>  	static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
>  
> @@ -9257,6 +9263,7 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>  	if (!vcpu->arch.apicv_active)
>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
>  
> +out:
>  	mutex_unlock(&vcpu->kvm->arch.apicv_update_lock);
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);

Should this be a separate patch?

As an aside, we have

static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu)
{
         return vcpu->arch.apic && vcpu->arch.apicv_active;
}

but really vcpu->arch.apicv_active should never be true if 
vcpu->arch.apic is.  So it should be possible to change this to "return 
vcpu->arch.apicv_active" with a comment that the serialization between 
apicv_inhibit_reasons and apicv_active happens via apicv_update_lock.

Thanks,

Paolo