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

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

[Xuquan (Euler)]

[-- Attachment #1: Type: text/plain, Size: 3206 bytes --]

>From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
From: Quan Xu <xuquan8@huawei.com>
Date: Fri, 19 Aug 2016 20:40:31 +0800
Subject: [RFC PATCH] 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. The guest receives more
periodic timer interrupt.

If the periodic timer interrut is delivered and not the highest priority, make
Xen be aware of this case to decrease the count of pending periodic timer
interrupt.

Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
Signed-off-by: Rongguang He <herongguang.he@huawei.com>
Signed-off-by: Quan Xu <xuquan8@huawei.com>
---
Why RFC:
1. I am not quite sure for other cases, such as nested case.
2. Within VMX non-root operation, an Asynchronous Enclave Exit (including external
   interrupts, non-maskable interrupt system-management interrrupts, exceptions
   and VM exit) may occur before delivery of a periodic timer interrupt, the periodic
   timer interrupt may be lost when a coming periodic timer interrupt is delivered.
   Actually, and so current code is.
---
 xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 8fca08c..d3a034e 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -334,7 +334,21 @@ void vmx_intr_assist(void)
             __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
         }

-        pt_intr_post(v, intack);
+       /*
+        * If the periodic timer interrut is delivered and not the highest priority,
+        * make Xen be aware of this case to decrease the count of pending periodic
+        * timer interrupt.
+        */
+        if ( pt_vector != -1 && intack.vector > pt_vector )
+        {
+            struct hvm_intack pt_intack;
+
+            pt_intack.vector = pt_vector;
+            pt_intack.source = hvm_intsrc_lapic;
+            pt_intr_post(v, pt_intack);
+        }
+        else
+            pt_intr_post(v, intack);
     }
     else
     {
--
1.7.12.4

[-- Attachment #2: 0001-x86-apicv-fix-RTC-periodic-timer-and-apicv-issue.patch --]
[-- Type: application/octet-stream, Size: 3137 bytes --]

From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
From: Quan Xu <xuquan8@huawei.com>
Date: Fri, 19 Aug 2016 20:40:31 +0800
Subject: [RFC PATCH] 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. The guest receives more
periodic timer interrupt.

If the periodic timer interrut is delivered and not the highest priority, make
Xen be aware of this case to decrease the count of pending periodic timer
interrupt.

Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
Signed-off-by: Rongguang He <herongguang.he@huawei.com>
Signed-off-by: Quan Xu <xuquan8@huawei.com>

---
Why RFC:
1. I am not quite sure for other cases, such as nested case.
2. Within VMX non-root operation, an Asynchronous Enclave Exit (including external
   interrupts, non-maskable interrupt system-management interrrupts, exceptions
   and VM exit) may occur before delivery of a periodic timer interrupt, the periodic
   timer interrupt may be lost when a coming periodic timer interrupt is delivered.
   Actually, and so current code is.
---
 xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 8fca08c..d3a034e 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -334,7 +334,21 @@ void vmx_intr_assist(void)
             __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
         }
 
-        pt_intr_post(v, intack);
+       /*
+        * If the periodic timer interrut is delivered and not the highest priority,
+        * make Xen be aware of this case to decrease the count of pending periodic
+        * timer interrupt.
+        */
+        if ( pt_vector != -1 && intack.vector > pt_vector )
+        {
+            struct hvm_intack pt_intack;
+
+            pt_intack.vector = pt_vector;
+            pt_intack.source = hvm_intsrc_lapic;
+            pt_intr_post(v, pt_intack);
+        }
+        else
+            pt_intr_post(v, intack);
     }
     else
     {
-- 
1.7.12.4


[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

On 2016/8/19 20:58, Xuquan (Euler) wrote:
> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
> From: Quan Xu <xuquan8@huawei.com>
> Date: Fri, 19 Aug 2016 20:40:31 +0800
> Subject: [RFC PATCH] 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. The guest receives more
> periodic timer interrupt.
>
> If the periodic timer interrut is delivered and not the highest priority, make
> Xen be aware of this case to decrease the count of pending periodic timer
> interrupt.

Nice catch!

>
> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
> Signed-off-by: Quan Xu <xuquan8@huawei.com>
> ---
> Why RFC:
> 1. I am not quite sure for other cases, such as nested case.

We don't support the nested APICv now. So it is ok for this patch.

> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including external
>    interrupts, non-maskable interrupt system-management interrrupts, exceptions
>    and VM exit) may occur before delivery of a periodic timer interrupt, the periodic
>    timer interrupt may be lost when a coming periodic timer interrupt is delivered.
>    Actually, and so current code is.

I think you mean the combination not interrupt lost. It should be ok 
since it happens rarely and even native OS will encounter the same 
problem if it disable the interrupt for too long.

> ---
>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 8fca08c..d3a034e 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
>
> -        pt_intr_post(v, intack);
> +       /*
> +        * If the periodic timer interrut is delivered and not the highest priority,
> +        * make Xen be aware of this case to decrease the count of pending periodic
> +        * timer interrupt.
> +        */
> +        if ( pt_vector != -1 && intack.vector > pt_vector )
> +        {
> +            struct hvm_intack pt_intack;
> +
> +            pt_intack.vector = pt_vector;
> +            pt_intack.source = hvm_intsrc_lapic;
> +            pt_intr_post(v, pt_intack);
> +        }
> +        else
> +            pt_intr_post(v, intack);
>      }
>      else
>      {
> --
> 1.7.12.4
>

Looks good to me.

Reviewed-by: Yang Zhang <yang.zhang.wz@gmail.com>

-- 
Yang
Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 5:38 PM, Yang Zhang wrote:
>On 2016/8/19 20:58, Xuquan (Euler) wrote:
>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>> From: Quan Xu <xuquan8@huawei.com>
>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>> Subject: [RFC PATCH] 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. The guest receives more periodic timer interrupt.
>>
>> If the periodic timer interrut is delivered and not the highest
>> priority, make Xen be aware of this case to decrease the count of
>> pending periodic timer interrupt.
>
>Nice catch!
>
>>
>> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
>> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
>> Signed-off-by: Quan Xu <xuquan8@huawei.com>
>> ---
>> Why RFC:
>> 1. I am not quite sure for other cases, such as nested case.
>
>We don't support the nested APICv now. So it is ok for this patch.
>
>> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including external
>>    interrupts, non-maskable interrupt system-management interrrupts, exceptions
>>    and VM exit) may occur before delivery of a periodic timer interrupt, the periodic
>>    timer interrupt may be lost when a coming periodic timer interrupt is delivered.
>>    Actually, and so current code is.
>
>I think you mean the combination not interrupt lost. It should be ok since it happens rarely and even native OS will encounter the same problem if it disable the interrupt for too long.
>

Yes, it is 'combination'.

