Re: [PATCH v5 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:

New issues:
- [High] Missing its_vpe copy in flush_hyp_vgic_state breaks GICv4 virtual LPI injection

Pre-existing issues:
- [High] KVM_SET_ONE_REG updates to GICv3 VMCR are silently dropped under pKVM during vcpu_put()
--

--- Patch [7]: [PATCH v5 7/8] KVM: arm64: Add primitives to flush/sync the VGIC state at EL2 ---
commit 5a3c12abf480537e61537f1e45492ee3e349846c
Author: Marc Zyngier <maz@kernel.org>

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

pKVM performs its own world switch for protected VMs but has no primitives
to move the per-vCPU VGIC state between the host and hypervisor vCPU
contexts. Add flush_hyp_vgic_state() and sync_hyp_vgic_state().

> 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;
> +	/* Should be a one-off */
> +	hyp_cpu_if->vgic_sre	= (ICC_SRE_EL1_DIB |
> +				   ICC_SRE_EL1_DFB |
> +				   ICC_SRE_EL1_SRE);
> +	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];
> +}

[Severity: High]
Does this code miss copying the its_vpe struct?

Previously, this was a full struct assignment. By explicitly copying fields
instead, its_vpe is omitted, leaving its_vpe.its_vm as NULL in the
hypervisor vCPU state.

When the guest runs, __vgic_v3_activate_traps() executes at EL2 and relies
on this field:

arch/arm64/kvm/hyp/vgic-v3-sr.c:__vgic_v3_activate_traps() {
    if (static_branch_unlikely(&vgic_v3_cpuif_trap) ||
        cpu_if->its_vpe.its_vm || !cpu_if->vgic_sre)
        write_gicreg(vgic_ich_hcr_trap_bits() | ICH_HCR_EL2_En, ICH_HCR_EL2);
    ...
}

Since its_vm is NULL, ICH_HCR_EL2_En is not set. Could this break GICv4
direct virtual LPI injection for non-protected guests?

[ ... ]

> @@ -166,9 +199,6 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> -	struct vgic_v3_cpu_if *hyp_cpu_if = &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> -	struct vgic_v3_cpu_if *host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3;
> -	unsigned int i;
>  
>  	fpsimd_sve_sync(&hyp_vcpu->vcpu);
>  	sync_debug_state(hyp_vcpu);
> @@ -181,10 +211,7 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  
>  	host_vcpu->arch.iflags		= hyp_vcpu->vcpu.arch.iflags;
>  
> -	host_cpu_if->vgic_hcr		= hyp_cpu_if->vgic_hcr;
> -	host_cpu_if->vgic_vmcr		= hyp_cpu_if->vgic_vmcr;
> -	for (i = 0; i < hyp_cpu_if->used_lrs; ++i)
> -		host_cpu_if->vgic_lr[i] = hyp_cpu_if->vgic_lr[i];
> +	sync_hyp_vgic_state(hyp_vcpu);
>  }

[Severity: High]
This is a pre-existing issue, but does handle___vgic_v3_save_aprs()
silently drop userspace modifications to the GICv3 VMCR?

When userspace sets the VMCR register, vcpu_load() first calls
handle___vgic_v3_restore_vmcr_aprs(), which copies host_cpu_if->vgic_vmcr
to the hyp vCPU. KVM_SET_ONE_REG then updates the host copy of the register.

Finally, vcpu_put() calls handle___vgic_v3_save_aprs():

arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v3_save_aprs() {
    ...
    hyp_cpu_if = &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
    ...
    host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3;
    host_cpu_if->vgic_vmcr = hyp_cpu_if->vgic_vmcr;
}

Doesn't this unconditionally overwrite the new host value (set by
KVM_SET_ONE_REG) with the old value stored in the hyp vCPU at load time,
potentially breaking VM migration and state restoration?

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