diff mbox series

[v2] KVM: x86: fix kvm_has_noapic_vcpu updates when fail to create vcpu

Message ID 20231123010424.10274-1-lirongqing@baidu.com (mailing list archive)
State New, archived
Headers show
Series [v2] KVM: x86: fix kvm_has_noapic_vcpu updates when fail to create vcpu | expand

Commit Message

Li RongQing Nov. 23, 2023, 1:04 a.m. UTC 
Static key kvm_has_noapic_vcpu should be reduced when fail to create
vcpu, opportunistically change to call kvm_free_lapic only when LAPIC
is in kernel in kvm_arch_vcpu_destroy

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---

Comments

Sean Christopherson Dec. 1, 2023, 4:57 p.m. UTC | #1 
On Thu, Nov 23, 2023, Li RongQing wrote:
> Static key kvm_has_noapic_vcpu should be reduced when fail to create
> vcpu, opportunistically change to call kvm_free_lapic only when LAPIC
> is in kernel in kvm_arch_vcpu_destroy

Heh, this has been on my todo list for a comically long time.

> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> diff v1: call kvm_free_lapic conditionally in kvm_arch_vcpu_destroy
> 
>  arch/x86/kvm/x86.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2c92407..3cadf28 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12079,7 +12079,10 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	kfree(vcpu->arch.mci_ctl2_banks);
>  	free_page((unsigned long)vcpu->arch.pio_data);
>  fail_free_lapic:
> -	kvm_free_lapic(vcpu);
> +	if (lapic_in_kernel(vcpu))
> +		kvm_free_lapic(vcpu);
> +	else
> +		static_branch_dec(&kvm_has_noapic_vcpu);
>  fail_mmu_destroy:
>  	kvm_mmu_destroy(vcpu);
>  	return r;
> @@ -12122,14 +12125,17 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>  	kvm_pmu_destroy(vcpu);
>  	kfree(vcpu->arch.mce_banks);
>  	kfree(vcpu->arch.mci_ctl2_banks);
> -	kvm_free_lapic(vcpu);
> +
> +	if (lapic_in_kernel(vcpu))
> +		kvm_free_lapic(vcpu);
> +	else
> +		static_branch_dec(&kvm_has_noapic_vcpu);

Rather than split code like this, what if we let the APIC code deal with bumping
the static branch?  The effect of this bug is purely just loss of an optimization
that has *very* dubious value to begin with, i.e. having a minimal diff isn't a
priority.  lapic.h already declares kvm_has_noapic_vcpu, so this would make lapic.*
the sole owner of the code.

E.g. (untested)

---
 arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c   | 29 +++--------------------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f5fab29c827f..45ffc7d1e49e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -124,6 +124,9 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
 	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
 }
 
+__read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
+EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
+
 __read_mostly DEFINE_STATIC_KEY_DEFERRED_FALSE(apic_hw_disabled, HZ);
 __read_mostly DEFINE_STATIC_KEY_DEFERRED_FALSE(apic_sw_disabled, HZ);
 
@@ -2467,8 +2470,10 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu->arch.apic)
+	if (!vcpu->arch.apic) {
+		static_branch_dec(&kvm_has_noapic_vcpu);
 		return;
+	}
 
 	hrtimer_cancel(&apic->lapic_timer.timer);
 
@@ -2810,6 +2815,11 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
 
 	ASSERT(vcpu != NULL);
 
+	if (!irqchip_in_kernel(vcpu->kvm)) {
+		static_branch_inc(&kvm_has_noapic_vcpu);
+		return 0;
+	}
+
 	apic = kzalloc(sizeof(*apic), GFP_KERNEL_ACCOUNT);
 	if (!apic)
 		goto nomem;
@@ -2845,6 +2855,21 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
 	static_branch_inc(&apic_sw_disabled.key); /* sw disabled at reset */
 	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
 
