| Message ID | 1417426124-9685-1-git-send-email-tiejun.chen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/12/2014 10:28, Tiejun Chen wrote: > 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. If you want to do this, please NULL out the function pointer instead, as KVM already does for hwapic_irr_update. Paolo > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > arch/x86/kvm/lapic.c | 3 ++- > arch/x86/kvm/vmx.c | 3 --- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index e0e5642..2ddc426 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -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); > } > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6a951d8..f0c16a9 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/12/1 19:43, Paolo Bonzini wrote: > > > On 01/12/2014 10:28, Tiejun Chen wrote: >> 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. > > If you want to do this, please NULL out the function pointer instead, as > KVM already does for hwapic_irr_update. Are you saying something below? if (enable_apicv) ... else { kvm_x86_ops->hwapic_irr_update = NULL; But there's a little bit difference to NULL out hwapic_isr_update(), static int vmx_vm_has_apicv(struct kvm *kvm) { return enable_apicv && irqchip_in_kernel(kvm); } Yes, I can do something like this, static __init int hadware_setup(void) { ... if (enable_apicv) { ... if (!irqchip_in_kernel(kvm)) kvm_x86_ops->hwapic_isr_update = NULL; } else { ... kvm_x86_ops->hwapic_isr_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? Here what I will intend to do is trying to reduce some cost (reduplicate check) with a little code, so its may not be worth changing much more. Tiejun > > Paolo > >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >> --- >> arch/x86/kvm/lapic.c | 3 ++- >> arch/x86/kvm/vmx.c | 3 --- >> 2 files changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index e0e5642..2ddc426 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -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); >> } >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6a951d8..f0c16a9 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/12/2014 03:32, Chen, Tiejun wrote: > > Are you saying something below? > > if (enable_apicv) > ... > else { > kvm_x86_ops->hwapic_irr_update = NULL; Yes. > 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. Paolo > then rebase other callers to hwapic_isr_update(), is it really good? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index e0e5642..2ddc426 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -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); } diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6a951d8..f0c16a9 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -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;
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 <tiejun.chen@intel.com> --- arch/x86/kvm/lapic.c | 3 ++- arch/x86/kvm/vmx.c | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-)