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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2012-08-31:
>>>> 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?

The current logic requires to clear it when guest writes EOI. When VCPU 
received an interrupt, it set the TMR if it is a level trigger interrupt and do 
nothing if it is edge triggered(it assumes the corresponding bit in TMR is 
clear).

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


Best regards,
Yang


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