From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v5 19/20] KVM: arm/arm64: Get rid of kvm_timer_flush_hwstate Date: Wed, 29 Nov 2017 18:39:00 +0100 Message-ID: <20171129173900.GE10563@lvm> References: <1509093281-15225-1-git-send-email-cdall@linaro.org> <1509093281-15225-20-git-send-email-cdall@linaro.org> <20171127165012.2egz3dos6h4zj7ub@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Marc Zyngier , Eric Auger , kvm@vger.kernel.org, Catalin Marinas , Will Deacon To: Andrew Jones Return-path: Received: from mail-wr0-f196.google.com ([209.85.128.196]:42507 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934224AbdK2RjD (ORCPT ); Wed, 29 Nov 2017 12:39:03 -0500 Received: by mail-wr0-f196.google.com with SMTP id s66so4126998wrc.9 for ; Wed, 29 Nov 2017 09:39:03 -0800 (PST) Content-Disposition: inline In-Reply-To: <20171127165012.2egz3dos6h4zj7ub@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 27, 2017 at 05:50:12PM +0100, Andrew Jones wrote: > On Fri, Oct 27, 2017 at 10:34:40AM +0200, Christoffer Dall wrote: > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > index 132d39a..14c50d1 100644 > > --- a/virt/kvm/arm/arm.c > > +++ b/virt/kvm/arm/arm.c > > @@ -656,7 +656,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > > > > local_irq_disable(); > > > > - kvm_timer_flush_hwstate(vcpu); > > kvm_vgic_flush_hwstate(vcpu); > > > > /* > > -- > > 2.7.4 > > Hi Christoffer, > > I realize this is already merged, but I have a question about the > above hunk. IIUC, the patch "KVM: arm/arm64: Move timer/vgic > flush/sync under disabled irq" only moved kvm_vgic_flush_hwstate() > under disabled irq because it had to be run after > kvm_timer_flush_hwstate(). Now that kvm_timer_flush_hwstate() is > gone can/should it be moved back out? > That's a great question. I think my reasoning was, that since we now disable the VCPU interface (and thereby maintenance interrupts), any logic that relies on a maintenance interrupt firing and causing a guest exit after we've called flush, would no longer work if we move this back out where interrupts are enabled. Thinking about this a bit more, I don't think that would ever make sense, and we should be able to move it out. I've tested these patches pretty heavily and would want such a change to get the same kind of exposure, so I can start testing it, and if there are no issues, put it in next later on in the cycle. Does that sound reasonable? Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: cdall@linaro.org (Christoffer Dall) Date: Wed, 29 Nov 2017 18:39:00 +0100 Subject: [PATCH v5 19/20] KVM: arm/arm64: Get rid of kvm_timer_flush_hwstate In-Reply-To: <20171127165012.2egz3dos6h4zj7ub@kamzik.brq.redhat.com> References: <1509093281-15225-1-git-send-email-cdall@linaro.org> <1509093281-15225-20-git-send-email-cdall@linaro.org> <20171127165012.2egz3dos6h4zj7ub@kamzik.brq.redhat.com> Message-ID: <20171129173900.GE10563@lvm> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Nov 27, 2017 at 05:50:12PM +0100, Andrew Jones wrote: > On Fri, Oct 27, 2017 at 10:34:40AM +0200, Christoffer Dall wrote: > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > index 132d39a..14c50d1 100644 > > --- a/virt/kvm/arm/arm.c > > +++ b/virt/kvm/arm/arm.c > > @@ -656,7 +656,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > > > > local_irq_disable(); > > > > - kvm_timer_flush_hwstate(vcpu); > > kvm_vgic_flush_hwstate(vcpu); > > > > /* > > -- > > 2.7.4 > > Hi Christoffer, > > I realize this is already merged, but I have a question about the > above hunk. IIUC, the patch "KVM: arm/arm64: Move timer/vgic > flush/sync under disabled irq" only moved kvm_vgic_flush_hwstate() > under disabled irq because it had to be run after > kvm_timer_flush_hwstate(). Now that kvm_timer_flush_hwstate() is > gone can/should it be moved back out? > That's a great question. I think my reasoning was, that since we now disable the VCPU interface (and thereby maintenance interrupts), any logic that relies on a maintenance interrupt firing and causing a guest exit after we've called flush, would no longer work if we move this back out where interrupts are enabled. Thinking about this a bit more, I don't think that would ever make sense, and we should be able to move it out. I've tested these patches pretty heavily and would want such a change to get the same kind of exposure, so I can start testing it, and if there are no issues, put it in next later on in the cycle. Does that sound reasonable? Thanks, -Christoffer