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?
-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