>> ---
>>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
>> index 8fca08c..d3a034e 100644
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>>
>> -        pt_intr_post(v, intack);
>> +       /*
>> +        * If the periodic timer interrut is delivered and not the highest priority,
>> +        * make Xen be aware of this case to decrease the count of pending periodic
>> +        * timer interrupt.
>> +        */
>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>> +        {
>> +            struct hvm_intack pt_intack;
>> +
>> +            pt_intack.vector = pt_vector;
>> +            pt_intack.source = hvm_intsrc_lapic;
>> +            pt_intr_post(v, pt_intack);
>> +        }
>> +        else
>> +            pt_intr_post(v, intack);
>>      }
>>      else
>>      {
>> --
>> 1.7.12.4
>>
>
>Looks good to me.
>
>Reviewed-by: Yang Zhang <yang.zhang.wz@gmail.com>
>


Thanks for your comments!!
Quan


>--
>Yang
>Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 19.08.16 at 14:58, <xuquan8@huawei.com> wrote:
> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
> From: Quan Xu <xuquan8@huawei.com>
> Date: Fri, 19 Aug 2016 20:40:31 +0800
> Subject: [RFC PATCH] 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. The guest receives more
> periodic timer interrupt.
> 
> If the periodic timer interrut is delivered and not the highest priority, make
> Xen be aware of this case to decrease the count of pending periodic timer
> interrupt.

I can see the issue you're trying to address, but for one - doesn't
this lead to other double accounting, namely once the pt irq becomes
the highest priority one?

> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
> 
> -        pt_intr_post(v, intack);
> +       /*
> +        * If the periodic timer interrut is delivered and not the highest priority,
> +        * make Xen be aware of this case to decrease the count of pending periodic
> +        * timer interrupt.
> +        */
> +        if ( pt_vector != -1 && intack.vector > pt_vector )
> +        {
> +            struct hvm_intack pt_intack;
> +
> +            pt_intack.vector = pt_vector;
> +            pt_intack.source = hvm_intsrc_lapic;
> +            pt_intr_post(v, pt_intack);
> +        }
> +        else
> +            pt_intr_post(v, intack);

And then I'm having two problems with this change: Processing other
than the highest priority irq here looks to be non-architectural. From
looking over pt_intr_post() there's only pt related accounting and no
actual interrupt handling there, but I'd like this to be (a) confirmed
and (b) called out in the comment.

Plus the change above is in no way apicv specific, so I'd also like to
see clarification why this change is valid (i.e. at least not making
things worse) even without apicv in use.

And of course in any event VMX maintainer feedback is going to be
needed here.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

On 2016/8/22 18:35, Jan Beulich wrote:
>>>> On 19.08.16 at 14:58, <xuquan8@huawei.com> wrote:
>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>> From: Quan Xu <xuquan8@huawei.com>
>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>> Subject: [RFC PATCH] 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. The guest receives more
>> periodic timer interrupt.
>>
>> If the periodic timer interrut is delivered and not the highest priority, make
>> Xen be aware of this case to decrease the count of pending periodic timer
>> interrupt.
>
> I can see the issue you're trying to address, but for one - doesn't
> this lead to other double accounting, namely once the pt irq becomes
> the highest priority one?

"intack.vector > pt_vector"
I think above check in code can avoid the double accounting problem. It 
recalculate pending timer interrupt only when the highest priority 
interrupt is not timer.

>
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>>
>> -        pt_intr_post(v, intack);
>> +       /*
>> +        * If the periodic timer interrut is delivered and not the highest priority,
>> +        * make Xen be aware of this case to decrease the count of pending periodic
>> +        * timer interrupt.
>> +        */
>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>> +        {
>> +            struct hvm_intack pt_intack;
>> +
>> +            pt_intack.vector = pt_vector;
>> +            pt_intack.source = hvm_intsrc_lapic;
>> +            pt_intr_post(v, pt_intack);
>> +        }
>> +        else
>> +            pt_intr_post(v, intack);
>
> And then I'm having two problems with this change: Processing other
> than the highest priority irq here looks to be non-architectural. From
> looking over pt_intr_post() there's only pt related accounting and no
> actual interrupt handling there, but I'd like this to be (a) confirmed
> and (b) called out in the comment.
>
> Plus the change above is in no way apicv specific, so I'd also like to
> see clarification why this change is valid (i.e. at least not making
> things worse) even without apicv in use.

It is apicv specific case. Above code will execute only when cpu has 
virtual interrupt delivery which is part of apicv feature.

>
> And of course in any event VMX maintainer feedback is going to be
> needed here.
>
> Jan
>


-- 
Yang
Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 7:40 PM, Yang Zhang wrote:
>On 2016/8/22 18:35, Jan Beulich wrote:
>>>>> On 19.08.16 at 14:58, <xuquan8@huawei.com> wrote:
>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
>>> 2001
>>> From: Quan Xu <xuquan8@huawei.com>
>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer interrupt.
>>>
>>> If the periodic timer interrut is delivered and not the highest
>>> priority, make Xen be aware of this case to decrease the count of
>>> pending periodic timer interrupt.
>>
>> I can see the issue you're trying to address, but for one - doesn't
>> this lead to other double accounting, namely once the pt irq becomes
>> the highest priority one?
>
>"intack.vector > pt_vector"
>I think above check in code can avoid the double accounting problem. It
>recalculate pending timer interrupt only when the highest priority interrupt is
>not timer.
>

Yes, 

>>
>>> --- a/xen/arch/x86/hvm/vmx/intr.c
>>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>>              __vmwrite(EOI_EXIT_BITMAP(i),
>v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>>          }
>>>
>>> -        pt_intr_post(v, intack);
>>> +       /*
>>> +        * If the periodic timer interrut is delivered and not the highest
>priority,
>>> +        * make Xen be aware of this case to decrease the count of
>pending periodic
>>> +        * timer interrupt.
>>> +        */
>>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>>> +        {
>>> +            struct hvm_intack pt_intack;
>>> +
>>> +            pt_intack.vector = pt_vector;
>>> +            pt_intack.source = hvm_intsrc_lapic;
>>> +            pt_intr_post(v, pt_intack);
>>> +        }
>>> +        else
>>> +            pt_intr_post(v, intack);
>>
>> And then I'm having two problems with this change: Processing other
>> than the highest priority irq here looks to be non-architectural. From
>> looking over pt_intr_post() there's only pt related accounting and no
>> actual interrupt handling there, but I'd like this to be (a) confirmed
>> and (b) called out in the comment.
>>
>> Plus the change above is in no way apicv specific, so I'd also like to
>> see clarification why this change is valid (i.e. at least not making
>> things worse) even without apicv in use.
>
>It is apicv specific case. Above code will execute only when cpu has
>virtual interrupt delivery which is part of apicv feature.
>

Yes, I think so too. Thanks for your clarification!!

Quan


>>
>> And of course in any event VMX maintainer feedback is going to be
>> needed here.
>>
>> Jan
>>
>
>
>--
>Yang
>Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 22.08.16 at 13:40, <yang.zhang.wz@gmail.com> wrote:
> On 2016/8/22 18:35, Jan Beulich wrote:
>>>>> On 19.08.16 at 14:58, <xuquan8@huawei.com> wrote:
>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>>> From: Quan Xu <xuquan8@huawei.com>
>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>> Subject: [RFC PATCH] 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. The guest receives more
>>> periodic timer interrupt.
>>>
>>> If the periodic timer interrut is delivered and not the highest priority, make
>>> Xen be aware of this case to decrease the count of pending periodic timer
>>> interrupt.
>>
>> I can see the issue you're trying to address, but for one - doesn't
>> this lead to other double accounting, namely once the pt irq becomes
>> the highest priority one?
> 
> "intack.vector > pt_vector"
> I think above check in code can avoid the double accounting problem. It 
> recalculate pending timer interrupt only when the highest priority 
> interrupt is not timer.

I don't understand: When the pt one is highest priority, surely
recalculation is also needed. I'm not talking about a problem in
the case where the conditional is true, but about a possible
subsequent run through vmx_intr_assist() when the previous
higher priority interrupt has got handled, but the pt one (now
being highest) is still pending. (Or similarly consider the case
where on the next run through vmx_intr_assist() another higher
priority interrupts appeared.)

I'm not saying the change is wrong, I'm merely asking for further
proof that it doesn't open up new issues.

>>> --- a/xen/arch/x86/hvm/vmx/intr.c
>>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>>          }
>>>
>>> -        pt_intr_post(v, intack);
>>> +       /*
>>> +        * If the periodic timer interrut is delivered and not the highest priority,
>>> +        * make Xen be aware of this case to decrease the count of pending periodic
>>> +        * timer interrupt.
>>> +        */
>>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>>> +        {
>>> +            struct hvm_intack pt_intack;
>>> +
>>> +            pt_intack.vector = pt_vector;
>>> +            pt_intack.source = hvm_intsrc_lapic;
>>> +            pt_intr_post(v, pt_intack);
>>> +        }
>>> +        else
>>> +            pt_intr_post(v, intack);
>>
>> And then I'm having two problems with this change: Processing other
>> than the highest priority irq here looks to be non-architectural. From
>> looking over pt_intr_post() there's only pt related accounting and no
>> actual interrupt handling there, but I'd like this to be (a) confirmed
>> and (b) called out in the comment.
>>
>> Plus the change above is in no way apicv specific, so I'd also like to
>> see clarification why this change is valid (i.e. at least not making
>> things worse) even without apicv in use.
> 
> It is apicv specific case. Above code will execute only when cpu has 
> virtual interrupt delivery which is part of apicv feature.

Ah, right, I didn't look closely enough what conditional this is
inside of.

Jan

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>> Subject: [RFC PATCH] 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. The guest receives
>> more
>> periodic timer interrupt.
>>
>> If the periodic timer interrut is delivered and not the highest priority, make
>> Xen be aware of this case to decrease the count of pending periodic timer
>> interrupt.
>
>I can see the issue you're trying to address, but for one - doesn't
>this lead to other double accounting, namely once the pt irq becomes
>the highest priority one?
>

It is does NOT lead to other double accounting..
As if the pt irq becomes the highest priority one, the intack is pt one..
Then:

 +        else
 +            pt_intr_post(v, intack);


