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

On Fri, 2026-07-24 at 12:09 +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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).

Pasting directly from the A.a GICv5 specification Section 9.4.6, the
priority fields in any of the PPI priority registers are:

PRIORITY<x>, bits [60:56, 52:48, 44:40, 36:32, 28:24, 20:16, 12:8,
4:0], for x = 7 to 0

Unless I'm very much mistaken, that's 0x1f repeated for every byte.

> 
> 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?

No, I don't think that we should. For things like the arch timer,
that's using DVI anyhow, so we mask off those bits before writing the
state to the ICH_PPI_* registers.

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

Thanks,
Sascha