Re: [PATCH v1 2/3] KVM: arm64: Fix ID register initialization for non-protected pKVM guests

[Marc Zyngier <maz@kernel.org>]

On Thu, 12 Feb 2026 09:02:51 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> In protected mode, the hypervisor maintains a separate instance of
> the `kvm` structure for each VM. For non-protected VMs, this structure is
> initialized from the host's `kvm` state.
> 
> Currently, `pkvm_init_features_from_host()` copies the
> `KVM_ARCH_FLAG_ID_REGS_INITIALIZED` flag from the host without the
> underlying `id_regs` data being initialized. This results in the
> hypervisor seeing the flag as set while the ID registers remain zeroed.
> 
> Consequently, `kvm_has_feat()` checks at EL2 fail (return 0) for
> non-protected VMs. This breaks logic that relies on feature detection,
> such as `ctxt_has_tcrx()` for TCR2_EL1 support. As a result, certain
> system registers (e.g., TCR2_EL1, PIR_EL1, POR_EL1) are not
> saved/restored during the world switch, which could lead to state
> corruption.
> 
> Fix this by explicitly copying the ID registers from the host `kvm` to
> the hypervisor `kvm` for non-protected VMs during vCPU initialization,
> since we trust the host with its non-protected guests' features. Also
> ensure `KVM_ARCH_FLAG_ID_REGS_INITIALIZED` is cleared initially in
> `pkvm_init_features_from_host` so that `vm_copy_id_regs` can properly
> initialize them and set the flag once done.
> 
> Fixes: 41d6028e28bd ("KVM: arm64: Convert the SVE guest vcpu flag to a vm flag")
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/pkvm.c | 37 ++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 12b2acfbcfd1..267854ed29c8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -344,6 +344,8 @@ static void pkvm_init_features_from_host(struct pkvm_hyp_vm *hyp_vm, const struc
>  
>  	/* No restrictions for non-protected VMs. */
>  	if (!kvm_vm_is_protected(kvm)) {
> +		clear_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &host_arch_flags);
> +
>  		hyp_vm->kvm.arch.flags = host_arch_flags;

Can't you just have

  		hyp_vm->kvm.arch.flags &= ~BIT_ULL(KVM_ARCH_FLAG_ID_REGS_INITIALIZED);

since there are no atomicity requirements here?

>  
>  		bitmap_copy(kvm->arch.vcpu_features,
> @@ -471,6 +473,36 @@ static int pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *h
>  	return ret;
>  }
>  
> +static int vm_copy_id_regs(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct pkvm_hyp_vm *hyp_vm = pkvm_hyp_vcpu_to_hyp_vm(hyp_vcpu);
> +	const struct kvm *host_kvm = hyp_vm->host_kvm;
> +	struct kvm *kvm = &hyp_vm->kvm;
> +
> +	if (!test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &host_kvm->arch.flags))
> +		return -EINVAL;
> +
> +	if (test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags))
> +		return 0;
> +
> +	memcpy(kvm->arch.id_regs, host_kvm->arch.id_regs, sizeof(kvm->arch.id_regs));
> +	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);

This looks a bit odd. Can you have another vcpu doing this in
parallel? You seem to be holding vm_table_lock at this stage, so
that's probably OK,  but I'd have expected something like:

	if (test_and_set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags))
		return 0;

	memcpy(kvm->arch.id_regs, host_kvm->arch.id_regs, sizeof(kvm->arch.id_regs));

which makes the intent slightly clearer.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.