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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/10/14 01:00, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-10-24:
>>>>> On 24.10.14 at 04:00, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-10-21:
>>>>>>> On 21.10.14 at 07:32, <yang.z.zhang@intel.com> 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.
> I want to catch the first RC release since we will perform a full testing with first RC. But it is ok now since the first RC already released. 

With the changes Jan wants, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

We don't have much time for out-of-band testing at the moment, but I
will test it specifically as part of our 4.5-rc$N testing.  The patch
does look functionally very similar to the previous version which we
positively confirmed as fixing our observed issues.

~Andrew