>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>              __vmwrite(EOI_EXIT_BITMAP(i),
>> v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>>
>> -        pt_intr_post(v, intack);
>> +       /*
>> +        * If the periodic timer interrut is delivered and not the highest
>> priority,
>> +        * make Xen be aware of this case to decrease the count of pending
>> periodic
>> +        * timer interrupt.
>> +        */
>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>> +        {
>> +            struct hvm_intack pt_intack;
>> +
>> +            pt_intack.vector = pt_vector;
>> +            pt_intack.source = hvm_intsrc_lapic;
>> +            pt_intr_post(v, pt_intack);
>> +        }
>> +        else
>> +            pt_intr_post(v, intack);
>
>And then I'm having two problems with this change: Processing other
>than the highest priority irq here looks to be non-architectural. From
>looking over pt_intr_post() there's only pt related accounting and no
>actual interrupt handling there, but I'd like this to be
> (a) confirmed

To me, YES.. but I hope Kevin could confirm it again.

>and (b) called out in the comment.

Agreed. Good idea.

>
>Plus the change above is in no way apicv specific, so I'd also like to
>see clarification why this change is valid (i.e. at least not making
>things worse) even without apicv in use.
>

__iiuc__, it is apicv specific, as these change is under condition 'cpu_has_vmx_virtual_intr_delivery' (Virtual Interrupt Delivery is one of apicv features) as below:

    if ( cpu_has_vmx_virtual_intr_delivery ...) {  ___change___ }
 



>And of course in any event VMX maintainer feedback is going to be
>needed here.


Thanks for your comments!!
Quan

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>> Subject: [RFC PATCH] 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. The guest receives
>>> more
>>> periodic timer interrupt.
>>>
>>> If the periodic timer interrut is delivered and not the highest priority, make
>>> Xen be aware of this case to decrease the count of pending periodic timer
>>> interrupt.
>>
>>I can see the issue you're trying to address, but for one - doesn't
>>this lead to other double accounting, namely once the pt irq becomes
>>the highest priority one?
>>
> 
> It is does NOT lead to other double accounting..
> As if the pt irq becomes the highest priority one, the intack is pt one..
> Then:
> 
>  +        else
>  +            pt_intr_post(v, intack);

As just said in reply to Yang: If this is still the same interrupt instance
as in a prior run through here which took the if() branch, this change
looks like having the potential of double accounting.

>>Plus the change above is in no way apicv specific, so I'd also like to
>>see clarification why this change is valid (i.e. at least not making
>>things worse) even without apicv in use.
>>
> 
> __iiuc__, it is apicv specific, as these change is under condition 
> 'cpu_has_vmx_virtual_intr_delivery' (Virtual Interrupt Delivery is one of 
> apicv features) as below:
> 
>     if ( cpu_has_vmx_virtual_intr_delivery ...) {  ___change___ }

Indeed, as already said in reply to Yang, I apparently didn't look
closely enough. I'm sorry for the noise.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

On 2016/8/22 20:04, Jan Beulich wrote:
>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>> Subject: [RFC PATCH] 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. The guest receives
>>>> more
>>>> periodic timer interrupt.
>>>>
>>>> If the periodic timer interrut is delivered and not the highest priority, make
>>>> Xen be aware of this case to decrease the count of pending periodic timer
>>>> interrupt.
>>>
>>> I can see the issue you're trying to address, but for one - doesn't
>>> this lead to other double accounting, namely once the pt irq becomes
>>> the highest priority one?
>>>
>>
>> It is does NOT lead to other double accounting..
>> As if the pt irq becomes the highest priority one, the intack is pt one..
>> Then:
>>
>>  +        else
>>  +            pt_intr_post(v, intack);
>
> As just said in reply to Yang: If this is still the same interrupt instance
> as in a prior run through here which took the if() branch, this change
> looks like having the potential of double accounting.

This cannot happen: if pt intr is the second priority one, then 
corresponding vIRR bit will be cleared by hardware while the highest 
priority intr is delivered to guest. And the next vmx_intr_assit cannot 
aware of it since the vIRR bit is zero.

-- 
Yang
Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 9:10 PM, Yang Zhang wrote:
>On 2016/8/22 20:04, Jan Beulich wrote:
>>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
>>>>> 2001
>>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer
>>>>> interrupt.
>>>>>
>>>>> If the periodic timer interrut is delivered and not the highest
>>>>> priority, make Xen be aware of this case to decrease the count of
>>>>> pending periodic timer interrupt.
>>>>
>>>> I can see the issue you're trying to address, but for one - doesn't
>>>> this lead to other double accounting, namely once the pt irq becomes
>>>> the highest priority one?
>>>>
>>>
>>> It is does NOT lead to other double accounting..
>>> As if the pt irq becomes the highest priority one, the intack is pt one..
>>> Then:
>>>
>>>  +        else
>>>  +            pt_intr_post(v, intack);
>>
>> As just said in reply to Yang: If this is still the same interrupt
>> instance as in a prior run through here which took the if() branch,
>> this change looks like having the potential of double accounting.
>
>This cannot happen: if pt intr is the second priority one, then corresponding vIRR
>bit will be cleared by hardware while the highest priority intr is delivered to
>guest. And the next vmx_intr_assit cannot aware of it since the vIRR bit is zero.
>

I am afraid that this may happen.. as I mentioned as 2nd RFC reason, 

Within VMX non-root operation, an Asynchronous Enclave Exit (including external
interrupts, non-maskable interrupt system-management interrrupts, exceptions and VM exit)
may occur before delivery of a periodic timer interrupt, the pt bit of VIRR is still there...

but I will explain more why it doesn't have the potential of double accounting in current code in next email.

Quan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 22.08.16 at 15:09, <yang.zhang.wz@gmail.com> wrote:
> On 2016/8/22 20:04, Jan Beulich wrote:
>>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>>> Subject: [RFC PATCH] 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. The guest receives
>>>>> more
>>>>> periodic timer interrupt.
>>>>>
>>>>> If the periodic timer interrut is delivered and not the highest priority, 
> make
>>>>> Xen be aware of this case to decrease the count of pending periodic timer
>>>>> interrupt.
>>>>
>>>> I can see the issue you're trying to address, but for one - doesn't
>>>> this lead to other double accounting, namely once the pt irq becomes
>>>> the highest priority one?
>>>>
>>>
>>> It is does NOT lead to other double accounting..
>>> As if the pt irq becomes the highest priority one, the intack is pt one..
>>> Then:
>>>
>>>  +        else
>>>  +            pt_intr_post(v, intack);
>>
>> As just said in reply to Yang: If this is still the same interrupt instance
>> as in a prior run through here which took the if() branch, this change
>> looks like having the potential of double accounting.
> 
> This cannot happen: if pt intr is the second priority one, then 
> corresponding vIRR bit will be cleared by hardware while the highest 
> priority intr is delivered to guest. And the next vmx_intr_assit cannot 
> aware of it since the vIRR bit is zero.

In addition to what Quan said in response - aren't you mixing up
vISR and vIRR here? I can see vISR being clear when a higher prio
interrupt gets delivered (unless that happened to interrupt the pt
interrupt handler), but I would hope that virtualized behavior
matches non-virtualized one in that vIRR accumulates requests.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

On 2016/8/22 22:57, Jan Beulich wrote:
>>>> On 22.08.16 at 15:09, <yang.zhang.wz@gmail.com> wrote:
>> On 2016/8/22 20:04, Jan Beulich wrote:
>>>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>>>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 2001
>>>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>>>> Subject: [RFC PATCH] 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. The guest receives
>>>>>> more
>>>>>> periodic timer interrupt.
>>>>>>
>>>>>> If the periodic timer interrut is delivered and not the highest priority,
>> make
>>>>>> Xen be aware of this case to decrease the count of pending periodic timer
>>>>>> interrupt.
>>>>>
>>>>> I can see the issue you're trying to address, but for one - doesn't
>>>>> this lead to other double accounting, namely once the pt irq becomes
>>>>> the highest priority one?
>>>>>
>>>>
>>>> It is does NOT lead to other double accounting..
>>>> As if the pt irq becomes the highest priority one, the intack is pt one..
>>>> Then:
>>>>
>>>>  +        else
>>>>  +            pt_intr_post(v, intack);
>>>
>>> As just said in reply to Yang: If this is still the same interrupt instance
>>> as in a prior run through here which took the if() branch, this change
>>> looks like having the potential of double accounting.
>>
>> This cannot happen: if pt intr is the second priority one, then
>> corresponding vIRR bit will be cleared by hardware while the highest
>> priority intr is delivered to guest. And the next vmx_intr_assit cannot
>> aware of it since the vIRR bit is zero.
>
> In addition to what Quan said in response - aren't you mixing up
> vISR and vIRR here? I can see vISR being clear when a higher prio

Right. It should be ISR not IRR.

> interrupt gets delivered (unless that happened to interrupt the pt
> interrupt handler), but I would hope that virtualized behavior
> matches non-virtualized one in that vIRR accumulates requests.

-- 
Yang
Alibaba Cloud Computing

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 8:04 PM, <JBeulich@suse.com> wrote: 
>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
>>>> 2001
>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer
>>>> interrupt.
>>>>
>>>> If the periodic timer interrut is delivered and not the highest
>>>> priority, make Xen be aware of this case to decrease the count of
>>>> pending periodic timer interrupt.
>>>
>>>I can see the issue you're trying to address, but for one - doesn't
>>>this lead to other double accounting, namely once the pt irq becomes
>>>the highest priority one?
>>>
>>
>> It is does NOT lead to other double accounting..
>> As if the pt irq becomes the highest priority one, the intack is pt one..
>> Then:
>>
>>  +        else
>>  +            pt_intr_post(v, intack);
>
>As just said in reply to Yang: If this is still the same interrupt instance as in a
>prior run through here which took the if() branch, this change looks like having
>the potential of double accounting.
>

I very appreciate your detail review. It looks like, but actually it doesn't happen.

 As the key parameter 'pt->irq_issued'..

In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..
In pt_intr_post(), clear the pt->irq_issued before touching the count 'pt->pending_intr_nr'..

According to your assumption, at the second call to pt_intr_post(), As if 'pt->irq_issued' is clear, pt is NULL in is_pt_irq() check,
then return, there is no chance to touch the count 'pt->pending_intr_nr'..
------
void pt_intr_post(struct vcpu *v, struct hvm_intack intack)
{
...
    pt = is_pt_irq(v, intack);
    if ( pt == NULL )
    {
        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
        return;
    }
...

     
...
}


static struct periodic_time *is_pt_irq()
{
 ....
    list_for_each_entry ( pt, head, list )
    {
        if ( pt->pending_intr_nr && ________pt->irq_issued_______ &&
             (intack.vector == pt_irq_vector(pt, intack.source)) )
            return pt;
    }

    return NULL;
}





__IIUC__, this question is based on the following pseudocode detail the behavior of virtual-interrupt delivery is __not__ atomic:

Vector <- RVI;
VISR[Vector] <- 1;
SVI <- Vector;
VPPR<- Vector & F0H;
VIRR[Vector] <- 0;
IF any bits set in VIRR
   Then RVI<-highest index of bit set in VIRR
   ELSE RVI <-0
FI
Deliver interrupt with Vector through IDT
...


We'd better check this first, as Yang said, this is atomic..

Quan

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 4477 bytes --]

