Re: [Xen-devel] [PATCH for 4.5 v2] vmx, apicv: fix save/restore issue with apicv

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.10.14 at 04:00, <yang.z.zhang@xxxxxxxxx> wrote:
> Jan Beulich wrote on 2014-10-21:
>>>>> On 21.10.14 at 07:32, <yang.z.zhang@xxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1584,6 +1584,8 @@ static void vmx_process_isr(int isr, struct
>>> vcpu
>>> *v)  {
>>>      unsigned long status;
>>>      u8 old;
>>> +    unsigned int i, vector;
>> 
>> I don't really see the need for two variables here - just "i" would
>> seem to suffice.
>> 
>>> +    struct vlapic *vlapic = vcpu_vlapic(v);
>> 
>> const?
>> 
>>> @@ -1597,6 +1599,28 @@ static void vmx_process_isr(int isr, struct
>>> vcpu
>> *v)
>>>          status |= isr << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
>>>          __vmwrite(GUEST_INTR_STATUS, status);
>>>      }
>>> +
>>> +    /*
>>> +     * Theoretically, only level triggered interrupts can have their
>>> +     * corresponding bits set in the eoi exit bitmap. That is, the bits
>>> +     * set in the eoi exit bitmap should also be set in TMR. But a periodic
>>> +     * timer interrupt does not follow the rule: it is edge triggered, but
>>> +     * requires its corresponding bit be set in the eoi exit bitmap. So we
>>> +     * should not construct the eoi exit bitmap based on TMR.
>>> +     * Here we will construct the eoi exit bitmap via (IRR | ISR). This
>>> +     * means that EOIs to the interrupts that are set in the IRR or ISR 
> will
>>> +     * cause VM exits after restoring, regardless of the trigger modes. It
>>> +     * is acceptable because the subsequent interrupts will set up the eoi
>>> +     * bitmap correctly.
>>> +     */
>>> +    for ( vector = 0x10; vector < NR_VECTORS; vector++ )
>>> +        if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_IRR]) ||
>>> +             vlapic_test_vector(vector, &vlapic->regs->data[APIC_ISR]) )
>>> +            set_bit(vector,  v->arch.hvm_vmx.eoi_exit_bitmap);
>>> +
>>> +    for ( i = 0; i < 4; i++ )
>> 
>> ARRAY_SIZE(v->arch.hvm_vmx.eoi_exit_bitmap) instead of 4 please, like
>> already done in construct_vmcs().
>> 
>>> +        __vmwrite(EOI_EXIT_BITMAP(i),
>>> + v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>> +
>>>      vmx_vmcs_exit(v);
>>>  }
>>> --- a/xen/include/asm-x86/hvm/vlapic.h
>>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>>> @@ -61,6 +61,8 @@
>>> 
>>>  #define VEC_POS(v) ((v) % 32)
>>>  #define REG_POS(v) (((v) / 32) * 0x10)
>>> +#define vlapic_test_vector(vec, bitmap) \ +    test_bit(VEC_POS(vec),
>>> (uint32_t *)((bitmap) + REG_POS(vec)))
>> 
>> Don't cast away possible const-ness here.
>> 
>> But of course if there are no other needs to change the patch I could
>> take care of all of these while committing.
> 
> It appears no other comments. Should I send out the v3 or you will help to 
> commit it with modifications?

As said above - I'm fine doing those minor adjustments, but I
certainly would like to wait for Citrix folks' confirmation. This has
been taking so long that I don't think waiting a few more days
really matters.

Jan


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