diff mbox

[v5,19/20] KVM: arm/arm64: Get rid of kvm_timer_flush_hwstate

Message ID 1509093281-15225-20-git-send-email-cdall@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Christoffer Dall Oct. 27, 2017, 8:34 a.m. UTC 
Now when both the vtimer and the ptimer when using both the in-kernel
vgic emulation and a userspace IRQ chip are driven by the timer signals
and at the vcpu load/put boundaries, instead of recomputing the timer
state at every entry/exit to/from the guest, we can get entirely rid of
the flush hwstate function.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_arch_timer.h |  1 -
 virt/kvm/arm/arch_timer.c    | 24 ------------------------
 virt/kvm/arm/arm.c           |  1 -
 3 files changed, 26 deletions(-)

Comments

Andrew Jones Nov. 27, 2017, 4:50 p.m. UTC | #1 
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?

Thanks,
drew
Christoffer Dall Nov. 29, 2017, 5:39 p.m. UTC | #2 
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
Andrew Jones Nov. 29, 2017, 6:17 p.m. UTC | #3 
On Wed, Nov 29, 2017 at 06:39:00PM +0100, Christoffer Dall wrote:
> 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?

Sounds good to me. Thanks!

drew
diff mbox

Patch

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index c538f70..2352f3a 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -66,7 +66,6 @@  int kvm_timer_hyp_init(void);
 int kvm_timer_enable(struct kvm_vcpu *vcpu);
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
 bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu);
 void kvm_timer_update_run(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 20e56c4..53d9bd4 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -305,12 +305,6 @@  static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
-	/*
-	 * If userspace modified the timer registers via SET_ONE_REG before
-	 * the vgic was initialized, we mustn't set the vtimer->irq.level value
-	 * because the guest would never see the interrupt.  Instead wait
-	 * until we call this function from kvm_timer_flush_hwstate.
-	 */
 	if (unlikely(!timer->enabled))
 		return;
 
@@ -489,24 +483,6 @@  bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
 	       ptimer->irq.level != plevel;
 }
 
-/**
- * kvm_timer_flush_hwstate - prepare timers before running the vcpu
- * @vcpu: The vcpu pointer
- *
- * Check if the virtual timer has expired while we were running in the host,
- * and inject an interrupt if that was the case, making sure the timer is
- * masked or disabled on the host so that we keep executing.  Also schedule a
- * software timer for the physical timer if it is enabled.
- */
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
-{
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
-
-	if (unlikely(!timer->enabled))
-		return;
-}
-
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
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);
 
 		/*