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

["Jan Beulich" <JBeulich@suse.com>]

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