linux-kernel - Re: [PATCH 2/5] KVM: x86: Drop @vcpu parameter from kvm_x86_ops.hwapic_isr

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]

Date:   Wed, 15 Jun 2022 15:15:56 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] KVM: x86: Drop @vcpu parameter from
 kvm_x86_ops.hwapic_isr_update()

On 6/15/22 01:05, Sean Christopherson wrote:
> Drop the unused @vcpu parameter from hwapic_isr_update().  AMD/AVIC is
> unlikely to implement the helper, and VMX/APICv doesn't need the vCPU as
> it operates on the current VMCS.  The result is somewhat odd, but allows
> for a decent amount of (future) cleanup in the APIC code.
> 
> No functional change intended.

Yeah, that's a bit odd; what it saves is essentially the apic->vcpu 
dereference.  I don't really like it, so if you want to have a v2 that 
passes the struct kvm_lapic* instead (which is free and keeps irr/isr 
functions consistent), I'll gladly switch.  But I _have_ queued the 
series in the meanwhile, so that's a good reason to ignore me.

Paolo

> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>   arch/x86/include/asm/kvm_host.h | 2 +-
>   arch/x86/kvm/lapic.c            | 8 ++++----
>   arch/x86/kvm/vmx/vmx.c          | 2 +-
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 7e98b2876380..16acc54d49a7 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1517,7 +1517,7 @@ struct kvm_x86_ops {
>   	bool (*check_apicv_inhibit_reasons)(enum kvm_apicv_inhibit reason);
>   	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
>   	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
> -	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
> +	void (*hwapic_isr_update)(int isr);
>   	bool (*guest_apic_has_interrupt)(struct kvm_vcpu *vcpu);
>   	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
>   	void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index a413a1d8df4c..cc0da5671eb9 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -556,7 +556,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
>   	 * just set SVI.
>   	 */
>   	if (unlikely(vcpu->arch.apicv_active))
> -		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, vec);
> +		static_call_cond(kvm_x86_hwapic_isr_update)(vec);
>   	else {
>   		++apic->isr_count;
>   		BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> @@ -604,7 +604,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
>   	 * and must be left alone.
>   	 */
>   	if (unlikely(vcpu->arch.apicv_active))
> -		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
> +		static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
>   	else {
>   		--apic->isr_count;
>   		BUG_ON(apic->isr_count < 0);
> @@ -2457,7 +2457,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
>   	if (vcpu->arch.apicv_active) {
>   		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
>   		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, -1);
> -		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, -1);
> +		static_call_cond(kvm_x86_hwapic_isr_update)(-1);
>   	}
>   
>   	vcpu->arch.apic_arb_prio = 0;
> @@ -2737,7 +2737,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
>   	if (vcpu->arch.apicv_active) {
>   		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
>   		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
> -		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
> +		static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
>   	}
>   	kvm_make_request(KVM_REQ_EVENT, vcpu);
>   	if (ioapic_in_kernel(vcpu->kvm))
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5e14e4c40007..42f8924a90f4 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6556,7 +6556,7 @@ static void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu)
>   	put_page(page);
>   }
>   
> -static void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
> +static void vmx_hwapic_isr_update(int max_isr)
>   {
>   	u16 status;
>   	u8 old;