2016年8月22日星期一，Xuquan (Euler) <xuquan8@huawei.com> 写道：

> On August 22, 2016 8:04 PM, <JBeulich@suse.com <javascript:;>> wrote:
> >>>> On 22.08.16 at 13:41, <xuquan8@huawei.com <javascript:;>> wrote:
> >> On August 22, 2016 6:36 PM, <JBeulich@suse.com <javascript:;>> wrote:
> >>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
> >>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
> >>>> 2001
> >>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
> >>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
> >>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer
> >>>> interrupt.
> >>>>
> >>>> If the periodic timer interrut is delivered and not the highest
> >>>> priority, make Xen be aware of this case to decrease the count of
> >>>> pending periodic timer interrupt.
> >>>
> >>>I can see the issue you're trying to address, but for one - doesn't
> >>>this lead to other double accounting, namely once the pt irq becomes
> >>>the highest priority one?
> >>>
> >>
> >> It is does NOT lead to other double accounting..
> >> As if the pt irq becomes the highest priority one, the intack is pt
> one..
> >> Then:
> >>
> >>  +        else
> >>  +            pt_intr_post(v, intack);
> >
> >As just said in reply to Yang: If this is still the same interrupt
> instance as in a
> >prior run through here which took the if() branch, this change looks like
> having
> >the potential of double accounting.
> >
>
> I very appreciate your detail review. It looks like, but actually it
> doesn't happen.
>
>  As the key parameter 'pt->irq_issued'..
>
> In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..
> In pt_intr_post(), clear the pt->irq_issued before touching the count
> 'pt->pending_intr_nr'..
>
> According to your assumption, at the second call to pt_intr_post(), As if
> 'pt->irq_issued' is clear, pt is NULL in is_pt_irq() check,
> then return, there is no chance to touch the count 'pt->pending_intr_nr'..
> ------
> void pt_intr_post(struct vcpu *v, struct hvm_intack intack)
> {
> ...
>     pt = is_pt_irq(v, intack);
>     if ( pt == NULL )
>     {
>         spin_unlock(&v->arch.hvm_vcpu.tm_lock);
>         return;
>     }
> ...
>
>
> ...
> }
>
>
> static struct periodic_time *is_pt_irq()
> {
>  ....
>     list_for_each_entry ( pt, head, list )
>     {
>         if ( pt->pending_intr_nr && ________pt->irq_issued_______ &&
>              (intack.vector == pt_irq_vector(pt, intack.source)) )
>             return pt;
>     }
>
>     return NULL;
> }
>
>
>
>
>
> __IIUC__, this question is based on the following pseudocode detail the
> behavior of virtual-interrupt delivery is __not__ atomic:
>
> Vector <- RVI;
> VISR[Vector] <- 1;
> SVI <- Vector;
> VPPR<- Vector & F0H;
> VIRR[Vector] <- 0;
> IF any bits set in VIRR
>    Then RVI<-highest index of bit set in VIRR
>    ELSE RVI <-0
> FI
> Deliver interrupt with Vector through IDT
> ...
>
>
> We'd better check this first, as Yang said, this is atomic..


i have said that this is ensured by hardware.


>
> Quan
>


-- 
best regards
yang

[-- Attachment #1.2: Type: text/html, Size: 5998 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Yang Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 4477 bytes --]

2016年8月22日星期一，Xuquan (Euler) <xuquan8@huawei.com> 写道：

> On August 22, 2016 8:04 PM, <JBeulich@suse.com <javascript:;>> wrote:
> >>>> On 22.08.16 at 13:41, <xuquan8@huawei.com <javascript:;>> wrote:
> >> On August 22, 2016 6:36 PM, <JBeulich@suse.com <javascript:;>> wrote:
> >>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
> >>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
> >>>> 2001
> >>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
> >>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
> >>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer
> >>>> interrupt.
> >>>>
> >>>> If the periodic timer interrut is delivered and not the highest
> >>>> priority, make Xen be aware of this case to decrease the count of
> >>>> pending periodic timer interrupt.
> >>>
> >>>I can see the issue you're trying to address, but for one - doesn't
> >>>this lead to other double accounting, namely once the pt irq becomes
> >>>the highest priority one?
> >>>
> >>
> >> It is does NOT lead to other double accounting..
> >> As if the pt irq becomes the highest priority one, the intack is pt
> one..
> >> Then:
> >>
> >>  +        else
> >>  +            pt_intr_post(v, intack);
> >
> >As just said in reply to Yang: If this is still the same interrupt
> instance as in a
> >prior run through here which took the if() branch, this change looks like
> having
> >the potential of double accounting.
> >
>
> I very appreciate your detail review. It looks like, but actually it
> doesn't happen.
>
>  As the key parameter 'pt->irq_issued'..
>
> In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..
> In pt_intr_post(), clear the pt->irq_issued before touching the count
> 'pt->pending_intr_nr'..
>
> According to your assumption, at the second call to pt_intr_post(), As if
> 'pt->irq_issued' is clear, pt is NULL in is_pt_irq() check,
> then return, there is no chance to touch the count 'pt->pending_intr_nr'..
> ------
> void pt_intr_post(struct vcpu *v, struct hvm_intack intack)
> {
> ...
>     pt = is_pt_irq(v, intack);
>     if ( pt == NULL )
>     {
>         spin_unlock(&v->arch.hvm_vcpu.tm_lock);
>         return;
>     }
> ...
>
>
> ...
> }
>
>
> static struct periodic_time *is_pt_irq()
> {
>  ....
>     list_for_each_entry ( pt, head, list )
>     {
>         if ( pt->pending_intr_nr && ________pt->irq_issued_______ &&
>              (intack.vector == pt_irq_vector(pt, intack.source)) )
>             return pt;
>     }
>
>     return NULL;
> }
>
>
>
>
>
> __IIUC__, this question is based on the following pseudocode detail the
> behavior of virtual-interrupt delivery is __not__ atomic:
>
> Vector <- RVI;
> VISR[Vector] <- 1;
> SVI <- Vector;
> VPPR<- Vector & F0H;
> VIRR[Vector] <- 0;
> IF any bits set in VIRR
>    Then RVI<-highest index of bit set in VIRR
>    ELSE RVI <-0
> FI
> Deliver interrupt with Vector through IDT
> ...
>
>
> We'd better check this first, as Yang said, this is atomic..


i have said that this is ensured by hardware.


>
> Quan
>


-- 
best regards
yang

[-- Attachment #1.2: Type: text/html, Size: 5998 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 22.08.16 at 16:02, <xuquan8@huawei.com> wrote:
> On August 22, 2016 8:04 PM, <JBeulich@suse.com> wrote: 
>>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00
>>>>> 2001
>>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>>> Subject: [RFC PATCH] 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. The guest receives more periodic timer
>>>>> interrupt.
>>>>>
>>>>> If the periodic timer interrut is delivered and not the highest
>>>>> priority, make Xen be aware of this case to decrease the count of
>>>>> pending periodic timer interrupt.
>>>>
>>>>I can see the issue you're trying to address, but for one - doesn't
>>>>this lead to other double accounting, namely once the pt irq becomes
>>>>the highest priority one?
>>>>
>>>
>>> It is does NOT lead to other double accounting..
>>> As if the pt irq becomes the highest priority one, the intack is pt one..
>>> Then:
>>>
>>>  +        else
>>>  +            pt_intr_post(v, intack);
>>
>>As just said in reply to Yang: If this is still the same interrupt instance 
> as in a
>>prior run through here which took the if() branch, this change looks like 
> having
>>the potential of double accounting.
>>
> 
> I very appreciate your detail review. It looks like, but actually it doesn't 
> happen.
> 
>  As the key parameter 'pt->irq_issued'..
> 
> In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..
> In pt_intr_post(), clear the pt->irq_issued before touching the count 
> 'pt->pending_intr_nr'..
> 
> According to your assumption, at the second call to pt_intr_post(), As if 
> 'pt->irq_issued' is clear, pt is NULL in is_pt_irq() check,
> then return, there is no chance to touch the count 'pt->pending_intr_nr'..

Hmm, wait: That second pass would also get us through
pt_update_irq() a second time, which might cause irq_issued to get
set again.

Granted this code is fragile, therefore please excuse that I'm trying
to be extra careful with accepting changes to it.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 22, 2016 11:12 PM, <JBeulich@suse.com> wrote:
>>>> On 22.08.16 at 16:02, <xuquan8@huawei.com> wrote:
>> On August 22, 2016 8:04 PM, <JBeulich@suse.com> wrote:
>>>>>> On 22.08.16 at 13:41, <xuquan8@huawei.com> wrote:
>>>> On August 22, 2016 6:36 PM, <JBeulich@suse.com> wrote:
>>>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> wrote:
>>>>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17
>00:00:00
>>>>>> 2001
>>>>>> From: Quan Xu <xuquan8@xxxxxxxxxx>
>>>>>> Date: Fri, 19 Aug 2016 20:40:31 +0800
>>>>>> Subject: [RFC PATCH] 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. The guest receives
>>>>>> more periodic timer interrupt.
>>>>>>
>>>>>> If the periodic timer interrut is delivered and not the highest
>>>>>> priority, make Xen be aware of this case to decrease the count of
>>>>>> pending periodic timer interrupt.
>>>>>
>>>>>I can see the issue you're trying to address, but for one - doesn't
>>>>>this lead to other double accounting, namely once the pt irq becomes
>>>>>the highest priority one?
>>>>>
>>>>
>>>> It is does NOT lead to other double accounting..
>>>> As if the pt irq becomes the highest priority one, the intack is pt one..
>>>> Then:
>>>>
>>>>  +        else
>>>>  +            pt_intr_post(v, intack);
>>>
>>>As just said in reply to Yang: If this is still the same interrupt
>>>instance
>> as in a
>>>prior run through here which took the if() branch, this change looks
>>>like
>> having
>>>the potential of double accounting.
>>>
>>
>> I very appreciate your detail review. It looks like, but actually it
>> doesn't happen.
>>
>>  As the key parameter 'pt->irq_issued'..
>>
>> In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..
>> In pt_intr_post(), clear the pt->irq_issued before touching the count
>> 'pt->pending_intr_nr'..
>>
>> According to your assumption, at the second call to pt_intr_post(), As
>> if 'pt->irq_issued' is clear, pt is NULL in is_pt_irq() check, then
>> return, there is no chance to touch the count 'pt->pending_intr_nr'..
>
>Hmm, wait: That second pass would also get us through
>pt_update_irq() a second time, which might cause irq_issued to get set again.
>

 iiuc this case is IRQ combination, issue twice but delivery once..
Another enhancement may be required here,
     IF  hvm_intsrc_lapic && the PT IRQ index bit in VIRR is not clear
        THEN don't issue PT IRQ in pt_update_irq()
     FI

>Granted this code is fragile, therefore please excuse that I'm trying to be extra
>careful with accepting changes to it.

Understood :):)...

Quan

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Tian, Kevin]

> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> Sent: Friday, August 19, 2016 8:59 PM
> 
> 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. The guest receives more
> periodic timer interrupt.
> 
> If the periodic timer interrut is delivered and not the highest priority, make
> Xen be aware of this case to decrease the count of pending periodic timer
> interrupt.
> 
> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
> Signed-off-by: Quan Xu <xuquan8@huawei.com>
> ---
> Why RFC:
> 1. I am not quite sure for other cases, such as nested case.
> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including external
>    interrupts, non-maskable interrupt system-management interrrupts, exceptions
>    and VM exit) may occur before delivery of a periodic timer interrupt, the periodic
>    timer interrupt may be lost when a coming periodic timer interrupt is delivered.
>    Actually, and so current code is.
> ---
>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 8fca08c..d3a034e 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
> 
> -        pt_intr_post(v, intack);
> +       /*
> +        * If the periodic timer interrut is delivered and not the highest priority,
> +        * make Xen be aware of this case to decrease the count of pending periodic
> +        * timer interrupt.
> +        */
> +        if ( pt_vector != -1 && intack.vector > pt_vector )
> +        {
> +            struct hvm_intack pt_intack;
> +
> +            pt_intack.vector = pt_vector;
> +            pt_intack.source = hvm_intsrc_lapic;
> +            pt_intr_post(v, pt_intack);
> +        }
> +        else
> +            pt_intr_post(v, intack);
>      }

Here is my thought. w/o above patch one pending pt interrupt may 
result in multiple injections of guest timer interrupt, as you identified, 
when pt_vector happens to be not the highest pending vector. Then
w/ your patch, multiple pending pt interrupt instances may be combined
as one injection of guest timer interrupt. Especially intack.vector
>pt_vector doesn't mean pt_intr is eligible for injection, which might
be blocked due to other reasons. As Jan pointed out, this path is very 
fragile, so better we can have a fix to sustain the original behavior
with one pending pt instance causing one actual injection.

What about doing pt_intr_post in EOI-induced VM-exit instead? EOI exit
bit for pt_intr is already set to 1 which means every injection would incur
an EOI-induced VM-exit:

       /*
        * 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
        */
        if (pt_vector != -1)
            vmx_set_eoi_exit_bitmap(v, pt_vector);

I don't think delaying post makes much difference here. Even we do
post earlier like your patch, further pending instances have to wait
until current instance is completed. So as long as post happens before
EOI is completed, it should be fine.

Thanks
Kevin

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 30, 2016 1:58 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> Sent: Friday, August 19, 2016 8:59 PM
>>
>> 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. The guest receives more periodic timer interrupt.
>>
>> If the periodic timer interrut is delivered and not the highest
>> priority, make Xen be aware of this case to decrease the count of
>> pending periodic timer interrupt.
>>
>> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
>> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
>> Signed-off-by: Quan Xu <xuquan8@huawei.com>
>> ---
>> Why RFC:
>> 1. I am not quite sure for other cases, such as nested case.
>> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including
>external
>>    interrupts, non-maskable interrupt system-management interrrupts,
>exceptions
>>    and VM exit) may occur before delivery of a periodic timer interrupt, the
>periodic
>>    timer interrupt may be lost when a coming periodic timer interrupt is
>delivered.
>>    Actually, and so current code is.
>> ---
>>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
>> index 8fca08c..d3a034e 100644
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>              __vmwrite(EOI_EXIT_BITMAP(i),
>v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>>
>> -        pt_intr_post(v, intack);
>> +       /*
>> +        * If the periodic timer interrut is delivered and not the highest
>priority,
>> +        * make Xen be aware of this case to decrease the count of pending
>periodic
>> +        * timer interrupt.
>> +        */
>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>> +        {
>> +            struct hvm_intack pt_intack;
>> +
>> +            pt_intack.vector = pt_vector;
>> +            pt_intack.source = hvm_intsrc_lapic;
>> +            pt_intr_post(v, pt_intack);
>> +        }
>> +        else
>> +            pt_intr_post(v, intack);
>>      }
>
>Here is my thought. w/o above patch one pending pt interrupt may result in
>multiple injections of guest timer interrupt, as you identified, when pt_vector
>happens to be not the highest pending vector. Then w/ your patch, multiple
>pending pt interrupt instances may be combined as one injection of guest timer
>interrupt. Especially intack.vector
>>pt_vector doesn't mean pt_intr is eligible for injection, which might
>be blocked due to other reasons. As Jan pointed out, this path is very fragile, so
>better we can have a fix to sustain the original behavior with one pending pt
>instance causing one actual injection.
>

Agreed. Good summary..

