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

["Xuquan (Euler)" <xuquan8@xxxxxxxxxx>]

On August 22, 2016 11:12 PM, <JBeulich@xxxxxxxx> wrote:
>>>> On 22.08.16 at 16:02, <xuquan8@xxxxxxxxxx> wrote:
>> On August 22, 2016 8:04 PM, <JBeulich@xxxxxxxx> wrote:
>>>>>> On 22.08.16 at 13:41, <xuquan8@xxxxxxxxxx> wrote:
>>>> On August 22, 2016 6:36 PM, <JBeulich@xxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel