From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [PATCH] kvm: x86: remove vmx_vm_has_apicv() outside of hwapic_isr_update()
Date: Fri, 19 Dec 2014 10:32:47 +0800
Message-ID: <54938E4F.7060105@intel.com>
References: <1417426124-9685-1-git-send-email-tiejun.chen@intel.com> <547C5477.7030101@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga01.intel.com ([192.55.52.88]:30445 "EHLO mga01.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751196AbaLSCcw (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 18 Dec 2014 21:32:52 -0500
In-Reply-To: <547C5477.7030101@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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
>