>What about doing pt_intr_post in EOI-induced VM-exit instead? EOI exit bit for
>pt_intr is already set to 1 which means every injection would incur an
>EOI-induced VM-exit:
>
>       /*
>        * 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
>        */
>        if (pt_vector != -1)
>            vmx_set_eoi_exit_bitmap(v, pt_vector);
>
>I don't think delaying post makes much difference here. Even we do post earlier
>like your patch, further pending instances have to wait until current instance is
>completed. So as long as post happens before EOI is completed, it should be
>fine.

Although I really wanted to fix here, I gave up.. 
We need to try our best to narrow down this 'window' -- from pt_update_irq() to pt_intr_post()..
I think It is too later to call pt_intr_post() in EOI-induced VM-exit, _iiuc_, we cannot make sure that each PT interrupt injection incurs an EOI-induced VM-exit before the next VM entry..
then one pending pt interrupt may result in multiple injections of guest timer interrupt.. 

to be honest, my fix is also a tradeoff.. w/ my patch, multiple pending pt interrupt instances may be combined as one injection of guest timer interrupt..
 but as Yang said, It should be ok since it happens rarely and even native OS will encounter the same problem if it disable the interrupt for too long.



Thanks for your comments!!

Quan
Huawei 2012 lab

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 06.09.16 at 10:53, <xuquan8@huawei.com> wrote:
> to be honest, my fix is also a tradeoff.. w/ my patch, multiple pending pt 
> interrupt instances may be combined as one injection of guest timer 
> interrupt..
>  but as Yang said, It should be ok since it happens rarely and even native 
> OS will encounter the same problem if it disable the interrupt for too long.

And one OS may properly deal with that situation while another
might not. For example, let me remind you of issues the hypervisor
had in earlier days when no event/softirq processing happened for
long enough, resulting in time management issues due to missed
platform timer overflow handling. IOW an OS may avoid that
situation by simply not disabling IRQs for too long a time.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On September 06, 2016 5:05 PM, Jan Beulich <JBeulich@suse.com>
>>>> On 06.09.16 at 10:53, <xuquan8@huawei.com> wrote:
>> to be honest, my fix is also a tradeoff.. w/ my patch, multiple
>> pending pt interrupt instances may be combined as one injection of
>> guest timer interrupt..
>>  but as Yang said, It should be ok since it happens rarely and even
>> native OS will encounter the same problem if it disable the interrupt for too
>long.
>
>And one OS may properly deal with that situation while another might not. For
>example, let me remind you of issues the hypervisor had in earlier days when no
>event/softirq processing happened for long enough, resulting in time
>management issues due to missed platform timer overflow handling. IOW an OS
>may avoid that situation by simply not disabling IRQs for too long a time.
>
Thanks for your reminding,
Jan, do you have any suggestion to fix this PT and apicv issue?
What do you think about Kevin's?

Quan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Jan Beulich]

>>> On 06.09.16 at 13:22, <xuquan8@huawei.com> wrote:
> On September 06, 2016 5:05 PM, Jan Beulich <JBeulich@suse.com>
>>>>> On 06.09.16 at 10:53, <xuquan8@huawei.com> wrote:
>>> to be honest, my fix is also a tradeoff.. w/ my patch, multiple
>>> pending pt interrupt instances may be combined as one injection of
>>> guest timer interrupt..
>>>  but as Yang said, It should be ok since it happens rarely and even
>>> native OS will encounter the same problem if it disable the interrupt for too
>>long.
>>
>>And one OS may properly deal with that situation while another might not. For
>>example, let me remind you of issues the hypervisor had in earlier days when no
>>event/softirq processing happened for long enough, resulting in time
>>management issues due to missed platform timer overflow handling. IOW an OS
>>may avoid that situation by simply not disabling IRQs for too long a time.
>>
> Thanks for your reminding,
> Jan, do you have any suggestion to fix this PT and apicv issue?
> What do you think about Kevin's?

Kevin's suggestion sounded reasonable, iirc, and I'd rely on his
and your better VMX knowledge for the low level details.

Jan


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On September 06, 2016 9:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.09.16 at 13:22, <xuquan8@huawei.com> wrote:
>> On September 06, 2016 5:05 PM, Jan Beulich <JBeulich@suse.com>
>>>>>> On 06.09.16 at 10:53, <xuquan8@huawei.com> wrote:
>>>> to be honest, my fix is also a tradeoff.. w/ my patch, multiple
>>>> pending pt interrupt instances may be combined as one injection of
>>>> guest timer interrupt..
>>>>  but as Yang said, It should be ok since it happens rarely and even
>>>> native OS will encounter the same problem if it disable the
>>>> interrupt for too
>>>long.
>>>
>>>And one OS may properly deal with that situation while another might
>>>not. For example, let me remind you of issues the hypervisor had in
>>>earlier days when no event/softirq processing happened for long
>>>enough, resulting in time management issues due to missed platform
>>>timer overflow handling. IOW an OS may avoid that situation by simply not
>disabling IRQs for too long a time.
>>>
>> Thanks for your reminding,
>> Jan, do you have any suggestion to fix this PT and apicv issue?
>> What do you think about Kevin's?
>
>Kevin's suggestion sounded reasonable, iirc, and I'd rely on his and your better
>VMX knowledge for the low level details.
>

I'll try to verify Kevin's later.
Kevin is really a master but I am just a rookie. I always appreciate both of your education..

Quan

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
>> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> Sent: Friday, August 19, 2016 8:59 PM
>>
>> 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. The guest receives more periodic timer interrupt.
>>
>> If the periodic timer interrut is delivered and not the highest
>> priority, make Xen be aware of this case to decrease the count of
>> pending periodic timer interrupt.
>>
>> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
>> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
>> Signed-off-by: Quan Xu <xuquan8@huawei.com>
>> ---
>> Why RFC:
>> 1. I am not quite sure for other cases, such as nested case.
>> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including
>external
>>    interrupts, non-maskable interrupt system-management interrrupts,
>exceptions
>>    and VM exit) may occur before delivery of a periodic timer interrupt, the
>periodic
>>    timer interrupt may be lost when a coming periodic timer interrupt is
>delivered.
>>    Actually, and so current code is.
>> ---
>>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
>> index 8fca08c..d3a034e 100644
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
>>              __vmwrite(EOI_EXIT_BITMAP(i),
>v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>>
>> -        pt_intr_post(v, intack);
>> +       /*
>> +        * If the periodic timer interrut is delivered and not the highest
>priority,
>> +        * make Xen be aware of this case to decrease the count of pending
>periodic
>> +        * timer interrupt.
>> +        */
>> +        if ( pt_vector != -1 && intack.vector > pt_vector )
>> +        {
>> +            struct hvm_intack pt_intack;
>> +
>> +            pt_intack.vector = pt_vector;
>> +            pt_intack.source = hvm_intsrc_lapic;
>> +            pt_intr_post(v, pt_intack);
>> +        }
>> +        else
>> +            pt_intr_post(v, intack);
>>      }
>
>Here is my thought. w/o above patch one pending pt interrupt may result in
>multiple injections of guest timer interrupt, as you identified, when pt_vector
>happens to be not the highest pending vector. Then w/ your patch, multiple
>pending pt interrupt instances may be combined as one injection of guest timer
>interrupt. Especially intack.vector
>>pt_vector doesn't mean pt_intr is eligible for injection, which might
>be blocked due to other reasons. As Jan pointed out, this path is very fragile, so
>better we can have a fix to sustain the original behavior with one pending pt
>instance causing one actual injection.
>
>What about doing pt_intr_post in EOI-induced VM-exit instead? EOI exit bit for
>pt_intr is already set to 1 which means every injection would incur an
>EOI-induced VM-exit:
>
>       /*
>        * 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
>        */
>        if (pt_vector != -1)
>            vmx_set_eoi_exit_bitmap(v, pt_vector);
>
>I don't think delaying post makes much difference here. Even we do post earlier
>like your patch, further pending instances have to wait until current instance is
>completed. So as long as post happens before EOI is completed, it should be
>fine.

Kevin, I verify your suggestion with below modification. wall clock time is __still__ faster. I hope this modification is correct to your suggestion.

I __guess__ that the vm-entry is more frequent than PT interrupt,
Specifically, if there is a PT interrupt pending, the count (pending_intr_nr) is 1..

before next PT interrupt coming, each PT interrupt injection may not incur an EOI-induced VM-exit directly, multiple PT interrupt inject for a pending PT interrupt,
then multiple EOI-induced VM-exit incur with multiple pt_intr_post() to decrease the count (pending_intr_nr), but the count (pending_intr_nr) is still 1 before next PT interrupt coming, then only one pt_intr_post() is effective..

Thanks for your time!!
Quan

======= modification ========

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1d5d287..cc247c3 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
 void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
 {
     struct domain *d = vlapic_domain(vlapic);
+    struct hvm_intack pt_intack;
+
+    pt_intack.vector = vector;
+    pt_intack.source = hvm_intsrc_lapic;
+    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);

     if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
         vioapic_update_EOI(d, vector);
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 8fca08c..29d9bbf 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -333,8 +333,6 @@ void vmx_intr_assist(void)
             clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
             __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
         }
-
-        pt_intr_post(v, intack);
     }
     else
     {
 

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Tian, Kevin]

> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> Sent: Friday, September 09, 2016 11:02 AM
> 
> On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> Sent: Friday, August 19, 2016 8:59 PM
> >>
> >> 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. The guest receives more periodic timer interrupt.
> >>
> >> If the periodic timer interrut is delivered and not the highest
> >> priority, make Xen be aware of this case to decrease the count of
> >> pending periodic timer interrupt.
> >>
> >> Signed-off-by: Yifei Jiang <jiangyifei@huawei.com>
> >> Signed-off-by: Rongguang He <herongguang.he@huawei.com>
> >> Signed-off-by: Quan Xu <xuquan8@huawei.com>
> >> ---
> >> Why RFC:
> >> 1. I am not quite sure for other cases, such as nested case.
> >> 2. Within VMX non-root operation, an Asynchronous Enclave Exit (including
> >external
> >>    interrupts, non-maskable interrupt system-management interrrupts,
> >exceptions
> >>    and VM exit) may occur before delivery of a periodic timer interrupt, the
> >periodic
> >>    timer interrupt may be lost when a coming periodic timer interrupt is
> >delivered.
> >>    Actually, and so current code is.
> >> ---
> >>  xen/arch/x86/hvm/vmx/intr.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> >> index 8fca08c..d3a034e 100644
> >> --- a/xen/arch/x86/hvm/vmx/intr.c
> >> +++ b/xen/arch/x86/hvm/vmx/intr.c
> >> @@ -334,7 +334,21 @@ void vmx_intr_assist(void)
> >>              __vmwrite(EOI_EXIT_BITMAP(i),
> >v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >>          }
> >>
> >> -        pt_intr_post(v, intack);
> >> +       /*
> >> +        * If the periodic timer interrut is delivered and not the highest
> >priority,
> >> +        * make Xen be aware of this case to decrease the count of pending
> >periodic
> >> +        * timer interrupt.
> >> +        */
> >> +        if ( pt_vector != -1 && intack.vector > pt_vector )
> >> +        {
> >> +            struct hvm_intack pt_intack;
> >> +
> >> +            pt_intack.vector = pt_vector;
> >> +            pt_intack.source = hvm_intsrc_lapic;
> >> +            pt_intr_post(v, pt_intack);
> >> +        }
> >> +        else
> >> +            pt_intr_post(v, intack);
> >>      }
> >
> >Here is my thought. w/o above patch one pending pt interrupt may result in
> >multiple injections of guest timer interrupt, as you identified, when pt_vector
> >happens to be not the highest pending vector. Then w/ your patch, multiple
> >pending pt interrupt instances may be combined as one injection of guest timer
> >interrupt. Especially intack.vector
> >>pt_vector doesn't mean pt_intr is eligible for injection, which might
> >be blocked due to other reasons. As Jan pointed out, this path is very fragile, so
> >better we can have a fix to sustain the original behavior with one pending pt
> >instance causing one actual injection.
> >
> >What about doing pt_intr_post in EOI-induced VM-exit instead? EOI exit bit for
> >pt_intr is already set to 1 which means every injection would incur an
> >EOI-induced VM-exit:
> >
> >       /*
> >        * 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
> >        */
> >        if (pt_vector != -1)
> >            vmx_set_eoi_exit_bitmap(v, pt_vector);
> >
> >I don't think delaying post makes much difference here. Even we do post earlier
> >like your patch, further pending instances have to wait until current instance is
> >completed. So as long as post happens before EOI is completed, it should be
> >fine.
> 
> Kevin, I verify your suggestion with below modification. wall clock time is __still__ faster.
> I hope this modification is correct to your suggestion.
> 
> I __guess__ that the vm-entry is more frequent than PT interrupt,
> Specifically, if there is a PT interrupt pending, the count (pending_intr_nr) is 1..
> 
> before next PT interrupt coming, each PT interrupt injection may not incur an EOI-induced
> VM-exit directly, multiple PT interrupt inject for a pending PT interrupt,
> then multiple EOI-induced VM-exit incur with multiple pt_intr_post() to decrease the count
> (pending_intr_nr), but the count (pending_intr_nr) is still 1 before next PT interrupt coming,
> then only one pt_intr_post() is effective..

I don't quite understand your description here, but just for your patch see below...

> 
> Thanks for your time!!
> Quan
> 
> ======= modification ========
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 1d5d287..cc247c3 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  {
>      struct domain *d = vlapic_domain(vlapic);
> +    struct hvm_intack pt_intack;
> +
> +    pt_intack.vector = vector;
> +    pt_intack.source = hvm_intsrc_lapic;
> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
> 
>      if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(d, vector);
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 8fca08c..29d9bbf 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
> -
> -        pt_intr_post(v, intack);
>      }
>      else
>      {
> 

Because we update pt irq in every vmentry, 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 HW virtual interrupt injection (vIRR->0, vISR->1) and 
EOI-induced exit (vISR->0), since pt_intr_post hasn't been invoked
yet. I guess this is the reason why you still see faster wallclock.

I think you need mark this pending_intr_post situation explicitly. 
Then pt_update_irq should skip such pt timer when pending_intr_post 
of that timer is true (otherwise the update is meaningless since 
previous one hasn't been posted yet). Then with your change to post 
in EOI-induced exit handler, it should work correctly to meet the goal 
- one virtual interrupt delivery for one pending pt intr...

Thanks
Kevin

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On September 12, 2016 3:58 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> Sent: Friday, September 09, 2016 11:02 AM
>>
>> On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
>> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> >> Sent: Friday, August 19, 2016 8:59 PM
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 1d5d287..cc247c3 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)  void
>> vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)  {
>>      struct domain *d = vlapic_domain(vlapic);
>> +    struct hvm_intack pt_intack;
>> +
>> +    pt_intack.vector = vector;
>> +    pt_intack.source = hvm_intsrc_lapic;
>> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
>>
>>      if ( vlapic_test_and_clear_vector(vector,
>&vlapic->regs->data[APIC_TMR]) )
>>          vioapic_update_EOI(d, vector); diff --git
>> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
>> 8fca08c..29d9bbf 100644
>> --- a/xen/arch/x86/hvm/vmx/intr.c
>> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
>>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
>>              __vmwrite(EOI_EXIT_BITMAP(i),
>v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>>          }
>> -
>> -        pt_intr_post(v, intack);
>>      }
>>      else
>>      {
>>
>
>Because we update pt irq in every vmentry, 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 HW virtual interrupt
>injection (vIRR->0, vISR->1) and EOI-induced exit (vISR->0), since pt_intr_post
>hasn't been invoked yet. I guess this is the reason why you still see faster
>wallclock.
>

Agreed. A good description. My bad description is from another aspect.

>I think you need mark this pending_intr_post situation explicitly.
>Then pt_update_irq should skip such pt timer when pending_intr_post of that
>timer is true (otherwise the update is meaningless since previous one hasn't
>been posted yet). Then with your change to post in EOI-induced exit handler, it
>should work correctly to meet the goal
>- one virtual interrupt delivery for one pending pt intr...
>
I think we are at least on the right track.
But I can't follow ' pending_intr_post ', a new parameter? Thanks.


Quan



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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Tian, Kevin]

> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> Sent: Monday, September 12, 2016 5:08 PM
> 
> On September 12, 2016 3:58 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> Sent: Friday, September 09, 2016 11:02 AM
> >>
> >> On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
> >> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> >> Sent: Friday, August 19, 2016 8:59 PM
> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> >> index 1d5d287..cc247c3 100644
> >> --- a/xen/arch/x86/hvm/vlapic.c
> >> +++ b/xen/arch/x86/hvm/vlapic.c
> >> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)  void
> >> vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)  {
> >>      struct domain *d = vlapic_domain(vlapic);
> >> +    struct hvm_intack pt_intack;
> >> +
> >> +    pt_intack.vector = vector;
> >> +    pt_intack.source = hvm_intsrc_lapic;
> >> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
> >>
> >>      if ( vlapic_test_and_clear_vector(vector,
> >&vlapic->regs->data[APIC_TMR]) )
> >>          vioapic_update_EOI(d, vector); diff --git
> >> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
> >> 8fca08c..29d9bbf 100644
> >> --- a/xen/arch/x86/hvm/vmx/intr.c
> >> +++ b/xen/arch/x86/hvm/vmx/intr.c
> >> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
> >>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
> >>              __vmwrite(EOI_EXIT_BITMAP(i),
> >v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >>          }
> >> -
> >> -        pt_intr_post(v, intack);
> >>      }
> >>      else
> >>      {
> >>
> >
> >Because we update pt irq in every vmentry, 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 HW virtual interrupt
> >injection (vIRR->0, vISR->1) and EOI-induced exit (vISR->0), since pt_intr_post
> >hasn't been invoked yet. I guess this is the reason why you still see faster
> >wallclock.
> >
> 
> Agreed. A good description. My bad description is from another aspect.
> 
> >I think you need mark this pending_intr_post situation explicitly.
> >Then pt_update_irq should skip such pt timer when pending_intr_post of that
> >timer is true (otherwise the update is meaningless since previous one hasn't
> >been posted yet). Then with your change to post in EOI-induced exit handler, it
> >should work correctly to meet the goal
> >- one virtual interrupt delivery for one pending pt intr...
> >
> I think we are at least on the right track.
> But I can't follow ' pending_intr_post ', a new parameter? Thanks.
> 
> 

yes, a new parameter to record whether a intr_post operation is pending

Thanks
Kevin

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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Xuquan (Euler)]

