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

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

Sorry for the late response.

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Jan Beulich
> Sent: Friday, August 31, 2012 8:31 PM
> To: Li, Jiongxi
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> >>> On 31.08.12 at 11:30, "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). ' 
vlapic->regs->data[APIC_TMR]' set by 'vlapic_set_irq' should be clear here, or 
the interrupt of the same vector will be always treated as level trigger even 
it becomes edge trigger later.
> 
> > +    {
> > +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> > +    }
> > +
> > +    hvm_dpci_msi_eoi(current->domain, vector); }
> >...
> > --- 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?

> 
> >
> >          intack = hvm_vcpu_ack_pending_irq(v, intack); @@ -253,6
> > +277,29 @@ void vmx_intr_assist(void)
> >      {
> >          hvm_inject_hw_exception(TRAP_machine_check,
> HVM_DELIVER_NO_ERROR_CODE);
> >      }
> > +    else if ( cpu_has_vmx_virtual_intr_delivery &&
> > +              intack.source != hvm_intsrc_pic &&
> > +              intack.source != hvm_intsrc_vector )
> > +    {
> > +        /* we need update the RVI field */
> > +        unsigned long status = __vmread(GUEST_INTR_STATUS);
> > +        status &= ~(unsigned long)0x0FF;
> > +        status |= (unsigned long)0x0FF &
> > +                    intack.vector;
> > +        __vmwrite(GUEST_INTR_STATUS, status);
> > +        if (v->arch.hvm_vmx.eoi_exitmap_changed) {
> > +#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' ?
> 
> > +                __vmwrite(EOI_EXIT_BITMAP##e,
> > + (v).eoi_exit_bitmap[e]);}}
> > +
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 0);
> 
> This is not very logical: Passing just 'v' to the macro, and accessing the 
> full field
> there would be more consistent.
OK, I will modify the passing just 'v' to the macro and modify the macro
> 
> 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
> 
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 1);
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 2);
> > +                UPDATE_EOI_EXITMAP(v->arch.hvm_vmx, 3);
> > +        }
> > +
> > +        pt_intr_post(v, intack);
> > +    }
> >      else
> >      {
> >          HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel