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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.08.16 at 13:40, <yang.zhang.wz@xxxxxxxxx> wrote:
> On 2016/8/22 18:35, Jan Beulich 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?
> 
> "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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel