From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v2 4/9] KVM: arm/arm64: replace vcpu->arch.pause with a vcpu request Date: Tue, 4 Apr 2017 19:19:53 +0200 Message-ID: <20170404171953.GP11752@cbox> References: <20170331160658.4331-1-drjones@redhat.com> <20170331160658.4331-5-drjones@redhat.com> <20170404160417.GN11752@cbox> <06b2a225-192c-9c96-c092-6e0575dd9410@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: marc.zyngier@arm.com, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: <06b2a225-192c-9c96-c092-6e0575dd9410@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Tue, Apr 04, 2017 at 06:24:36PM +0200, Paolo Bonzini wrote: > > > On 04/04/2017 18:04, Christoffer Dall wrote: > >> For pause, only the requester should do the clearing. > > This suggests that maybe this should not be a request. The request > would be just the need to act on a GIC command, exactly as before this patch. Maybe the semantics should be: requester: vcpu: ---------- ----- make_requet(vcpu, KVM_REQ_PAUSE); handles the request by clearing it and setting vcpu->pause = true; wait until vcpu->pause == true make_request(vcpu, KVM_REQ_UNPAUSE); vcpus 'wake up' clear the UNPAUSE request and set vcpu->pause = false; The benefit would be that we get to re-use the complicated "figure out the VCPU mode and whether or not we should send an IPI and get the barriers right" stuff. > > What I don't understand is: > > >> With this patch, while the vcpu will still initially enter > >> the guest, it will exit immediately due to the IPI sent by the vcpu > >> kick issued after making the vcpu request. > > Isn't this also true of KVM_REQ_VCPU_EXIT that was used before? > > So this: > > + vcpu->arch.power_off || kvm_request_pending(vcpu)) { > + WRITE_ONCE(vcpu->mode, OUTSIDE_GUEST_MODE); > > is the crux of the fix, you can keep using vcpu->arch.pause. Probably; I feel like there's a fix here which should be a separate patch from using a different requests instead of the KVM_REQ_VCPU_EXIT + the pause flag. > > By the way, vcpu->arch.power_off can go away from this "if" too because > KVM_RUN and KVM_SET_MP_STATE are mutually exclusive through the vcpu mutex. But we also allow setting the power_off flag from the in-kernel PSCI emulation in the context of another VCPU thread. > The earlier check is enough: > > if (vcpu->arch.power_off || vcpu->arch.pause) > vcpu_sleep(vcpu); > > > >> + /* > >> + * Indicate we're in guest mode now, before doing a final > >> + * check for pending vcpu requests. The general barrier > >> + * pairs with the one in kvm_arch_vcpu_should_kick(). > >> + * Please see the comment there for more details. > >> + */ > >> + WRITE_ONCE(vcpu->mode, IN_GUEST_MODE); > >> + smp_mb(); > > > > There are two changes here: > > > > there's a change from a normal write to a WRITE_ONCE and there's also a > > change to that adds a memory barrier. I feel like I'd like to know if > > these are tied together or two separate cleanups. I also wonder if we > > could split out more general changes from the pause thing to have a > > better log of why we changed the run loop? > > You probably should just use smp_store_mb here. > That looks cleaner at least. Thanks, -Christoffer