KVM: arm64: Fix HCR_EL2 for non-protected VMs in pKVM

A VMM can trigger the WARN_ON(!vcpu_is_el2()) in handle_tlbi_el1() on a CPU with FEAT_EVT2 and FEAT_TLBIOS but no FEAT_FGT, by setting ID_AA64ISAR0_EL1.TLB to NI and having the guest execute TLBI VMALLE1OS.

Without FGT, HCR_EL2.TTLBOS traps the instruction. KVM records no FGT trap information on such a host, so triage_sysreg_trap() doesn't UNDEF it. The instruction reaches handle_tlbi_el1(), which warns before checking whether the guest supports it.

Check support before the warning.

Fixes 0cb8aae226768 ("KVM: arm64: nv: Add handling of outer-shareable TLBI operations")

Cc [email protected]

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/kvm/sys_regs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/arm64/kvm/sys_regs.c +4 -3

index 44aae52c473d7..0ce29ce678b08 100644
@@ -4257,6 +4257,10 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u32 sys_encoding = sys_insn(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2);
 
+	/* Without FGT, HCR_EL2.TTLBOS also traps a hidden TLBI OS from vEL1 */
+	if (!kvm_supported_tlbi_s1e1_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
+
 	/*
 	 * If we're here, this is because we've trapped on a EL1 TLBI
 	 * instruction that affects the EL1 translation regime while

@@ -4279,9 +4283,6 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 	WARN_ON(!vcpu_is_el2(vcpu));
 
-	if (!kvm_supported_tlbi_s1e1_op(vcpu, sys_encoding))
-		return undef_access(vcpu, p, r);
-
 	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
 		kvm_handle_s1e2_tlbi(vcpu, sys_encoding, p->regval);
 		return true;

Signature · 1 line

2.39.5

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.

Fixes b56680de9c648 ("KVM: arm64: Initialize trap register values in hyp in pKVM")

Cc [email protected]

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/kvm/hyp/nvhe/pkvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm64/kvm/hyp/nvhe/pkvm.c +0 -0

index 459bd9eb7e4bc..affc9595fda20 100644

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

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.

Other than that, the patch looks great! :)

In pKVM, a non-protected VM gets only TWI, TWE and VSE from the HCR_EL2 the host computes for it. EL2 sets the rest in pkvm_vcpu_reset_hcr(), which covers only part of vcpu_set_hcr(). On a CPU with MTE the VM can then read GMID_EL1, on one without FGT it can execute a TLBI OS its ID registers hide, and it never gets the host's TVM, VI or VF.

Use the host's HCR_EL2 on every entry instead, except for the bits EL2 owns. The other bits only control what the VM's own execution traps on and which virtual exceptions are pending for it. The host computes them from the vCPU's features, ID registers and flags, which EL2 already takes from the host for a non-protected VM, as it takes MDCR_EL2, HCRX_EL2 and the fine-grained traps.

ATA, an owned bit, stays clear, as pKVM doesn't support MTE for any guest. TID2 and TID4 move to pvm_init_traps_hcr(), since EL2 now sets them only for a protected VM. A protected VM's HCR_EL2 is unchanged.

Fixes b56680de9c648 ("KVM: arm64: Initialize trap register values in hyp in pKVM")

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/include/asm/kvm_arm.h       |  1 +
 arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 13 +++++++++++++
 arch/arm64/kvm/hyp/nvhe/hyp-main.c     |  7 ++++---
 arch/arm64/kvm/hyp/nvhe/pkvm.c         | 18 ++++++++----------
 4 files changed, 26 insertions(+), 13 deletions(-)

arch/arm64/include/asm/kvm_arm.h +1 -0

index 4bfbd827c5aa7..8d187650e463e 100644
@@ -30,6 +30,7 @@
 #define HCR_AMVOFFEN	__HCR(AMVOFFEN)
 #define HCR_TICAB	__HCR(TICAB)
 #define HCR_TID4	__HCR(TID4)
+#define HCR_GPF		__HCR(GPF)
 #define HCR_FIEN	__HCR(FIEN)
 #define HCR_FWB		__HCR(FWB)
 #define HCR_NV2		__HCR(NV2)

arch/arm64/kvm/hyp/include/nvhe/pkvm.h +3 -0

index c904647d2f760..75b1122db4c91 100644
@@ -12,6 +12,19 @@
 #include <nvhe/gfp.h>
 #include <nvhe/spinlock.h>
 
+/*
+ * HCR_EL2 bits EL2 owns for a non-protected VM, whatever the host sets: those
+ * that restrict the guest, configure EL2 or what it switches (E2H, RW), or

arch/arm64/kvm/hyp/include/nvhe/pkvm.h +0 -0

 /*
  * Holds the relevant data for maintaining the vcpu state completely at hyp.
  */

arch/arm64/kvm/hyp/nvhe/hyp-main.c +1 -0

index 9a3b92e626adb..dec99d5bbee78 100644
@@ -216,6 +216,7 @@ static void sync_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
 static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 {
 	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
+	u64 host_hcr_mask = HCR_TWI | HCR_TWE | HCR_VSE;
 
 	fpsimd_sve_flush();
 	flush_debug_state(hyp_vcpu);

Quoted 3 lines

ATA, an owned bit, stays clear, as pKVM doesn't support MTE for any guest. TID2 and TID4 move to pvm_init_traps_hcr(), since EL2 now sets them only for a protected VM. A protected VM's HCR_EL2 is unchanged.

But the host does set these bits for non-protected VMs. What is going to honor these traps?

No mention of why you are adding HCR_EL2_GPF here?

arch/arm64/kvm/hyp/include/nvhe/pkvm.h line 27

Quoted 7 lines

arch/arm64/kvm/hyp/include/nvhe/pkvm.h +7 -0

+#define PKVM_HCR_EL2_OWNED        ((HCR_GUEST_FLAGS & ~(HCR_TWI | HCR_TWE)) | HCR_BSU |        \
+                                 HCR_E2H | HCR_TGE | HCR_TEA | HCR_GPF | HCR_TERR |        \
+                                 HCR_FWB | HCR_DC | HCR_ID | HCR_CD | HCR_NV |                \
+                                 HCR_NV1 | HCR_NV2 | HCR_API | HCR_APK | HCR_ATA |        \
+                                 HCR_DCT | HCR_FIEN | HCR_AMVOFFEN | HCR_ENSCXT |        \
+                                 HCR_EL2_RES0)
+

The name of the macro doesn't indicate that this only applies to protected VM.

Also, please don't add new uses of the compat HCR macros. I really want to remove them (probably post -rc1).

This feels fragile. You start with a restrictive set (TWI, TWE, VSE), and then drop it all. At this point, I have no idea what you are letting in.

It would be better to express things in a consistent way:

- either the bits that are controlled by the host for either protected and non-protected guests,

- or the bits that are controlled by the hypervisor for either cases.

Here, you're mixing both, and that's confusing.