| Message ID | 1509093281-15225-5-git-send-email-cdall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 27/10/17 09:34, Christoffer Dall wrote: > If the vgic is not initialized, don't try to grab its spinlocks or > traverse its data structures. > > This is important because we soon have to start considering the active > state of a virtual interrupts when doing vcpu_load, which may happen > early on before the vgic is initialized. I understand this patch is on its way to Linus already, but I just found this by browsing for VGIC changes... > Signed-off-by: Christoffer Dall <cdall@linaro.org> > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/vgic/vgic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c > index fed717e..e1f7dbc 100644 > --- a/virt/kvm/arm/vgic/vgic.c > +++ b/virt/kvm/arm/vgic/vgic.c > @@ -777,6 +777,9 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq) > struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq); Isn't vgic_get_irq() already accessing VGIC data structures? In which case this assignment should be moved after the vgic_initialized() check below? Cheers, Andre. > bool map_is_active; > > + if (!vgic_initialized(vcpu->kvm)) > + return false; > + > spin_lock(&irq->irq_lock); > map_is_active = irq->hw && irq->active; > spin_unlock(&irq->irq_lock); >
On Thu, Nov 16, 2017 at 12:29:48PM +0000, Andre Przywara wrote: > Hi, > > On 27/10/17 09:34, Christoffer Dall wrote: > > If the vgic is not initialized, don't try to grab its spinlocks or > > traverse its data structures. > > > > This is important because we soon have to start considering the active > > state of a virtual interrupts when doing vcpu_load, which may happen > > early on before the vgic is initialized. > > I understand this patch is on its way to Linus already, but I just found > this by browsing for VGIC changes... > > > Signed-off-by: Christoffer Dall <cdall@linaro.org> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > virt/kvm/arm/vgic/vgic.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c > > index fed717e..e1f7dbc 100644 > > --- a/virt/kvm/arm/vgic/vgic.c > > +++ b/virt/kvm/arm/vgic/vgic.c > > @@ -777,6 +777,9 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq) > > struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq); > > Isn't vgic_get_irq() already accessing VGIC data structures? In which > case this assignment should be moved after the vgic_initialized() check > below? Theoretically, yes, but we're saved by two things: First, this is only ever called on PPIs from the timer code, which are always statically allocated as part of the VCPU. Second, this isn't actually needed anymore after the irqchip in userspace support, which partially reworked the timer init sequence (done after the first version of these patches), so this patch could actually have been entirely dropped, or reworked as you suggest. Thanks, -Christoffer
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index fed717e..e1f7dbc 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -777,6 +777,9 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq) struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq); bool map_is_active; + if (!vgic_initialized(vcpu->kvm)) + return false; + spin_lock(&irq->irq_lock); map_is_active = irq->hw && irq->active; spin_unlock(&irq->irq_lock);