Re: [PATCH v2 05/16] KVM: arm64: gic-v5: Add ITS device skeleton

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] KVM: arm64: gic-v5: unrestored base address in vgic_v5_register_its_iodev()
- [Low] KVM: arm64: gic-v5: missing tools UAPI header sync for GICv5 ITS definitions
- [Low] KVM: arm64: gic-v5: dead code in vgic_v5_register_its_iodev()
- [Low] KVM: arm64: gic-v5: stale GICv3 ITS description in commit message
--

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

KVM: arm64: gic-v5: Add ITS device skeleton

This commit introduces the initial KVM device plumbing for a GICv5 ITS,
including the UAPI device type and address attribute, struct vgic_v5_its
state, and a RAZ/WI MMIO region placeholder registered when userspace sets
the ITS base address.

>     KVM: arm64: gic-v5: Add ITS device skeleton
>     
>     Introduce the basic KVM device plumbing for a GICv5 ITS. This adds the
>     GICv5 ITS device type and address attribute, registers the device ops,
>     and wires a GICv5 ITS IODEV into the generic VGIC MMIO dispatch path.
>     
>     Add struct vgic_v5_its to hold the ITS base address, the IODEV plumbing,
>     the backing KVM device pointer, and the general ITS mutex. Allocate this
>     state from the KVM device create path, initialise the mutex and base
>     address, and mark the VM as having an ITS. GICv5 MSIs require a DeviceID,
>     so set msis_require_devid when the ITS is created.
>     
>     Record the VGIC model required by an ITS and reject mismatched device
>     creation in either order. Check GICv3 ITS creation as well, so the two
>     ITS models cannot be combined in one VM.
>     
>     The MMIO region is registered once userspace provides the ITS base
>     address through KVM_VGIC_V5_ADDR_TYPE_ITS. For now the IODEV only
>     provides a RAZ/WI placeholder region; the actual ITS register emulation
>     is added separately.
>     
>     Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>

[Severity: Low]
This isn't a bug, but is the third paragraph of the commit message stale?
struct vgic_dist::its_model, the check in kvm_vgic_create(), and the GICv3
ITS creation check in vgic_its_create() were already introduced in commit
05620a314219 ("KVM: arm64: vgic: Require matching VGIC and ITS models"),
whereas this commit only checks and sets its_model in vgic_v5_its_create().

> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 65ee09bd14636..8c701173a2d88 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -99,8 +99,10 @@ struct kvm_regs {
>  
>  /* Supported VGICv5 address types  */
>  #define KVM_VGIC_V5_ADDR_TYPE_IRS	6
> +#define KVM_VGIC_V5_ADDR_TYPE_ITS	7
>  
>  #define KVM_VGIC_V5_IRS_SIZE		(2 * SZ_64K)
> +#define KVM_VGIC_V5_ITS_SIZE		(2 * SZ_64K)
>  
>  #define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
>  #define KVM_ARM_VCPU_EL1_32BIT		1 /* CPU running a 32bit VM */

[Severity: Low]
Should tools/arch/arm64/include/uapi/asm/kvm.h also be updated with
KVM_VGIC_V5_ADDR_TYPE_ITS and KVM_VGIC_V5_ITS_SIZE so that
tools/perf/check-headers.sh does not report an out-of-sync header warning
during tools builds?

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> new file mode 100644
> index 0000000000000..5790b029f09b6
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
> @@ -0,0 +1,186 @@

[ ... ]

> +static int vgic_v5_register_its_iodev(struct kvm *kvm, struct vgic_v5_its *its,
> +				      u64 addr)
> +{
> +	struct vgic_io_device *iodev = &its->iodev;
> +	int ret;
> +
> +	mutex_lock(&kvm->slots_lock);
> +	if (!IS_VGIC_ADDR_UNDEF(its->vgic_v5_its_base)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	if (!IS_ALIGNED(addr, SZ_64K)) {
> +		kvm_err("ITS Base address is not aligned to 64k\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	its->vgic_v5_its_base = addr;
> +	iodev->base_addr = addr;
> +	iodev->regions = vgic_v5_its_registers;
> +	iodev->nr_regions = ARRAY_SIZE(vgic_v5_its_registers);
> +	iodev->iodev_type = IODEV_GICV5_ITS;
> +	iodev->redist_vcpu = NULL;
> +	iodev->v5its = its;

[Severity: Low]
This isn't a bug, but since vgic_v5_its_set_attr() is the only caller of
vgic_v5_register_its_iodev() and already checks SZ_64K alignment in
vgic_check_iorange(), is the !IS_ALIGNED(addr, SZ_64K) check here needed?

Also, redist_vcpu and v5its are members of the same anonymous union in
struct vgic_io_device, so setting iodev->redist_vcpu = NULL is immediately
overwritten by iodev->v5its = its.

> +	kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops);
> +
> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr,
> +				      KVM_VGIC_V5_ITS_SIZE, &iodev->dev);
> +out:
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
If kvm_io_bus_register_dev() fails in vgic_v5_register_its_iodev() (for
example with -ENOMEM or -ENOSPC), should its->vgic_v5_its_base be reset to
VGIC_ADDR_UNDEF before returning?

Leaving its->vgic_v5_its_base set to addr on failure causes
vgic_v5_its_get_attr() to report the address as configured even though the
MMIO device was never registered on KVM_MMIO_BUS, and causes any subsequent
retry of KVM_SET_DEVICE_ATTR via vgic_v5_its_set_attr() to fail with
-EEXIST in vgic_check_iorange().

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