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).
-Christoffer
--
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
