Re: [PATCH v1 10/11] 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 1 potential issue(s) to consider:
- [High] Unconditional pinning of vgic_sre to SRE=1 breaks GICv2 emulation for non-protected guests.
--

commit 3d9e59afaa5e29a6ce57ece8ccacae434b38915d
Author: Marc Zyngier <maz@kernel.org>

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

This commit adds flush_hyp_vgic_state() and sync_hyp_vgic_state()
primitives to move per-vCPU VGIC state between the host and hypervisor
contexts for pKVM.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2f165b6c7b071..23e644c24a03a 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
[ ... ]
> @@ -99,6 +99,46 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
[ ... ]
> +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, max_lrs, i;
> +
> +	host_cpu_if	= &host_vcpu->arch.vgic_cpu.vgic_v3;
> +	hyp_cpu_if	= &hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3;
> +
> +	max_lrs		= (read_gicreg(ICH_VTR_EL2) & ICH_VTR_EL2_ListRegs) + 1;
> +	used_lrs	= host_cpu_if->used_lrs;
> +	used_lrs	= min(used_lrs, max_lrs);
> +
> +	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);

[Severity: High]
Does hardcoding ICC_SRE_EL1_SRE here break GICv2 emulation for non-protected
guests?

When __vgic_v3_restore_state() executes on guest entry, it will program
ICC_SRE_EL1 with this value. Setting the SRE bit to 1 disables the GICv2
memory-mapped interface (GICV) for EL1. Non-protected guests configured to
use GICv2 rely on vgic_sre == 0 to keep the memory-mapped interface enabled.

Should this value be copied from the host-set values for non-protected VMs
rather than being unconditionally pinned?

> +	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];
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612065925.755562-1-tabba@google.com?part=10