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

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

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

This patch fixes two issues:

1. Interrupts on PIR are lost during save/restore. Syncing the PIR
into IRR during save will fix it.

2. EOI exit bitmap doesn't set up correctly after restore. Here we
will construct the eoi exit bitmap via (IRR | ISR). Though it may cause
unnecessary eoi exit of the interrupts that pending in IRR or ISR during
save/restore, each pending interrupt only causes one vmexit. The
subsequent interrupts will adjust the eoi exit bitmap correctly. So
the performance hurt can be ignored.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
Hi Olaf and Malcolm, please help to retest this version.

 xen/arch/x86/hvm/vlapic.c        |    3 +++
 xen/arch/x86/hvm/vmx/vmx.c       |   24 ++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vlapic.h |    2 ++
 3 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 089d13f..6691e50 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1293,6 +1293,9 @@ static int lapic_save_regs(struct domain *d, hvm_domain_context_t *h)
 
     for_each_vcpu ( d, v )
     {
+        if ( hvm_funcs.sync_pir_to_irr )
+            hvm_funcs.sync_pir_to_irr(v);
+
         s = vcpu_vlapic(v);
         if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )
             break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 304aeea..df4737d 100644
--- 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;
+    struct vlapic *vlapic = vcpu_vlapic(v);
 
     if ( isr < 0 )
         isr = 0;
@@ -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++ )
+        __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
+
     vmx_vmcs_exit(v);
 }
 
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 16752b5..384b59a 100644
--- 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)))
 #define vlapic_test_and_set_vector(vec, bitmap)                         \
     test_and_set_bit(VEC_POS(vec), (uint32_t *)((bitmap) + REG_POS(vec)))
 #define vlapic_test_and_clear_vector(vec, bitmap)                       \
-- 
1.7.6.3

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

[Jan Beulich]

>>> 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.

Jan

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

[Olaf Hering]

On Tue, Oct 21, Yang Zhang wrote:

> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> This patch fixes two issues:
> 
> 1. Interrupts on PIR are lost during save/restore. Syncing the PIR
> into IRR during save will fix it.
> 
> 2. EOI exit bitmap doesn't set up correctly after restore. Here we
> will construct the eoi exit bitmap via (IRR | ISR). Though it may cause
> unnecessary eoi exit of the interrupts that pending in IRR or ISR during
> save/restore, each pending interrupt only causes one vmexit. The
> subsequent interrupts will adjust the eoi exit bitmap correctly. So
> the performance hurt can be ignored.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> Hi Olaf and Malcolm, please help to retest this version.

Yes, this works as well. Thanks!

Olaf

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

[Zhang, Yang Z]

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?

Best regards,
Yang

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

[Jan Beulich]

>>> 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.

Jan

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

[Zhang, Yang Z]

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. 


> 
> Jan


Best regards,
Yang

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

[Andrew Cooper]

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

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

[Tian, Kevin]

> From: Zhang, Yang Z
> Sent: Tuesday, October 21, 2014 1:33 PM
> 
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> This patch fixes two issues:
> 
> 1. Interrupts on PIR are lost during save/restore. Syncing the PIR
> into IRR during save will fix it.
> 
> 2. EOI exit bitmap doesn't set up correctly after restore. Here we
> will construct the eoi exit bitmap via (IRR | ISR). Though it may cause
> unnecessary eoi exit of the interrupts that pending in IRR or ISR during
> save/restore, each pending interrupt only causes one vmexit. The
> subsequent interrupts will adjust the eoi exit bitmap correctly. So
> the performance hurt can be ignored.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> ---
> Hi Olaf and Malcolm, please help to retest this version.
> 
>  xen/arch/x86/hvm/vlapic.c        |    3 +++
>  xen/arch/x86/hvm/vmx/vmx.c       |   24 ++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/vlapic.h |    2 ++
>  3 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 089d13f..6691e50 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1293,6 +1293,9 @@ static int lapic_save_regs(struct domain *d,
> hvm_domain_context_t *h)
> 
>      for_each_vcpu ( d, v )
>      {
> +        if ( hvm_funcs.sync_pir_to_irr )
> +            hvm_funcs.sync_pir_to_irr(v);
> +
>          s = vcpu_vlapic(v);
>          if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) !=
> 0 )
>              break;
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 304aeea..df4737d 100644
> --- 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;
> +    struct vlapic *vlapic = vcpu_vlapic(v);
> 
>      if ( isr < 0 )
>          isr = 0;
> @@ -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++ )
> +        __vmwrite(EOI_EXIT_BITMAP(i),
> v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> +
>      vmx_vmcs_exit(v);
>  }
> 
> diff --git a/xen/include/asm-x86/hvm/vlapic.h
> b/xen/include/asm-x86/hvm/vlapic.h
> index 16752b5..384b59a 100644
> --- 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)))
>  #define vlapic_test_and_set_vector(vec, bitmap)
> \
>      test_and_set_bit(VEC_POS(vec), (uint32_t *)((bitmap) + REG_POS(vec)))
>  #define vlapic_test_and_clear_vector(vec, bitmap)
> \
> --
> 1.7.6.3