[PATCH v2 2/2] KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to kvm_create_vgic()

[Changyuan Lyu]

Hi Marc,

On Wed, Feb 12, 2025 at 18:25:58 +0000, Marc Zyngier <maz at kernel.org> wrote:
> If userspace creates vcpus, then a vgic, we end-up in a situation
> where irqchip_in_kernel() will return true, but no private interrupt
> has been allocated for these vcpus. This situation will continue
> until userspace initialises the vgic, at which point we fix the
> early vcpus. Should a vcpu run or be initialised in the interval,
> bad things may happen.
>
> An obvious solution is to move this fix-up phase to the point where
> the vgic is created. This ensures that from that point onwards,
> all vcpus have their private interrupts, as new vcpus will directly
> allocate them.

I have a concern that this patch might cause an issue if userspace creates
VCPUs *after* a VGIC is created but *before* it has been initialized via
`KVM_DEV_ARM_VGIC_CTRL_INIT`.

For example, consider the following call sequence:

1.  Create VGIC via `KVM_CREATE_DEVICE`.
2.  Create all VCPUs.
3.  Issue `KVM_ARM_VCPU_INIT` to all VCPUs.
4.  Issue `KVM_DEV_ARM_VGIC_CTRL_INIT` to the VGIC.

> With that, we have the invariant that when irqchip_in_kernel() is
> true, all vcpus have their private interrupts.
>
> Reported-by: Alexander Potapenko <glider at google.com>
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index bc7e22ab5d812..775461cf2d2db 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> [...]
>
> -static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu)
> +static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu, u32 type)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	int i;
> @@ -218,17 +236,28 @@ static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu)
>  			/* PPIs */
>  			irq->config = VGIC_CONFIG_LEVEL;
>  		}
> +
> +		switch (type) {
> +		case KVM_DEV_TYPE_ARM_VGIC_V3:
> +			irq->group = 1;
> +			irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
> +			break;

`vgic_allocate_private_irqs_locked()` appears to be called in two scenarios:

a) When the VGIC is created, in `kvm_vgic_create()`.
b) When a VCPU is created, in `kvm_vm_ioctl_create_vcpu()`.

For scenario (b), the call path is:

`kvm_vm_ioctl_create_vcpu()`
-> `kvm_arch_vcpu_create()`
  -> `kvm_vgic_vcpu_init()`
    -> `vgic_allocate_private_irqs()`
      -> `vgic_allocate_private_irqs_locked()`

However, since the VCPU has just been created, its `MPIDR_EL1` register
value has not been assigned at this point. The VCPU's `MPIDR_EL1`
register is populated later when `KVM_ARM_VCPU_INIT` is issued, via
the following call path:

`kvm_arch_vcpu_ioctl_vcpu_init()`
-> `kvm_vcpu_set_target()`
  -> `__kvm_vcpu_set_target()`
    -> `kvm_reset_vcpu()`
      -> `kvm_reset_sys_regs()`
        -> `reset_mpidr()`

Therefore, with the call sequence I mentioned at the beginning,
`irq->mpidr` would be assigned an uninitialized value. This suggests that
for `irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu)` to work correctly, we
must rely on the code path from scenario (a). This would imply that
with this patch, all VCPUs must be created and initialized before the
VGIC is created.

> +		case KVM_DEV_TYPE_ARM_VGIC_V2:
> +			irq->group = 0;
> +			irq->targets = BIT(vcpu->vcpu_id);
> +			break;
> +		}
>  	}
>
>  	return 0;
>  }
>
> -static int vgic_allocate_private_irqs(struct kvm_vcpu *vcpu)
> +static int vgic_allocate_private_irqs(struct kvm_vcpu *vcpu, u32 type)
>  {
>  	int ret;
>
>  	mutex_lock(&vcpu->kvm->arch.config_lock);
> -	ret = vgic_allocate_private_irqs_locked(vcpu);
> +	ret = vgic_allocate_private_irqs_locked(vcpu, type);
>  	mutex_unlock(&vcpu->kvm->arch.config_lock);
>
>  	return ret;
> @@ -258,7 +287,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	if (!irqchip_in_kernel(vcpu->kvm))
>  		return 0;
>
> -	ret = vgic_allocate_private_irqs(vcpu);
> +	ret = vgic_allocate_private_irqs(vcpu, dist->vgic_model);
>  	if (ret)
>  		return ret;
>
> [...]

Please let me know if my understanding is incorrect. Thank you!

Best,
Changyuan