On 23.09.16 10:57, Paolo Bonzini wrote:
>
>
> On 23/09/2016 09:14, Alexander Graf wrote:
>>>> +/*
>>>> + * 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?
Yup :)
>
>>> 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).
The beauty of having it in the timer update function is that it gets
called from flush_hwstate() as well. That way we catch the update also
in cases where we need it before we enter the vcpu.
> 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?
We could do that too, yes. But it puts more burden on user space - it
would have to ensure that it *always* checks for the pending timer
status. With this approach, user space may opt to only check for timer
state changes on -EINTR and things would still work.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