+	/*
+	 * Defer evaluating inhibits until the vCPU is first run, as this vCPU
+	 * will not get notified of any changes until this vCPU is visible to
+	 * other vCPUs (marked online and added to the set of vCPUs).
+	 *
+	 * Opportunistically mark APICv active as VMX in particularly is highly
+	 * unlikely to have inhibits.  Ignore the current per-VM APICv state so
+	 * that vCPU creation is guaranteed to run with a deterministic value,
+	 * the request will ensure the vCPU gets the correct state before VM-Entry.
+	 */
+	if (enable_apicv) {
+		apic->apicv_active = true;
+		kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
+	}
+
 	return 0;
 nomem_free_apic:
 	kfree(apic);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0572172f2e94..7d7d65c60276 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12014,27 +12014,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	if (r < 0)
 		return r;
 
-	if (irqchip_in_kernel(vcpu->kvm)) {
-		r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
-		if (r < 0)
-			goto fail_mmu_destroy;
-
-		/*
-		 * Defer evaluating inhibits until the vCPU is first run, as
-		 * this vCPU will not get notified of any changes until this
-		 * vCPU is visible to other vCPUs (marked online and added to
-		 * the set of vCPUs).  Opportunistically mark APICv active as
-		 * VMX in particularly is highly unlikely to have inhibits.
-		 * Ignore the current per-VM APICv state so that vCPU creation
-		 * is guaranteed to run with a deterministic value, the request
-		 * will ensure the vCPU gets the correct state before VM-Entry.
-		 */
-		if (enable_apicv) {
-			vcpu->arch.apic->apicv_active = true;
-			kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
-		}
-	} else
-		static_branch_inc(&kvm_has_noapic_vcpu);
+	r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
+	if (r < 0)
+		goto fail_mmu_destroy;
 
 	r = -ENOMEM;
 
@@ -12155,8 +12137,6 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 	free_page((unsigned long)vcpu->arch.pio_data);
 	kvfree(vcpu->arch.cpuid_entries);
-	if (!lapic_in_kernel(vcpu))
-		static_branch_dec(&kvm_has_noapic_vcpu);
 }
 
 void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
@@ -12433,9 +12413,6 @@ bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.apic_base & MSR_IA32_APICBASE_BSP) != 0;
 }
 
-__read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
-EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
-
 void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);

base-commit: 1d4405b36808dc8c2d9b65b627c2af4b62fe017e
--
Li RongQing Dec. 4, 2023, 9:13 a.m. UTC | #2 
> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Saturday, December 2, 2023 12:58 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: x86@kernel.org; kvm@vger.kernel.org; mlevitsk@redhat.com;
> yilun.xu@linux.intel.com
> Subject: Re: [PATCH v2] KVM: x86: fix kvm_has_noapic_vcpu updates when fail
> to create vcpu
> 
> On Thu, Nov 23, 2023, Li RongQing wrote:
> > Static key kvm_has_noapic_vcpu should be reduced when fail to create
> > vcpu, opportunistically change to call kvm_free_lapic only when LAPIC
> > is in kernel in kvm_arch_vcpu_destroy
> 
> Heh, this has been on my todo list for a comically long time.
> 
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > diff v1: call kvm_free_lapic conditionally in kvm_arch_vcpu_destroy
> >
> >  arch/x86/kvm/x86.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> > 2c92407..3cadf28 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -12079,7 +12079,10 @@ int kvm_arch_vcpu_create(struct kvm_vcpu
> *vcpu)
> >  	kfree(vcpu->arch.mci_ctl2_banks);
> >  	free_page((unsigned long)vcpu->arch.pio_data);
> >  fail_free_lapic:
> > -	kvm_free_lapic(vcpu);
> > +	if (lapic_in_kernel(vcpu))
> > +		kvm_free_lapic(vcpu);
> > +	else
> > +		static_branch_dec(&kvm_has_noapic_vcpu);
> >  fail_mmu_destroy:
> >  	kvm_mmu_destroy(vcpu);
> >  	return r;
> > @@ -12122,14 +12125,17 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu
> *vcpu)
> >  	kvm_pmu_destroy(vcpu);
> >  	kfree(vcpu->arch.mce_banks);
> >  	kfree(vcpu->arch.mci_ctl2_banks);
> > -	kvm_free_lapic(vcpu);
> > +
> > +	if (lapic_in_kernel(vcpu))
> > +		kvm_free_lapic(vcpu);
> > +	else
> > +		static_branch_dec(&kvm_has_noapic_vcpu);
> 
> Rather than split code like this, what if we let the APIC code deal with bumping
> the static branch?  

