From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC Date: Thu, 18 Feb 2016 15:18:18 +0100 Message-ID: <20160218141817.GA6289@potion.brq.redhat.com> References: <1455285574-27892-1-git-send-email-suravee.suthikulpanit@amd.com> <1455285574-27892-6-git-send-email-suravee.suthikulpanit@amd.com> <56BDFC72.7030905@redhat.com> <56C2C1BF.7010700@amd.com> <56C312E1.1080902@redhat.com> <20160216141330.GG10555@potion.brq.redhat.com> <56C354A5.4040807@redhat.com> <20160216180618.GA18952@potion.brq.redhat.com> <56C52B80.5050104@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , joro@8bytes.org, alex.williamson@redhat.com, gleb@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com To: Suravee Suthikulpanit Return-path: Content-Disposition: inline In-Reply-To: <56C52B80.5050104@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-02-18 09:25+0700, Suravee Suthikulpanit: > On 2/17/16 01:06, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>2016-02-16 17:56+0100, Paolo Bonzini: >>>>On 16/02/2016 15:13, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>>>>>Yeah, I think atomic there means that it won't race with other wr= ites to >>>>>>the same byte in IRR. We're fine as long as AVIC writes IRR befo= re >>>>>>checking IsRunning on every destination, which it seems to be. >>>> >>>>More precisely, if AVIC writes all IRRs (5.1) and ANDs all IsRunnin= g >>>>flags before checking the result of the AND (6). >>>> >>>>>>(It would, but I believe that AVIC designers made it sane and the= spec >>>>>> doesn't let me read it in a way that supports your theories.) >>>> >>>>I hope so as well, and you've probably convinced me. But I still t= hink >>>>the code is wrong in this patch. Let's look at the spec that you p= asted: >>The code definitely is wrong. I'll be more specific when disagreeing= , >>sorry. >> >=20 > Would you please be a bit more specific on what you think I am not do= ing > correctly to handle the #VMEXIT in the case of target not running bel= ow. >=20 > + case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN: > + kvm_lapic_reg_write(apic, APIC_ICR2, icrh); > + kvm_lapic_reg_write(apic, APIC_ICR, icrl); >=20 > This is actually not just writing to the register. Please note that w= riting > to APIC_ICR register would also be calling apic_send_ipi(), which res= ults in > injecting interrupts to the target core: Exactly. Injecting the interrupt in AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN handler is causing the double-injection bug that Paolo described. > Am I missing something? Probably that AVIC already wrote to all IRRs (and sent appropriate doorbells) before this VMEXIT, so KVM shouldn't repeat it. KVM just has to make sure that targeted VCPUs notice the interrupt, which means to kick (wake up) VCPUs that don't have IsRunning set. There is no need to do anything with running VCPUs, because they - are in guest mode and noticed the doorbell - are in host mode, where they will 1) VMRUN as fast as they can because the VCPU didn't want to halt (and IRR is handled on VMRUN) 2) check IRR after unsetting IsRunning and goto (1) if there are pending interrupts. (RFC doesn't do this, which is another bug) It's still possible that we misunderstood the spec. Does AVIC handle IPIs differently? Thanks.