Re: [PATCH v2 1/2] KVM: arm64: vgic: Require matching VGIC model for GICv3 ITS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: arm64: vgic: vgic_has_its() returns true without an in-kernel VGIC
--

commit 06339a92609918e3caedb4339d3324ff581b8fee
Author: Sascha Bischoff <sascha.bischoff@arm.com>

KVM: arm64: vgic: Require matching VGIC model for GICv3 ITS

This commit records the VGIC model required by an ITS device in
struct vgic_dist and checks model compatibility during both VGIC and ITS
creation so that incompatible devices are rejected regardless of creation
order. It also replaces dist->has_its with a check on dist->its_model in
vgic_has_its() and changes dist->vgic_model to enum kvm_device_type.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index e5657cb603567..7df5869cff990 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1875,6 +1875,19 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>  
>  	mutex_lock(&dev->kvm->arch.config_lock);
>  
> +	/*
> +	 * Reject creation of a GICv3 ITS if:
> +	 * - The VM already has a VGIC other than GICv3.
> +	 * - An existing ITS requires a VGIC model other than GICv3.
> +	 */
> +	if ((irqchip_in_kernel(dev->kvm) && !vgic_is_v3(dev->kvm)) ||
> +	    (dev->kvm->arch.vgic.its_model &&
> +	     dev->kvm->arch.vgic.its_model != KVM_DEV_TYPE_ARM_VGIC_V3)) {
> +		mutex_unlock(&dev->kvm->arch.config_lock);
> +		kfree(its);
> +		return -ENODEV;
> +	}
> +
>  	if (vgic_initialized(dev->kvm)) {
>  		ret = vgic_v4_init(dev->kvm);
>  		if (ret < 0) {
> @@ -1902,7 +1915,7 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>  	xa_init(&its->translation_cache);
>  
>  	dev->kvm->arch.vgic.msis_require_devid = true;
> -	dev->kvm->arch.vgic.has_its = true;
> +	dev->kvm->arch.vgic.its_model = KVM_DEV_TYPE_ARM_VGIC_V3;
>  	its->enabled = false;
>  	its->dev = dev;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 5ab789accc9d4..5f0a5b12e338f 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -42,10 +42,7 @@ bool vgic_has_its(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  
> -	if (dist->vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> -		return false;
> -
> -	return dist->has_its;
> +	return !!dist->its_model;
>  }
>  
>  bool vgic_supports_direct_msis(struct kvm *kvm)

[Severity: Medium]
Can vgic_has_its() now return true when a VM has created an ITS device
without creating an in-kernel VGIC (dist->vgic_model == 0)?

Previously, vgic_has_its() returned false when dist->vgic_model was 0.
With this change, vgic_its_create() sets dist->its_model even when
!irqchip_in_kernel(dev->kvm), while skipping vgic_v4_init() because
vgic_initialized(dev->kvm) is false.

If userspace creates a KVM_DEV_TYPE_ARM_VGIC_ITS device and runs a vCPU
without creating an in-kernel VGIC (which kvm_vgic_map_resources() permits
when !irqchip_in_kernel(kvm)), vgic_supports_direct_msis() evaluates to
true on GICv4.0 hosts:

arch/arm64/kvm/vgic/vgic-mmio-v3.c:vgic_supports_direct_msis() {
	if (system_supports_direct_sgis() && !vgic_supports_direct_sgis(kvm))
		return false;

	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
}

In the vCPU run loop, kvm_arch_vcpu_ioctl_run() calls
kvm_vgic_flush_hwstate() unconditionally:

arch/arm64/kvm/vgic/vgic.c:kvm_vgic_flush_hwstate() {
	if (vgic_supports_direct_irqs(vcpu->kvm) && kvm_vgic_global_state.has_gicv4)
		vgic_v4_commit(vcpu);

	if (!kvm_vgic_vcpu_pending_irq(vcpu))
		return;
}

Wouldn't this call vgic_v4_commit() -> its_commit_vpe() ->
irq_set_vcpu_affinity(0, &info) on an uninitialized its_vpe where
vpe->irq == 0, as well as allowing MSI entry points such as kvm_set_msi(),
kvm_arch_set_irq_inatomic(), vgic_msi_to_its(), and
kvm_vgic_v4_set_forwarding() to proceed without an in-kernel VGIC?

Should vgic_has_its() still check irqchip_in_kernel(kvm) (or
dist->vgic_model == dist->its_model)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019134242.1289002-1-sascha.bischoff@arm.com?part=1