On Fri, Dec 12, 2014 at 09:22:26PM +0000, Marc Zyngier wrote:
> On 2014-12-12 20:24, Christoffer Dall wrote:
> >On Fri, Dec 12, 2014 at 12:37:52PM +0100, Christoffer Dall wrote:
> >>On Fri, Dec 12, 2014 at 11:23:35AM +0000, Marc Zyngier wrote:
> >>> On 12/12/14 11:14, Christoffer Dall wrote:
> >>> > On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote:
> >>> >> On 09/12/14 15:44, Christoffer Dall wrote:
> >>> >>> Userspace assumes that it can wire up IRQ injections after
> >>having
> >>> >>> created all VCPUs and after having created the VGIC, but
> >>potentially
> >>> >>> before starting the first VCPU. This can currently lead
> >>to lost IRQs
> >>> >>> because the state of that IRQ injection is not stored
> >>anywhere and we
> >>> >>> don't return an error to userspace.
> >>> >>>
> >>> >>> We haven't seen this problem manifest itself yet,
> >>presumably because
> >>> >>> guests reset the devices on boot, but this could cause
> >>issues with
> >>> >>> migration and other non-standard startup configurations.
> >>> >>>
> >>> >>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >>> >>> ---
> >>> >>> virt/kvm/arm/vgic.c | 9 +++++++--
> >>> >>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>> >>>
> >>> >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >>> >>> index c98cc6b..feef015 100644
> >>> >>> --- a/virt/kvm/arm/vgic.c
> >>> >>> +++ b/virt/kvm/arm/vgic.c
> >>> >>> @@ -1693,8 +1693,13 @@ out:
> >>> >>> int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid,
> >>unsigned int irq_num,
> >>> >>> bool level)
> >>> >>> {
> >>> >>> - if (likely(vgic_ready(kvm)) &&
> >>> >>> - vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> >>> >>> + if (unlikely(!vgic_initialized(kvm))) {
> >>> >>> + mutex_lock(&kvm->lock);
> >>> >>> + vgic_init(kvm);
> >>> >>
> >>> >> What if this fails?
> >>> >>
> >>> > yeah, not good. The thing is that we also don't check the
> >>return value
> >>> > from kvm_vgic_inject_irq(), so we can do two things:
> >>> >
> >>> > (1) change this function to a void, carry out the check for
> >>> > vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export
> >>> > vgic_init() outside of vgic.c.
> >>> >
> >>> > (2) just error out if vgic_init() fails and print a kernel
> >>error (or
> >>> > even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c.
> >>> >
> >>> > In both cases we need to make sure that we never configure
> >>the timer to
> >>> > begin injecting IRQs before the vgic is initialized, as Eric
> >>pointed out
> >>> > before.
> >>> >
> >>> > What do you think?
> >>>
> >>> I'd favour option two.
> >>>
> >>> My reasoning is that the timer interrupt is triggered by the
> >>HW. If it
> >>> has fired, that's because we've programmed it to trigger, with
> >>means a
> >>> vcpu has run. At that point, the vgic would better be
> >>initialized, or we
> >>> have something much nastier on our hands.
> >>>
> >>Sounds reasonable to me, I'll do a quick respin with the check
> >>for the
> >>timer (to ensure the user even created a vgic).
> >>
> >Just to double-check, it is going to look something like this for the
> >arch-timer path:
> >
> >diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >index d4da244..c61d51d 100644
> >--- a/arch/arm/kvm/arm.c
> >+++ b/arch/arm/kvm/arm.c
> >@@ -441,6 +441,16 @@ static int kvm_vcpu_first_run_init(struct
> >kvm_vcpu *vcpu)
> > return ret;
> > }
> >
> >+#ifdef CONFIG_KVM_ARM_TIMER
> >+ /*
> >+ * If the Architected Timers are supported, userspace must have
> >+ * created an in-kernel irqchip, since otherwise we will receive
> >+ * virtual timer interrupt and have nowhere to route them to.
> >+ */
> >+ if (!irqchip_in_kernel(kvm))
> >+ return -ENODEV;
> >+#endif
>
> Not quite sure about that. Supporting the vgic/timer is a runtime
> decision, not a compile-time one.
>
> My guess is that we should have this test in kvm_timer_init instead,
> and simply not set the "kvm->arch.timer.enabled" field to 1 if we
> don't have an in-kernel irqchip.
>
> This should ensure that the timer is never context-switched, never
> enabled, whatever the guest decides to do.
>
yeah, you're right, that's much nicer.
> >+
> > return 0;
> > }
> >
> >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >index 22fa819..b10e495 100644
> >--- a/virt/kvm/arm/arch_timer.c
> >+++ b/virt/kvm/arm/arch_timer.c
> >@@ -61,12 +61,14 @@ static void timer_disarm(struct arch_timer_cpu
> >*timer)
> >
> > static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
> > {
> >+ int ret;
> > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >
> > timer->cntv_ctl |= ARCH_TIMER_CTRL_IT_MASK;
> >- kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
> >- timer->irq->irq,
> >- timer->irq->level);
> >+ ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
> >+ timer->irq->irq,
> >+ timer->irq->level);
> >+ BUG_ON(ret);
> > }
> >
> > static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>
> I'd be tempted to have that as a WARN_ONCE(), or something similar.
> Annoying enough, but not fatal.
> But I'll leave you as sole judge, I'm becoming soft these days... ;-)
>
I changed it to a WARN_ON, see new series.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
