Re: [PATCH] KVM: arm64: nv: Initialize hw_mmu to NULL for nested vCPUs

[Marc Zyngier]

Hi Dan,

On Wed, 11 Nov 2026 11:11:19 +0000,
Dan Carpenter <dan@xxxxxxxxxxx> wrote:
> 
> Before commit 6ded46b5a4fd ("KVM: arm64: nv: Keep reference on stage-2
> MMU when scheduled out"), kvm_vcpu_load_hw_mmu() would set ->hw_mmu on
> every load.  Now it only sets it when it is set to NULL.
> 
> In kvm_vcpu_init_nested() the vcpu->arch.hw_mmu points to &kvm->arch.mmu
> from when it was set in kvm_arch_vcpu_create().
> 
> Normally what happens next is a two step process.  KVM_ARM_VCPU_INIT
> calls kvm_reset_vcpu_core() and that sets the vcpu into hyp context
> (vEL2).  Then KVM_RUN calls kvm_vcpu_load_hw_mmu() which doesn't do
> anything.  But that is fine because we are in hyp context and the
> ->hw_mmu already is pointing to the correct place.
> 
> However if, for example, we are restoring a vcpu that was saved in L2
> context, then the VMM uses KVM_SET_ONE_REG to overwrite the vEL2 state
> that kvm_reset_vcpu_core() set up, and the vCPU is in L2 context when
> KVM_RUN loads it for the first time.  Ideally, kvm_vcpu_load_hw_mmu()
> would set the hw_mm to point to L2's page tables but, since is non-NULL,
> it leaves it as is.  At this point L2 can access L1's memory.

Ah, nice one. You can tell how much testing has gone into
save/restore...

>
> Fix this bug but setting ->hw_mmu to NULL in kvm_vcpu_init_nested() so

nit: s/but/by/

> that kvm_vcpu_load_hw_mmu() always sets it correctly on the first load.
> 
> Fixes: 6ded46b5a4fd ("KVM: arm64: nv: Keep reference on stage-2 MMU when scheduled out")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Dan Carpenter <dan@xxxxxxxxxxx>
> ---
>  arch/arm64/kvm/nested.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 78fa433774c6..60bccb66e52c 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -140,6 +140,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;
>  	}
>  
> +	vcpu->arch.hw_mmu = NULL;
> +

I think this really needs a comment to explain why we repaint hw_mmu
to NULL, because the next idiot who looks at this (likely me) will
probably remove this line...

The other thing is that this means that nothing can evaluate
vcpu->arch.hw_mmu anywhere outside of the loaded path, as it can be
NULL. I *think* this is OK, but I need to really convince myself...

I'd like to add something like:

	/*
	 * We can't rely on the S2 MMU being the canonical one,
	 * as userspace can restore a nested context. Nuke the
	 * hw_mmu early to ensure that we pick the correct one
	 * load time. It also means that we can't rely on hw_mmu
	 * being set to anything useful if the vcpu isn't loaded.
	 */

If that works for you, I'll squash that in.

Thanks,

	M.

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