[v5] x86/apicv: fix RTC periodic timer and apicv issue

Commit Message

Xuquan (Euler) Jan. 4, 2017, 12:21 p.m. UTC

From 9c23e1ff3eb75d71d691778a2e83421f645902fb Mon Sep 17 00:00:00 2001
From: Quan Xu <[e-mail]>
Date: Wed, 4 Jan 2017 20:03:31 +0800
Subject: [PATCH v5] x86/apicv: fix RTC periodic timer and apicv issue

When Xen apicv is enabled, wall clock time is faster on Windows7-32
guest with high payload (with 2vCPU, captured from xentrace, in
high payload, the count of IPI interrupt increases rapidly between
these vCPUs).

If IPI intrrupt (vector 0xe1) and periodic timer interrupt (vector 0xd1)
are both pending (index of bit set in vIRR), unfortunately, the IPI
intrrupt is high priority than periodic timer interrupt. Xen updates
IPI interrupt bit set in vIRR to guest interrupt status (RVI) as a high
priority and apicv (Virtual-Interrupt Delivery) delivers IPI interrupt
within VMX non-root operation without a VM-Exit. Within VMX non-root
operation, if periodic timer interrupt index of bit is set in vIRR and
highest, the apicv delivers periodic timer interrupt within VMX non-root
operation as well.

But in current code, if Xen doesn't update periodic timer interrupt bit
set in vIRR to guest interrupt status (RVI) directly, Xen is not aware
of this case to decrease the count (pending_intr_nr) of pending periodic
timer interrupt, then Xen will deliver a periodic timer interrupt again.

And that we update periodic timer interrupt in every VM-entry, there is
a chance that already-injected instance (before EOI-induced exit happens)
will incur another pending IRR setting if there is a VM-exit happens
between virtual interrupt injection (vIRR->0, vISR->1) and EOI-induced
exit (vISR->0), since pt_intr_post hasn't been invoked yet, then the
guest receives more periodic timer interrupt.

So we set eoi_exit_bitmap for intack.vector - give a chance to post
periodic time interrupts when periodic time interrupts become the
highest one.

Signed-off-by: Quan Xu <[e-mail]>
Acked-by: Kevin Tian <[e-mail]>
Tested-by: Chao Gao <[e-mail]>
---
 xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

--
1.7.12.4

Comments

Jan Beulich Jan. 4, 2017, 12:56 p.m. UTC | #1

>>> On 04.01.17 at 13:21, <[e-mail]> wrote:
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -312,13 +312,14 @@ void vmx_intr_assist(void)
>          unsigned int i, n;
> 
>         /*
> -        * Set eoi_exit_bitmap for periodic timer interrup to cause EOI-induced VM
> -        * exit, then pending periodic time interrups have the chance to be injected
> -        * for compensation
> +        * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
> +        * for intack.vector - give a chance to post periodic time interrupts when
> +        * periodic time interrupts become the highest one
>          */
> -        if (pt_vector != -1)
> -            vmx_set_eoi_exit_bitmap(v, pt_vector);
> -
> +        if ( pt_vector != -1 ) {

I would have said I'll fix the coding style violation here while committing,
but ...

> @@ -334,7 +335,8 @@ void vmx_intr_assist(void)
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
> 
> -        pt_intr_post(v, intack);
> +        if ( intack.vector == pt_vector )
> +            pt_intr_post(v, intack);

... this change doesn't appear to be mentioned in the description at
all, and I can't see why it is needed considering the is_pt_irq() check
(under lock) early on in pt_intr_post().

Jan

Tian, Kevin Jan. 5, 2017, 2:38 a.m. UTC | #2

> From: Jan Beulich [mailto:[e-mail]]
> Sent: Wednesday, January 04, 2017 8:57 PM
> 
> >>> On 04.01.17 at 13:21, <[e-mail]> wrote:
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -312,13 +312,14 @@ void vmx_intr_assist(void)
> >          unsigned int i, n;
> >
> >         /*
> > -        * Set eoi_exit_bitmap for periodic timer interrup to cause EOI-induced VM
> > -        * exit, then pending periodic time interrups have the chance to be injected
> > -        * for compensation
> > +        * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
> > +        * for intack.vector - give a chance to post periodic time interrupts when
> > +        * periodic time interrupts become the highest one
> >          */
> > -        if (pt_vector != -1)
> > -            vmx_set_eoi_exit_bitmap(v, pt_vector);
> > -
> > +        if ( pt_vector != -1 ) {
> 
> I would have said I'll fix the coding style violation here while committing,
> but ...
> 
> > @@ -334,7 +335,8 @@ void vmx_intr_assist(void)
> >              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >          }
> >
> > -        pt_intr_post(v, intack);
> > +        if ( intack.vector == pt_vector )
> > +            pt_intr_post(v, intack);
> 
> ... this change doesn't appear to be mentioned in the description at
> all, and I can't see why it is needed considering the is_pt_irq() check
> (under lock) early on in pt_intr_post().

yes, duplicated check here.

Thanks
Kevin

Xuquan (Euler) Jan. 5, 2017, 2:42 a.m. UTC | #3

On January 05, 2017 10:38 AM, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:[e-mail]]
>> Sent: Wednesday, January 04, 2017 8:57 PM
>>
>> >>> On 04.01.17 at 13:21, <[e-mail]> wrote:
>> > --- a/xen/arch/x86/hvm/vmx/intr.c
>> > +++ b/xen/arch/x86/hvm/vmx/intr.c
>> > @@ -334,7 +335,8 @@ void vmx_intr_assist(void)
>> >              __vmwrite(EOI_EXIT_BITMAP(i),
>v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>> >          }
>> >
>> > -        pt_intr_post(v, intack);
>> > +        if ( intack.vector == pt_vector )
>> > +            pt_intr_post(v, intack);
>>
>> ... this change doesn't appear to be mentioned in the description at
>> all, and I can't see why it is needed considering the is_pt_irq()
>> check (under lock) early on in pt_intr_post().
>
>yes, duplicated check here.
>

Agreed, drop it in next v6.

Quan

Patch

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 639a705..4d60eec 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -312,13 +312,14 @@  void vmx_intr_assist(void)
         unsigned int i, n;

        /*
-        * Set eoi_exit_bitmap for periodic timer interrup to cause EOI-induced VM
-        * exit, then pending periodic time interrups have the chance to be injected
-        * for compensation
+        * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
+        * for intack.vector - give a chance to post periodic time interrupts when
+        * periodic time interrupts become the highest one
         */
-        if (pt_vector != -1)
-            vmx_set_eoi_exit_bitmap(v, pt_vector);
-
+        if ( pt_vector != -1 ) {
+            ASSERT(intack.vector >= pt_vector);
+            vmx_set_eoi_exit_bitmap(v, intack.vector);
+        }
         /* we need update the RVI field */
         __vmread(GUEST_INTR_STATUS, &status);
         status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
@@ -334,7 +335,8 @@  void vmx_intr_assist(void)
             __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
         }

-        pt_intr_post(v, intack);
+        if ( intack.vector == pt_vector )
+            pt_intr_post(v, intack);
     }
     else
     {