From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Zhang Subject: Re: [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue Date: Mon, 22 Aug 2016 22:07:31 +0800 Message-ID: References: <57BAF1950200007800107DCE@prv-mh.provo.novell.com> <57BB066D0200007800107EDA@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1935913584574868574==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: "Xuquan (Euler)" Cc: Kevin Tian , Jan Beulich , "George.Dunlap@eu.citrix.com" , Andrew Cooper , "xen-devel@lists.xen.org" , "jun.nakajima@intel.com" List-Id: xen-devel@lists.xenproject.org --===============1935913584574868574== Content-Type: multipart/alternative; boundary=001a114aa6beb5caec053aa99191 --001a114aa6beb5caec053aa99191 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 2016=E5=B9=B48=E6=9C=8822=E6=97=A5=E6=98=9F=E6=9C=9F=E4=B8=80=EF=BC=8CXuqua= n (Euler) =E5=86=99=E9=81=93=EF=BC=9A > On August 22, 2016 8:04 PM, > wrote: > >>>> On 22.08.16 at 13:41, > wrote: > >> On August 22, 2016 6:36 PM, > wrote: > >>>>>> On 19.08.16 at 14:58, wrote: > >>>> From 9b2df963c13ad27e2cffbeddfa3267782ac3da2a Mon Sep 17 00:00:00 > >>>> 2001 > >>>> From: Quan Xu > >>>> 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 lik= e > 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 =3D is_pt_irq(v, intack); > if ( pt =3D=3D 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 =3D=3D 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 > --=20 best regards yang --001a114aa6beb5caec053aa99191 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

2016=E5=B9=B48=E6=9C=8822=E6=97=A5=E6=98=9F=E6=9C=9F=E4=B8=80=EF=BC= =8CXuquan (Euler) <xuquan8@huawei.= com> =E5=86=99=E9=81=93=EF=BC=9A
O= n August 22, 2016 8:04 PM, <JBeulich@suse.com> w= rote:
>>>> On 22.08.16 at 13:41, <xuquan8@huawei= .com> wrote:
>> On August 22, 2016 6:36 PM, <JBeulich@suse.c= om> wrote:
>>>>>> On 19.08.16 at 14:58, <xuquan8@xxxxxxxxxx> w= rote:
>>>> 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 Wi= ndows7-32
>>>> guest with high payload (with 2vCPU, captured from xentrac= e, in high
>>>> payload, the count of IPI interrupt increases rapidly betw= een these vCPUs).
>>>>
>>>> If IPI intrrupt (vector 0xe1) and periodic timer interrupt= (vector
>>>> 0xd1) are both pending (index of bit set in VIRR), unfortu= nately,
>>>> the IPI intrrupt is high priority than periodic timer inte= rrupt. Xen
>>>> updates IPI interrupt bit set in VIRR to guest interrupt s= tatus
>>>> (RVI) as a high priority and apicv (Virtual-Interrupt Deli= very)
>>>> delivers IPI interrupt within VMX non-root operation witho= ut a VM
>>>> exit. Within VMX non-root operation, if periodic timer int= errupt
>>>> index of bit is set in VIRR and highest, the apicv deliver= s periodic timer
>interrupt within VMX non-root operation as well.
>>>>
>>>> But in current code, if Xen doesn't update periodic ti= mer 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 ti= mer
>>>> interrupt.
>>>>
>>>> If the periodic timer interrut is delivered and not the hi= ghest
>>>> priority, make Xen be aware of this case to decrease the c= ount 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 be= comes
>>>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 p= t one..
>> Then:
>>
>>=C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else
>>=C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pt_intr_post(v, i= ntack);
>
>As just said in reply to Yang: If this is still the same interrupt inst= ance as in a
>prior run through here which took the if() branch, this change looks li= ke having
>the potential of double accounting.
>

I very appreciate your detail review. It looks like, but actually it doesn&= #39;t happen.

=C2=A0As the key parameter 'pt->irq_issued'..

In pt_update_irq(), once the PT irq is issued, set the pt->irq_issued..<= br> In pt_intr_post(), clear the pt->irq_issued before touching the count &#= 39;pt->pending_intr_nr'..

According to your assumption, at the second call to pt_intr_post(), As if &= #39;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)
{
...
=C2=A0 =C2=A0 pt =3D is_pt_irq(v, intack);
=C2=A0 =C2=A0 if ( pt =3D=3D NULL )
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 spin_unlock(&v->arch.hvm_vcpu.tm_lo= ck);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
=C2=A0 =C2=A0 }
...


...
}


static struct periodic_time *is_pt_irq()
{
=C2=A0....
=C2=A0 =C2=A0 list_for_each_entry ( pt, head, list )
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( pt->pending_intr_nr && ________= pt->irq_issued_______ &&
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(intack.vector =3D=3D pt_ir= q_vector(pt, intack.source)) )
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return pt;
=C2=A0 =C2=A0 }

=C2=A0 =C2=A0 return NULL;
}





__IIUC__, this question is based on the following pseudocode detail the beh= avior 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
=C2=A0 =C2=A0Then RVI<-highest index of bit set in VIRR
=C2=A0 =C2=A0ELSE 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.
=C2=A0

Quan


--
best regards
yang

--001a114aa6beb5caec053aa99191-- --===============1935913584574868574== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1935913584574868574==--