Re: [PATCH v3 7/8] KVM: arm64: Add primitives to flush/sync the VGIC state at EL2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Host updates to vgic_vmcr during MMIO emulation are silently discarded and reverted due to missing flush on vCPU entry and unconditional overwrite on vCPU exit.
- [High] flush_hyp_vgic_state() hardcodes vgic_sre to GICv3 values, permanently breaking GICv2 emulation by preventing ICH_VMCR_EL2 from being written.
--

commit 7488e8d4b40f79a0ec0bb726d02c6676c9426897
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Add primitives to flush/sync the VGIC state at EL2

This commit introduces flush_hyp_vgic_state() and sync_hyp_vgic_state() to
move per-vCPU VGIC state between host and hypervisor vCPU contexts for
protected VMs. It bounds used_lrs and hardcodes vgic_sre to a fixed value,
replacing the previous direct struct copy.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index f3233ee343a39..2db56146ec493 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -104,6 +104,45 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
>  	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
>  }
>  
> +static void flush_hyp_vgic_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +	struct vgic_v3_cpu_if *host_cpu_if, *hyp_cpu_if;
> +	unsigned int used_lrs, i;
> +
> +	host_cpu_if	= &host_vcpu->arch.vgic_cpu.vgic_v3;
> +	hyp_cpu_if	= &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +
> +	used_lrs	= host_cpu_if->used_lrs;
> +	used_lrs	= min(used_lrs, hyp_gicv3_nr_lr);
> +
> +	hyp_cpu_if->vgic_hcr	= host_cpu_if->vgic_hcr;

[Severity: High]
Does this code lose host-side updates to vgic_vmcr?

When a guest triggers a GICv2 MMIO trap, the host KVM run loop processes
it synchronously and updates host_cpu_if->vgic_vmcr. Because
flush_hyp_vgic_state() does not copy vgic_vmcr into the hypervisor state,
the hypervisor retains its stale value. 

When the guest exits, sync_hyp_vgic_state() copies the stale hypervisor
value back, which will permanently overwrite and clobber the host's update.
Could this lead to broken GICv2 CPU interface emulation and lost interrupts?

> +	/* Should be a one-off */
> +	hyp_cpu_if->vgic_sre	= (ICC_SRE_EL1_DIB |
> +				   ICC_SRE_EL1_DFB |
> +				   ICC_SRE_EL1_SRE);

[Severity: High]
Can hardcoding vgic_sre to GICv3 values here break GICv2 emulation?

For VMs using GICv2 emulation on GICv3 hardware, the host sets vgic_sre = 0
to enable compatibility mode. By unconditionally overriding it to GICv3
values here, the !cpu_if->vgic_sre check will evaluate to false in
__vgic_v3_activate_traps(), which prevents the hypervisor from writing to
ICH_VMCR_EL2. 

This appears to leave the hardware with a stale or zeroed VMCR and masks
guest interrupts. Is this intended?

> +	hyp_cpu_if->used_lrs	= used_lrs;
> +
> +	for (i = 0; i < used_lrs; i++)
> +		hyp_cpu_if->vgic_lr[i] = host_cpu_if->vgic_lr[i];
> +}
> +
> +static void sync_hyp_vgic_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +	struct vgic_v3_cpu_if *host_cpu_if, *hyp_cpu_if;
> +	unsigned int i;
> +
> +	host_cpu_if	= &host_vcpu->arch.vgic_cpu.vgic_v3;
> +	hyp_cpu_if	= &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +
> +	host_cpu_if->vgic_hcr = hyp_cpu_if->vgic_hcr;
> +	host_cpu_if->vgic_vmcr = hyp_cpu_if->vgic_vmcr;

[Severity: High]
As mentioned above, this unconditionally overwrites the host's vgic_vmcr
with the hypervisor's copy, which was never flushed from the host on entry.
Could this cause the host's MMIO emulation updates to be discarded?

> +
> +	for (i = 0; i < hyp_cpu_if->used_lrs; i++)
> +		host_cpu_if->vgic_lr[i] = hyp_cpu_if->vgic_lr[i];
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626070408.3420953-1-fuad.tabba@linux.dev?part=7