Re: [PATCH v1 2/4] KVM: arm64: Clear HCR_EL2.RW for 32-bit non-protected vCPUs

[Marc Zyngier <maz@kernel.org>]

On Fri, 25 Sep 2026 10:06:17 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> In pKVM, KVM_RUN on a vCPU created with KVM_ARM_VCPU_EL1_32BIT fails
> with KVM_EXIT_FAIL_ENTRY. pkvm_vcpu_reset_hcr(), pKVM's EL2 counterpart
> of vcpu_set_hcr(), never clears HCR_EL2.RW, so the first ERET into the
> vCPU is an illegal exception return. Protected VMs are AArch64-only, so
> only non-protected VMs are affected.
> 
> Clear RW for 32-bit vCPUs. The vCPU's features come from the host, and
> on a CPU without AArch32 EL1 a cleared RW would make EL2 switch
> registers that are UNDEFINED there. Clear it only when the system has
> AArch32 EL1, the same check system_supported_vcpu_features() makes on
> the host.

Apologies from repainting the proverbial bike shed, but I find the way
the above is written very hard to understand, because you are
describing minute aspects of the code without giving the big picture
upfront.

I'd rather see something like:

"pKVM maintains its own copy of a per-vcpu HCR_EL2. On initialisation
of the hypervisor's private vcpu structure, HCR_EL2.RW is set to 1
unconditionally.  However, nothing forbids userspace to create a
non-protected, AArch32 guest. Since HCR_EL2.RW==1, entering the guest
fails with an IL exception.

Make sure HCR_EL2.RW is cleared when the vcpu is AArch32 at EL1, and
that the HW actually supports this."

which describes the problem without the reader having to dig into
code. It makes matching the description and the code very easy. At
least for me, YMMV...

> 
> Fixes: b56680de9c648 ("KVM: arm64: Initialize trap register values in hyp in pKVM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  arch/arm64/kvm/hyp/nvhe/pkvm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 459bd9eb7e4bc..affc9595fda20 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -54,6 +54,14 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>  	else
>  		vcpu->arch.hcr_el2 |= HCR_TID2;
>  
> +	/*
> +	 * At EL2, vcpu_el1_is_32bit() reads HCR_EL2.RW, and EL2 switches the
> +	 * *32_EL2 registers when it returns true; they're UNDEFINED without AArch32 EL1.
> +	 */

That's another example: yes, this is all true. But how relevant is it?
Are you clearing RW just for the sake of vcpu_el1_is_32bit() to work
and prevent an UNDEF? No. It is so that 32bit guests do work. The
above really is only paraphrasing the ARM ARM.

What you don't describe is that if ARM64_HAS_32BIT_EL1 is not
implemented, you let the vcpu walk to the cliff with RW==1, and rely
on the CPU generating an IL exception. For me, this is the important
piece of information that needs to be captured.

> +	if (vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT) &&
> +	    cpus_have_final_cap(ARM64_HAS_32BIT_EL1))
> +		vcpu->arch.hcr_el2 &= ~HCR_RW;
> +
>  	if (vcpu_has_ptrauth(vcpu))
>  		vcpu->arch.hcr_el2 |= (HCR_API | HCR_APK);
>  

Other than that, the patch looks great! :)

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.