[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@xxxxxxxxx> 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) {} which will avoid any (indentation only) changes past the first of the two else-if-s. Plus it would make the logic of the code more clear, at once likely making apparent that there'll now be quite a few "goto out"-s that ought to be check for being replaceable by fewer instances of them placed slightly differently. >> > +#define UPDATE_EOI_EXITMAP(v, e) { \ >> > + if (test_and_clear_bit(e, &(v).eoi_exitmap_changed)) { \ >> >> Here and elsewhere - do you really need locked accesses? > Do you mean using the '__test_and_set_bit' ? Yes, if suitable. >> Furthermore, here and in other places you fail to write the upper halves for >> 32-bit, yet you also don't disable the code for 32-bit afaics. > OK, __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) ;would be > __vmwrite(EOI_EXIT_BITMAP##e, (v).eoi_exit_bitmap[e]) > #ifdef __i386__ > __vmwrite(EOI_EXIT_BITMAP##e##_HIGH, (v).eoi_exit_bitmap[e] >> 32) > #endif Something along those lines, yes. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |