Re: [PATCH v4 31/48] KVM: arm64: gic-v5: Add GICv5 EL1 sysreg userspace accessors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect priority mask `ICC_PPI_PRIORITYR_PRIORITY_MASK` corrupts GICv5 PPI priorities during save/restore.
- [High] Restoring the pending state of a level-triggered PPI overwrites the hardware wire state (`line_level`) instead of the software latch (`pending_latch`).
--

--- Patch [31]: [PATCH v4 31/48] KVM: arm64: gic-v5: Add GICv5 EL1 sysreg userspace accessors ---
commit bdb3308b5e4c3913da8b93ef491dc97004fe3ff0
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Add GICv5 EL1 sysreg userspace accessors

Now that KVM is at the point where it is able to run meaningful VMs
with GICv5, it is important to be able to save/restore the GICv5 state
in order to allow for VM migration.

> diff --git a/arch/arm64/kvm/vgic-sys-reg-v5.c b/arch/arm64/kvm/vgic-sys-reg-v5.c
> new file mode 100644
> index 0000000000000..6b255eacbf60d
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic-sys-reg-v5.c
> @@ -0,0 +1,519 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025, 2026 Arm Ltd.
> + */
> +
> +/*
> + * VGICv5 system registers handling functions for AArch64 mode
> + */
> +
> +#include <linux/irqchip/arm-gic-v5.h>
> +
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <linux/wordpart.h>
> +
> +#include <asm/kvm_emulate.h>
> +
> +#include "vgic/vgic.h"
> +#include "sys_regs.h"
> +
> +#define ICC_PPI_PRIORITYR_PRIORITY_MASK		REPEAT_BYTE(0x1f)

[Severity: High]
Does this mask correctly preserve the priority bits? GIC priority fields are
typically left-aligned in their 8-bit bytes (e.g., using bits [7:3], which
corresponds to a mask of 0xf8).

Applying REPEAT_BYTE(0x1f) later in set_gic_ppi_priorityr() appears to
isolate the bottom 5 bits (including the RAZ/WI bits [2:0]) and clear the
top valid priority bits, which could silently zero out the priorities when the
VMM restores the SYS_ICC_PPI_PRIORITYR<n>_EL1 registers during migration.

[ ... ]

> +static int set_gic_ppi_pendr(struct kvm_vcpu *vcpu,
> +			     const struct sys_reg_desc *r, u64 val)
> +{
> +	int i, start, end, reg = r->Op2 % 2;
> +
> +	/* If we only support architected PPIs, return */
> +	if (VGIC_V5_NR_PRIVATE_IRQS == 64 && reg == 1)
> +		return 0;
> +
> +	/*
> +	 * Update each struct vgic_irq with the pending state, treating Level
> +	 * and Edge interrupts differently. The exposed PPI mask is applied
> +	 * later, when vgic_v5_finalize_ppi_state() runs on first entry to each
> +	 * vCPU. See comment on set_gic_ppi_enabler() for details.
> +	 */
> +	start = reg * 64;
> +	end = min(start + 64, VGIC_V5_NR_PRIVATE_IRQS);
> +	for (i = start; i < end; i++) {
> +		u32 intid = vgic_v5_make_ppi(i);
> +		struct vgic_irq *irq;
> +
> +		irq = vgic_get_vcpu_irq(vcpu, intid);
> +
> +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
> +			bool level = !!(val & BIT_ULL(i - start));
> +
> +			if (irq->config == VGIC_CONFIG_LEVEL)
> +				irq->line_level = level;

[Severity: High]
Could this corrupt the interrupt state during VM migration by writing to
the hardware wire state rather than the software pending latch?

The SYS_ICC_PPI_SPENDR<n>_EL1 restore from userspace represents the software
pending state. If we set irq->line_level here for level-triggered interrupts,
it overwrites the ephemeral physical wire state (which gets re-evaluated by
backing hardware, like the arch timer) and the software-pended state would be
lost. Should this set irq->pending_latch for level interrupts as well?

> +			else
> +				irq->pending_latch = level;
> +		}
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}

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