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

[Christoph Egger <Christoph.Egger@amd.com>]

On 09/06/12 12:35, Jan Beulich wrote:

>>>> On 06.09.12 at 12:00, "Li, Jiongxi" <jiongxi.li@intel.com> 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)
>   {}


I think, move the VMX part into hvm/vmx/* and call a function pointer
from common code. Having VMX or SVM specific code in common code
is broken by design.

Christoph

-- 

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632