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

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

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?

Best regards,
Yang



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