On September 19, 2016 5:25 PM, Tian Kevin <kevin.tian@intel.com> wrote:
>> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> Sent: Monday, September 12, 2016 5:08 PM
>>
>> On September 12, 2016 3:58 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> >> Sent: Friday, September 09, 2016 11:02 AM
>> >>
>> >> On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
>> >> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
>> >> >> Sent: Friday, August 19, 2016 8:59 PM
>> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> >> index 1d5d287..cc247c3 100644
>> >> --- a/xen/arch/x86/hvm/vlapic.c
>> >> +++ b/xen/arch/x86/hvm/vlapic.c
>> >> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>> >> void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)  {
>> >>      struct domain *d = vlapic_domain(vlapic);
>> >> +    struct hvm_intack pt_intack;
>> >> +
>> >> +    pt_intack.vector = vector;
>> >> +    pt_intack.source = hvm_intsrc_lapic;
>> >> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
>> >>
>> >>      if ( vlapic_test_and_clear_vector(vector,
>> >&vlapic->regs->data[APIC_TMR]) )
>> >>          vioapic_update_EOI(d, vector); diff --git
>> >> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
>> >> 8fca08c..29d9bbf 100644
>> >> --- a/xen/arch/x86/hvm/vmx/intr.c
>> >> +++ b/xen/arch/x86/hvm/vmx/intr.c
>> >> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
>> >>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
>> >>              __vmwrite(EOI_EXIT_BITMAP(i),
>> >v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>> >>          }
>> >> -
>> >> -        pt_intr_post(v, intack);
>> >>      }
>> >>      else
>> >>      {
>> >>
>> >
>> >Because we update pt irq in every vmentry, 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 HW virtual interrupt injection (vIRR->0, vISR->1) and
>> >EOI-induced exit (vISR->0), since pt_intr_post hasn't been invoked
>> >yet. I guess this is the reason why you still see faster wallclock.
>> >
>>
>> Agreed. A good description. My bad description is from another aspect.
>>
>> >I think you need mark this pending_intr_post situation explicitly.
>> >Then pt_update_irq should skip such pt timer when pending_intr_post
>> >of that timer is true (otherwise the update is meaningless since
>> >previous one hasn't been posted yet). Then with your change to post
>> >in EOI-induced exit handler, it should work correctly to meet the
>> >goal
>> >- one virtual interrupt delivery for one pending pt intr...
>> >
>> I think we are at least on the right track.
>> But I can't follow ' pending_intr_post ', a new parameter? Thanks.
>>
>>
>
>yes, a new parameter to record whether a intr_post operation is pending


The existing parameter ' irq_issued ' looks good. I have tested with below modification last night, and it is working. 
If it is okay, I will send out v2..

Quan

==== modification =====
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1d5d287..cc247c3 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
 void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
 {
     struct domain *d = vlapic_domain(vlapic);
+    struct hvm_intack pt_intack;
+
+    pt_intack.vector = vector;
+    pt_intack.source = hvm_intsrc_lapic;
+    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);

     if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
         vioapic_update_EOI(d, vector);
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 8fca08c..29d9bbf 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -333,8 +333,6 @@ void vmx_intr_assist(void)
             clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
             __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
         }
-
-        pt_intr_post(v, intack);
     }
     else
     {
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 5c48fdb..620ca68 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -267,6 +267,11 @@ int pt_update_irq(struct vcpu *v)
         return -1;
     }

+    if ( earliest_pt->irq_issued )
+    {
+        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
+        return -1;
+    }
     earliest_pt->irq_issued = 1;
     irq = earliest_pt->irq;
     is_lapic = (earliest_pt->source == PTSRC_lapic);


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

Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Tian, Kevin]

> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> Sent: Tuesday, September 20, 2016 8:25 AM
> 
> On September 19, 2016 5:25 PM, Tian Kevin <kevin.tian@intel.com> wrote:
> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> Sent: Monday, September 12, 2016 5:08 PM
> >>
> >> On September 12, 2016 3:58 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
> >> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> >> Sent: Friday, September 09, 2016 11:02 AM
> >> >>
> >> >> On August 30, 2016 1:58 PM, Tian Kevin < kevin.tian@intel.com > wrote:
> >> >> >> From: Xuquan (Euler) [mailto:xuquan8@huawei.com]
> >> >> >> Sent: Friday, August 19, 2016 8:59 PM
> >> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> >> >> index 1d5d287..cc247c3 100644
> >> >> --- a/xen/arch/x86/hvm/vlapic.c
> >> >> +++ b/xen/arch/x86/hvm/vlapic.c
> >> >> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >> >> void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)  {
> >> >>      struct domain *d = vlapic_domain(vlapic);
> >> >> +    struct hvm_intack pt_intack;
> >> >> +
> >> >> +    pt_intack.vector = vector;
> >> >> +    pt_intack.source = hvm_intsrc_lapic;
> >> >> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
> >> >>
> >> >>      if ( vlapic_test_and_clear_vector(vector,
> >> >&vlapic->regs->data[APIC_TMR]) )
> >> >>          vioapic_update_EOI(d, vector); diff --git
> >> >> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
> >> >> 8fca08c..29d9bbf 100644
> >> >> --- a/xen/arch/x86/hvm/vmx/intr.c
> >> >> +++ b/xen/arch/x86/hvm/vmx/intr.c
> >> >> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
> >> >>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
> >> >>              __vmwrite(EOI_EXIT_BITMAP(i),
> >> >v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >> >>          }
> >> >> -
> >> >> -        pt_intr_post(v, intack);
> >> >>      }
> >> >>      else
> >> >>      {
> >> >>
> >> >
> >> >Because we update pt irq in every vmentry, 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 HW virtual interrupt injection (vIRR->0, vISR->1) and
> >> >EOI-induced exit (vISR->0), since pt_intr_post hasn't been invoked
> >> >yet. I guess this is the reason why you still see faster wallclock.
> >> >
> >>
> >> Agreed. A good description. My bad description is from another aspect.
> >>
> >> >I think you need mark this pending_intr_post situation explicitly.
> >> >Then pt_update_irq should skip such pt timer when pending_intr_post
> >> >of that timer is true (otherwise the update is meaningless since
> >> >previous one hasn't been posted yet). Then with your change to post
> >> >in EOI-induced exit handler, it should work correctly to meet the
> >> >goal
> >> >- one virtual interrupt delivery for one pending pt intr...
> >> >
> >> I think we are at least on the right track.
> >> But I can't follow ' pending_intr_post ', a new parameter? Thanks.
> >>
> >>
> >
> >yes, a new parameter to record whether a intr_post operation is pending
> 
> 
> The existing parameter ' irq_issued ' looks good. I have tested with below modification last
> night, and it is working.
> If it is okay, I will send out v2..

yes, looks it could serve the purpose. 

> 
> Quan
> 
> ==== modification =====
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 1d5d287..cc247c3 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  {
>      struct domain *d = vlapic_domain(vlapic);
> +    struct hvm_intack pt_intack;
> +
> +    pt_intack.vector = vector;
> +    pt_intack.source = hvm_intsrc_lapic;
> +    pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
> 
>      if ( vlapic_test_and_clear_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(d, vector);
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 8fca08c..29d9bbf 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
>              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
>              __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm_vmx.eoi_exit_bitmap[i]);
>          }
> -
> -        pt_intr_post(v, intack);
>      }
>      else
>      {
> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> index 5c48fdb..620ca68 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -267,6 +267,11 @@ int pt_update_irq(struct vcpu *v)
>          return -1;
>      }
> 
> +    if ( earliest_pt->irq_issued )
> +    {
> +        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +        return -1;
> +    }

You need move the check within the loop, so other pt timers are
not blocked in such situation...

Thanks
Kevin

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