All of lore.kernel.org
 help / color / mirror / Atom feed
From: christoffer.dall@linaro.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs
Date: Thu, 11 Dec 2014 13:01:58 +0100	[thread overview]
Message-ID: <20141211120158.GG28388@cbox> (raw)
In-Reply-To: <5488407E.50506@linaro.org>

On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
> On 12/09/2014 04:44 PM, 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, 
> Actually we did with VFIO signaling setup before VGIC init!
> presumably because

well, not with code in mainline

> > 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@linaro.org>
> > ---
> >  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);
> > +		mutex_unlock(&kvm->lock);
> > +	}
> I was previously encouraged to test the virtual interrupt controller
> readiness when setting irqfd up(proposal made in
> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
> correct? Reviewed-by on the whole series.
> 
I think we should move to your userspace explicit init for all
non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
userspace explicitly initializing the vgic.

Thanks for the review!

-Christoffer

WARNING: multiple messages have this Message-ID (diff)
From: Christoffer Dall <christoffer.dall@linaro.org>
To: Eric Auger <eric.auger@linaro.org>
Cc: kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs
Date: Thu, 11 Dec 2014 13:01:58 +0100	[thread overview]
Message-ID: <20141211120158.GG28388@cbox> (raw)
In-Reply-To: <5488407E.50506@linaro.org>

On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
> On 12/09/2014 04:44 PM, 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, 
> Actually we did with VFIO signaling setup before VGIC init!
> presumably because

well, not with code in mainline

> > 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@linaro.org>
> > ---
> >  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);
> > +		mutex_unlock(&kvm->lock);
> > +	}
> I was previously encouraged to test the virtual interrupt controller
> readiness when setting irqfd up(proposal made in
> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
> correct? Reviewed-by on the whole series.
> 
I think we should move to your userspace explicit init for all
non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
userspace explicitly initializing the vgic.

Thanks for the review!

-Christoffer

  reply	other threads:[~2014-12-11 12:01 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-09 15:43 [PATCH 0/5] Fix vgic initialization problems Christoffer Dall
2014-12-09 15:43 ` Christoffer Dall
2014-12-09 15:44 ` [PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps() Christoffer Dall
2014-12-09 15:44   ` Christoffer Dall
2014-12-10 10:11   ` Eric Auger
2014-12-10 10:11     ` Eric Auger
2014-12-11 11:48     ` Christoffer Dall
2014-12-11 11:48       ` Christoffer Dall
2014-12-11 18:25       ` Marc Zyngier
2014-12-11 18:25         ` Marc Zyngier
2014-12-09 15:44 ` [PATCH 2/5] arm/arm64: KVM: Rename vgic_initialized to vgic_ready Christoffer Dall
2014-12-09 15:44   ` Christoffer Dall
2014-12-11 18:26   ` Marc Zyngier
2014-12-11 18:26     ` Marc Zyngier
2014-12-09 15:44 ` [PATCH 3/5] arm/arm64: KVM: Add (new) vgic_initialized macro Christoffer Dall
2014-12-09 15:44   ` Christoffer Dall
2014-12-10 10:27   ` Eric Auger
2014-12-10 10:27     ` Eric Auger
2014-12-11 11:48     ` Christoffer Dall
2014-12-11 11:48       ` Christoffer Dall
2014-12-11 18:28   ` Marc Zyngier
2014-12-11 18:28     ` Marc Zyngier
2014-12-09 15:44 ` [PATCH 4/5] arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized Christoffer Dall
2014-12-09 15:44   ` Christoffer Dall
2014-12-10 12:35   ` Eric Auger
2014-12-10 12:35     ` Eric Auger
2014-12-11 11:55     ` Christoffer Dall
2014-12-11 11:55       ` Christoffer Dall
2014-12-11 18:30   ` Marc Zyngier
2014-12-11 18:30     ` Marc Zyngier
2014-12-09 15:44 ` [PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs Christoffer Dall
2014-12-09 15:44   ` Christoffer Dall
2014-12-10 12:45   ` Eric Auger
2014-12-10 12:45     ` Eric Auger
2014-12-11 12:01     ` Christoffer Dall [this message]
2014-12-11 12:01       ` Christoffer Dall
2014-12-11 12:38       ` Eric Auger
2014-12-11 12:38         ` Eric Auger
2014-12-12 11:06         ` Christoffer Dall
2014-12-12 11:06           ` Christoffer Dall
2014-12-15 10:43           ` Eric Auger
2014-12-15 10:43             ` Eric Auger
2014-12-11 18:35   ` Marc Zyngier
2014-12-11 18:35     ` Marc Zyngier
2014-12-12 11:14     ` Christoffer Dall
2014-12-12 11:14       ` Christoffer Dall
2014-12-12 11:23       ` Marc Zyngier
2014-12-12 11:23         ` Marc Zyngier
2014-12-12 11:37         ` Christoffer Dall
2014-12-12 11:37           ` Christoffer Dall
2014-12-12 20:24           ` Christoffer Dall
2014-12-12 20:24             ` Christoffer Dall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141211120158.GG28388@cbox \
    --to=christoffer.dall@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.