On Tue, Sep 20, 2016 at 06:28:15PM +0100, Marc Zyngier wrote:
> Alex,
>
> On 16/09/16 06:16, Alexander Graf wrote:
> > While adding the new vgic implementation, apparently nobody tested
> > the non-vgic path where user space controls the vgic, so two functions
> > slipped through the cracks that get called in generic code but don't
> > check whether hardware support is enabled.
> >
> > This patch guards them with proper checks to ensure we only try to
> > use vgic data structures if they are available. Without this, I get
> > a stack trace:
> >
> > [ 74.363037] Unable to handle kernel paging request at virtual address
> > ffe8
> > [...]
> > [ 74.929654] [] _raw_spin_lock+0x1c/0x58
> > [ 74.935133] [] kvm_vgic_flush_hwstate+0x88/0x288
> > [ 74.941406] [] kvm_arch_vcpu_ioctl_run+0xfc/0x630
> > [ 74.947766] [] kvm_vcpu_ioctl+0x2f4/0x710
> > [ 74.953420] [] do_vfs_ioctl+0xb0/0x728
> > [ 74.958807] [] SyS_ioctl+0x94/0xa8
> > [ 74.963844] [] el0_svc_naked+0x38/0x3c
> >
> > Fixes: 0919e84c0
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Alexander Graf
> > ---
> > virt/kvm/arm/vgic/vgic.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index e83b7fe..9f312ba 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -645,6 +645,9 @@ next:
> > /* Sync back the hardware VGIC state into our emulation after a guest's
> > run. */
> > void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> > {
> > + if (!vcpu->kvm->arch.vgic.enabled)
> > + return;
> > +
> > vgic_process_maintenance_interrupt(vcpu);
> > vgic_fold_lr_state(vcpu);
> > vgic_prune_ap_list(vcpu);
> > @@ -653,6 +656,9 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> > /* Flush our emulation state into the GIC hardware before entering the
> > guest. */
> > void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
> > {
> > + if (!vcpu->kvm->arch.vgic.enabled)
> > + return;
> > +
this is not correct, because it checks if the distributor is enabled,
not if the vgic as a thing in KVM is enabled. (The distributor can be
disabled, but a VCPU should still be able to EOI an active interrupt,
for example).
So this check should be
if (!vgic_initialized(vcpu->kvm))
return;
> > spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > vgic_flush_lr_state(vcpu);
> > spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >
>
> I hate that fix, because it papers over the fact that we have uninitialized
> structures all over the shop, and that's not exactly great.
I'm not completely convinced about this, because we have
vgic_initialized() checks in the arch timer code as well, and I can't
easily figure out if initializing all data structures etc. to a shim
would work for all vgic interactions.
Basically, I think we have a choice between
(1) locate *all* entry points to the gic code, and make sure they're
guarded with vgic_initialized(), or
(2) do something like you suggest and still go through all interactions
between the vgic and the rest of the system and ensure that whatever
shim/empty data structures we've allocated, actually end up doing the
right thing.
My gut feeling is to lean towards (1), but I don't feel overly strongly
about that.
FWIW: A few comments on the patch below:
>
> How about the following instead:
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index c94b90d..0961128 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -472,6 +472,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> return ret;
> }
>
> + if (unlikely(!irqchip_in_kernel(kvm)))
> + kvm_no_vgic_init(kvm);
> +
> /*
>* Enable the arch timers only if we have an in-kernel VGIC
>* and it has been properly initialized, since we cannot handle
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index bb46c03..1b70b1e 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -327,4 +327,6 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct
> kvm_msi *msi);
> */
> int kvm_vgic_setup_default_irq_routing(struct kvm *kvm);
>
> +void kvm_no_vgic_init(struct kvm *kvm);
> +
> #endif /* __KVM_ARM_VGIC_H */
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 83777c1..7b8f12b 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -151,9 +151,11 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned
> int nr_spis)
> INIT_LIST_HEAD(&dist->lpi_list_head);
> spin_lock_init(&dist->lpi_list_lock);
>
> - dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq), GFP_KERNEL);
> - if (!dist->spis)
> - return -ENOMEM;
> + if (nr_spis) {
> + dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq),
> GFP_KERNEL);
> + if (!dist->spis)
> +
