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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +    {
> +        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.

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

> +                __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.

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.

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