Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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