On 23/09/2016 09:14, Alexander Graf wrote:

quoted

quoted

+/*
+ * Synchronize the timer IRQ state with the interrupt controller.
+ */
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 {
 	int ret;
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
 	timer->active_cleared_last = false;
 	timer->irq.level = new_level;
-	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
+	trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
 				   timer->irq.level);
+	[...]
+		struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+		/* Populate the timer bitmap for user space */
+		regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
+		if (new_level)
+			regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;

I think if you got here, it means you have to exit to userspace to
update it of the new state.  If you don't want to propagate a return

Yes, but we can't exit straight away with our own exit reason because we
might be inside an MMIO exit path here which already occupies the
exit_reason.

So the idea is that whenever you're here you have one of the following
cases:

- are coming from kvm_timer_flush_hwstate, and then you exit immediately
with KVM_EXIT_INTR if needed

- you are coming from the kvm_timer_sync_hwstate just before
handle_exit.  Then if there's a vmexit you have already set
regs->kernel_timer_pending, if not you'll do a kvm_timer_flush_hwstate soon.

- you are coming from the kvm_timer_sync_hwstate in the middle of
kvm_arch_vcpu_ioctl_run, and then "continue" will either exit the loop
immediately (if ret <= 0) or go to kvm_timer_flush_hwstate as in the
previous case

Right?

quoted

Maybe I'm misunderstanding and user_timer_pending is just a cached
verison of what you said last, but as I said above, I think you can just
compare timer->irq.level with the last value the kvm_run struct, and if
something changed, you have to exit.

So how would user space know whether the line went up or down? Or didn't
change at all (if we coalesce with an MMIO exit)?

It would save the status of the line somewhere in its own variable,
without introducing a relatively delicate API between kernel and user.

I agree that you cannot update kernel_timer_pending only in
flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
That has to stay in kvm_timer_sync_hwstate (or it could become a new
function called at the very end of kvm_arch_vcpu_ioctl_run).

However, I'm not sure why user_timer_pending is necessary.  If it is
just the value you assigned to regs->kernel_timer_pending at the time of
the previous vmexit, can kvm_timer_flush_hwstate just do

   if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
       timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
       return 1;
   }

?  Or even

   if (timer->prev_irq_level != timer->irq.level) {
       timer->prev_irq_level = regs->irq.level;
       return 1;
   }

so that regs->kernel_timer_pending remains exclusively
kvm_timer_sync_hwstate's business?

Thanks,

Paolo

quoted

quoted

+
+		/*
+		 * As long as user space is aware that the timer is pending,
+		 * we do not need to get new host timer events.
+		 */

yes, correct, but I don't think this concept was clearly reflected in
your API text above.

quoted

+		if (timer->irq.level)
+			disable_percpu_irq(host_vtimer_irq);
+		else
+			enable_percpu_irq(host_vtimer_irq, 0);
+	}

could we move these two blocks into their own functions instead?  That
would also give nice names to the huge chunk of complicated
functionality, e.g. flush_timer_state_to_user() and
flush_timer_state_to_vgic().

That's probably a very useful cleanup, yes :).


Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html