I am fine, thanks

-Li
Xu Yilun Dec. 14, 2023, 3:04 p.m. UTC | #3 
On Fri, Dec 01, 2023 at 08:57:33AM -0800, Sean Christopherson wrote:
> On Thu, Nov 23, 2023, Li RongQing wrote:
> > Static key kvm_has_noapic_vcpu should be reduced when fail to create
> > vcpu, opportunistically change to call kvm_free_lapic only when LAPIC
> > is in kernel in kvm_arch_vcpu_destroy
> 
> Heh, this has been on my todo list for a comically long time.
> 
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > diff v1: call kvm_free_lapic conditionally in kvm_arch_vcpu_destroy
> > 
> >  arch/x86/kvm/x86.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2c92407..3cadf28 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -12079,7 +12079,10 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >  	kfree(vcpu->arch.mci_ctl2_banks);
> >  	free_page((unsigned long)vcpu->arch.pio_data);
> >  fail_free_lapic:
> > -	kvm_free_lapic(vcpu);
> > +	if (lapic_in_kernel(vcpu))
> > +		kvm_free_lapic(vcpu);
> > +	else
> > +		static_branch_dec(&kvm_has_noapic_vcpu);
> >  fail_mmu_destroy:
> >  	kvm_mmu_destroy(vcpu);
> >  	return r;
> > @@ -12122,14 +12125,17 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> >  	kvm_pmu_destroy(vcpu);
> >  	kfree(vcpu->arch.mce_banks);
> >  	kfree(vcpu->arch.mci_ctl2_banks);
> > -	kvm_free_lapic(vcpu);
> > +
> > +	if (lapic_in_kernel(vcpu))
> > +		kvm_free_lapic(vcpu);
> > +	else
> > +		static_branch_dec(&kvm_has_noapic_vcpu);
> 
> Rather than split code like this, what if we let the APIC code deal with bumping
> the static branch?  The effect of this bug is purely just loss of an optimization
> that has *very* dubious value to begin with, i.e. having a minimal diff isn't a
> priority.  lapic.h already declares kvm_has_noapic_vcpu, so this would make lapic.*
> the sole owner of the code.
> 
> E.g. (untested)
> 
> ---
>  arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++++++++-
>  arch/x86/kvm/x86.c   | 29 +++--------------------------
>  2 files changed, 29 insertions(+), 27 deletions(-)

This good to me.

Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>

> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index f5fab29c827f..45ffc7d1e49e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -124,6 +124,9 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
>  	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
>  }
>  
> +__read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
> +EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
> +
>  __read_mostly DEFINE_STATIC_KEY_DEFERRED_FALSE(apic_hw_disabled, HZ);
>  __read_mostly DEFINE_STATIC_KEY_DEFERRED_FALSE(apic_sw_disabled, HZ);
>  
> @@ -2467,8 +2470,10 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
> -	if (!vcpu->arch.apic)
> +	if (!vcpu->arch.apic) {
> +		static_branch_dec(&kvm_has_noapic_vcpu);
>  		return;
> +	}
>  
>  	hrtimer_cancel(&apic->lapic_timer.timer);
>  
> @@ -2810,6 +2815,11 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
>  
>  	ASSERT(vcpu != NULL);
>  
> +	if (!irqchip_in_kernel(vcpu->kvm)) {
> +		static_branch_inc(&kvm_has_noapic_vcpu);
> +		return 0;
> +	}
> +
>  	apic = kzalloc(sizeof(*apic), GFP_KERNEL_ACCOUNT);
>  	if (!apic)
>  		goto nomem;
> @@ -2845,6 +2855,21 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
>  	static_branch_inc(&apic_sw_disabled.key); /* sw disabled at reset */
>  	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
>  
> +	/*
> +	 * Defer evaluating inhibits until the vCPU is first run, as this vCPU
> +	 * will not get notified of any changes until this vCPU is visible to
> +	 * other vCPUs (marked online and added to the set of vCPUs).
> +	 *
> +	 * Opportunistically mark APICv active as VMX in particularly is highly
> +	 * unlikely to have inhibits.  Ignore the current per-VM APICv state so
> +	 * that vCPU creation is guaranteed to run with a deterministic value,
> +	 * the request will ensure the vCPU gets the correct state before VM-Entry.
> +	 */
> +	if (enable_apicv) {
> +		apic->apicv_active = true;
> +		kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> +	}
> +
>  	return 0;
>  nomem_free_apic:
>  	kfree(apic);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0572172f2e94..7d7d65c60276 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12014,27 +12014,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	if (r < 0)
>  		return r;
>  
> -	if (irqchip_in_kernel(vcpu->kvm)) {
> -		r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
> -		if (r < 0)
> -			goto fail_mmu_destroy;
> -
> -		/*
> -		 * Defer evaluating inhibits until the vCPU is first run, as
> -		 * this vCPU will not get notified of any changes until this
> -		 * vCPU is visible to other vCPUs (marked online and added to
> -		 * the set of vCPUs).  Opportunistically mark APICv active as
> -		 * VMX in particularly is highly unlikely to have inhibits.
> -		 * Ignore the current per-VM APICv state so that vCPU creation
> -		 * is guaranteed to run with a deterministic value, the request
> -		 * will ensure the vCPU gets the correct state before VM-Entry.
> -		 */
> -		if (enable_apicv) {
> -			vcpu->arch.apic->apicv_active = true;
> -			kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> -		}
> -	} else
> -		static_branch_inc(&kvm_has_noapic_vcpu);
> +	r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
> +	if (r < 0)
> +		goto fail_mmu_destroy;
>  
>  	r = -ENOMEM;
>  
> @@ -12155,8 +12137,6 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>  	free_page((unsigned long)vcpu->arch.pio_data);
>  	kvfree(vcpu->arch.cpuid_entries);
> -	if (!lapic_in_kernel(vcpu))
> -		static_branch_dec(&kvm_has_noapic_vcpu);
>  }
>  
>  void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> @@ -12433,9 +12413,6 @@ bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
>  	return (vcpu->arch.apic_base & MSR_IA32_APICBASE_BSP) != 0;
>  }
>  
> -__read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
> -EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
> -
>  void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> 
> base-commit: 1d4405b36808dc8c2d9b65b627c2af4b62fe017e
> -- 
> 
>
diff mbox series

Patch

diff v1: call kvm_free_lapic conditionally in kvm_arch_vcpu_destroy

 arch/x86/kvm/x86.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2c92407..3cadf28 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12079,7 +12079,10 @@  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	kfree(vcpu->arch.mci_ctl2_banks);
 	free_page((unsigned long)vcpu->arch.pio_data);
 fail_free_lapic:
-	kvm_free_lapic(vcpu);
+	if (lapic_in_kernel(vcpu))
+		kvm_free_lapic(vcpu);
+	else
+		static_branch_dec(&kvm_has_noapic_vcpu);
 fail_mmu_destroy:
 	kvm_mmu_destroy(vcpu);
 	return r;
@@ -12122,14 +12125,17 @@  void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_pmu_destroy(vcpu);
 	kfree(vcpu->arch.mce_banks);
 	kfree(vcpu->arch.mci_ctl2_banks);
-	kvm_free_lapic(vcpu);
+
+	if (lapic_in_kernel(vcpu))
+		kvm_free_lapic(vcpu);
+	else
+		static_branch_dec(&kvm_has_noapic_vcpu);
+
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	kvm_mmu_destroy(vcpu);
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 	free_page((unsigned long)vcpu->arch.pio_data);
 	kvfree(vcpu->arch.cpuid_entries);
-	if (!lapic_in_kernel(vcpu))
-		static_branch_dec(&kvm_has_noapic_vcpu);
 }
 
 void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)