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

["Li, Jiongxi" <jiongxi.li@xxxxxxxxx>]

A new patch is sent out according to review comment.
Also please see my comment interlaced

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Thursday, September 06, 2012 6:35 PM
> To: Li, Jiongxi
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: 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.
It is a good suggestion. But the original code is two parallel if() case, not 
the if/else-if case, and can't be changed to if/else-if case, so I just keep 
the original code here. :)

> 
> >> > +#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.
Since the parameter may also be changed in 'vlapic_set_irq' function, and that 
function may be called asynchronously, so a lock is needed here.

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