Re: [PATCH v5 04/20] KVM: arm/arm64: Guard kvm_vgic_map_is_active against !vgic_initialized

Christoffer Dall
2017-11-20T11:20:58Z

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