From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [ PATCH 2/2] xen: enable Virtual-interrupt delivery Date: Thu, 6 Sep 2012 13:03:45 +0200 Message-ID: <50488311.7020702@amd.com> References: <5040CAB30200007800097D73@nat28.tlf.novell.com> <504898870200007800099427@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <504898870200007800099427@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Jiongxi Li , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 09/06/12 12:35, Jan Beulich wrote: >>>> On 06.09.12 at 12:00, "Li, Jiongxi" wrote: >>>> +/* >>>> + * When "Virtual Interrupt Delivery" is enabled, this function is >>>> +used >>>> + * to handle EOI-induced VM exit >>>> + */ >>>> +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int >>>> +vector) { >>>> + ASSERT(cpu_has_vmx_virtual_intr_delivery); >>>> + >>>> + if ( vlapic_test_and_clear_vector(vector, >>>> + &vlapic->regs->data[APIC_TMR]) ) >>> >>> Why test_and_clear rather than just test? >> While virtual interrupt delivery is enabled, 'vlapic_EOI_set' function won't >> be called( 'vlapic_EOI_set' also has the test and clear call). ' > > Ah, okay. > >>>> --- a/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:30:38 2012 +0800 >>>> +++ b/xen/arch/x86/hvm/vmx/intr.c Fri Aug 31 09:49:39 2012 +0800 >>>> @@ -227,19 +227,43 @@ void vmx_intr_assist(void) >>>> goto out; >>>> >>>> intblk = hvm_interrupt_blocked(v, intack); >>>> - if ( intblk == hvm_intblk_tpr ) >>>> + if ( cpu_has_vmx_virtual_intr_delivery ) >>>> { >>>> - ASSERT(vlapic_enabled(vcpu_vlapic(v))); >>>> - ASSERT(intack.source == hvm_intsrc_lapic); >>>> - tpr_threshold = intack.vector >> 4; >>>> - goto out; >>>> + /* Set "Interrupt-window exiting" for ExtINT */ >>>> + if ( (intblk != hvm_intblk_none) && >>>> + ( (intack.source == hvm_intsrc_pic) || >>>> + ( intack.source == hvm_intsrc_vector) ) ) >>>> + { >>>> + enable_intr_window(v, intack); >>>> + goto out; >>>> + } >>>> + >>>> + if ( __vmread(VM_ENTRY_INTR_INFO) & >>> INTR_INFO_VALID_MASK ) >>>> + { >>>> + if ( (intack.source == hvm_intsrc_pic) || >>>> + (intack.source == hvm_intsrc_nmi) || >>>> + (intack.source == hvm_intsrc_mce) ) >>>> + enable_intr_window(v, intack); >>>> + >>>> + goto out; >>>> + } >>>> } >>>> + else >>>> + { >>>> + if ( intblk == hvm_intblk_tpr ) >>>> + { >>>> + ASSERT(vlapic_enabled(vcpu_vlapic(v))); >>>> + ASSERT(intack.source == hvm_intsrc_lapic); >>>> + tpr_threshold = intack.vector >> 4; >>>> + goto out; >>>> + } >>>> >>>> - if ( (intblk != hvm_intblk_none) || >>>> - (__vmread(VM_ENTRY_INTR_INFO) & >>> INTR_INFO_VALID_MASK) ) >>>> - { >>>> - enable_intr_window(v, intack); >>>> - goto out; >>>> + if ( (intblk != hvm_intblk_none) || >>>> + (__vmread(VM_ENTRY_INTR_INFO) & >>> INTR_INFO_VALID_MASK) ) >>>> + { >>>> + enable_intr_window(v, intack); >>>> + goto out; >>>> + } >>>> } >>> >>> If you made the above and if()/else if() series, some of the differences >> would go >>> away, making the changes easier to review. >> I can't quite understand you here. >> The original code looked like: >> if (a) >> {} >> if (b) >> {} >> And my change as follow: >> if ( cpu_has_vmx_virtual_intr_delivery ) >> { >> } >> else >> { >> if (a) >> {} >> if (b) >> {} >> } >> How should I adjust the code? > > Considering that the original could already have been written > with if/else-if, I was suggesting to expand this to your addition: > > if ( cpu_has_vmx_virtual_intr_delivery ) > { > } > else if (a) > {} > else if (b) > {} I think, move the VMX part into hvm/vmx/* and call a function pointer from common code. Having VMX or SVM specific code in common code is broken by design. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632