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

[Fuad Tabba]

Hi Marc,

On Fri, 25 Sep 2026 16:01:35 +0100, Marc Zyngier <maz@xxxxxxxxxx> wrote:
[...]
> 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.

Writing commit messages and comments is hard! :) I'm working on it...

>
> 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...

Much clearer, I'll use that for v2, and give the other messages in the
series the same treatment.

[...]
> 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.

The comment should say what happens without AArch32 EL1: RW stays set
and the entry fails with the IL exception, rather than EL2 switching
registers that are UNDEFINED there. I'll rewrite it for v2.

[...]

> Other than that, the patch looks great! :)

Sashiko didn't complain... :D

Cheers,
/fuad