kvm: x86: remove vmx_vm_has_apicv() outside of hwapic_isr_update()

In most cases calling hwapic_isr_update(), actually we always check if kvm_apic_vid_enabled() == 1, and also actually, kvm_apic_vid_enabled()

    -> kvm_x86_ops->vm_has_apicv()
        -> vmx_vm_has_apicv() or '0' in svm case

So its unnecessary to recall this inside hwapic_isr_update(), here just remove vmx_vm_has_apicv() out and follow others.

Signed-off-by Tiejun Chen <[email protected]>

---
 arch/x86/kvm/lapic.c | 3 ++-
 arch/x86/kvm/vmx.c   | 3 ---
 2 files changed, 2 insertions(+), 4 deletions(-)

arch/x86/kvm/lapic.c +2 -1

index e0e5642..2ddc426 100644
@@ -1739,7 +1739,8 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu,
 	if (kvm_x86_ops->hwapic_irr_update)
 		kvm_x86_ops->hwapic_irr_update(vcpu,
 				apic_find_highest_irr(apic));
-	kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
+	if (kvm_apic_vid_enabled(vcpu->kvm))
+	    kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	kvm_rtc_eoi_tracking_restore_one(vcpu);
 }

arch/x86/kvm/vmx.c +0 -3

index 6a951d8..f0c16a9 100644
@@ -7406,9 +7406,6 @@ static void vmx_hwapic_isr_update(struct kvm *kvm, int isr)
 	u16 status;
 	u8 old;
 
-	if (!vmx_vm_has_apicv(kvm))
-		return;
-
 	if (isr == -1)
 		isr = 0;
 

Quoted 6 lines · Tiejun Chen

In most cases calling hwapic_isr_update(), actually we always check if kvm_apic_vid_enabled() == 1, and also actually, kvm_apic_vid_enabled()

     -> kvm_x86_ops->vm_has_apicv()
         -> vmx_vm_has_apicv() or '0' in svm case

Quoted 2 lines

So its unnecessary to recall this inside hwapic_isr_update(), here just remove vmx_vm_has_apicv() out and follow others.

If you want to do this, please NULL out the function pointer instead, as KVM already does for hwapic_irr_update.

Paolo

Quoted 26 lines

Signed-off-by Tiejun Chen <[email protected]>

---
  arch/x86/kvm/lapic.c | 3 ++-
  arch/x86/kvm/vmx.c   | 3 ---
  2 files changed, 2 insertions(+), 4 deletions(-)

arch/x86/kvm/lapic.c +2 -1

index e0e5642..2ddc426 100644
@@ -1739,7 +1739,8 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu,
  	if (kvm_x86_ops->hwapic_irr_update)
  		kvm_x86_ops->hwapic_irr_update(vcpu,
  				apic_find_highest_irr(apic));
-	kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
+	if (kvm_apic_vid_enabled(vcpu->kvm))
+	    kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
  	kvm_make_request(KVM_REQ_EVENT, vcpu);
  	kvm_rtc_eoi_tracking_restore_one(vcpu);
  }

arch/x86/kvm/vmx.c +0 -0

index 6a951d8..f0c16a9 100644

arch/x86/kvm/vmx.c static void vmx_hwapic_isr_update(struct kvm *kvm, int isr) · line 7411

Quoted 9 lines

arch/x86/kvm/vmx.c +0 -3

@@ -7406,9 +7406,6 @@ static void vmx_hwapic_isr_update(struct kvm *kvm, int isr)
  	u16 status;
  	u8 old;
 
-	if (!vmx_vm_has_apicv(kvm))
-		return;
-
  	if (isr == -1)
  		isr = 0;
 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Are you saying something below?

if (enable_apicv)

    ...
else {
    kvm_x86_ops->hwapic_irr_update = NULL;

But this means we have to revise hadware_setup() to get 'kvm' inside,

then rebase other callers to hwapic_isr_update(), is it really good?

Quoted 7 lines · Tiejun

Are you saying something below?

if (enable_apicv)

     ...
else {
     kvm_x86_ops->hwapic_irr_update = NULL;

Yes.

Quoted 1 line

But this means we have to revise hadware_setup() to get 'kvm' inside,

This would not even be possible, since hardware_setup() is only called once.

However, for the only caller of hwapic_isr_update (but presumably all of them, as is the case for hwapic_irr_update), you already know that

irqchip_in_kernel(kvm) is true. You are in kvm_apic_post_state_restore, which takes a kvm_lapic_state, and no lapic exists if !irqchip_in_kernel(kvm).

(Yes, irqchip_in_kernel) is a bit weird and tests pic_irqchip(kvm) instead, but it's the same. It tests pic_irqchip(kvm) only because the LAPIC is per-cpu and irqchip_in_kernel takes a struct kvm).

So it's possible to NULL out hwapic_isr_update in hardware_setup. It simply shouldn't happen that you call hwapic_isr_update without the in-kernel irqchip. The kernel knows nothing about ISR/IRR without the in-kernel irqchip.

I can send out as a patch if we have on any objections.