Re: [PATCH v4 00/66] KVM: arm64: ARMv8.3/8.4 Nested Virtualization support

[Marc Zyngier <maz@kernel.org>]

Hi Jamie,

Funny, your email has a "Mail-Followup-To:" field that contains
everyone but you... Not ideal! ;-)

On Thu, 03 Jun 2021 08:07:22 +0100,
Jamie Iles <jamie@nuviainc.com> wrote:
> 
> Hi Marc,
> 
> On Mon, May 10, 2021 at 05:58:14PM +0100, Marc Zyngier wrote:
> > Here the bi-annual drop of the KVM/arm64 NV support code.
> > 
> > Not a lot has changed since [1], except for a discovery mechanism for
> > the EL2 support, some tidying up in the idreg emulation, dropping RMR
> > support, and a rebase on top of 5.13-rc1.
> > 
> > As usual, blame me for any bug, and nobody else.
> > 
> > It is still massively painful to run on the FVP, but if you have a
> > Neoverse V1 or N2 system that is collecting dust, I have the right
> > stuff to keep it busy!
> 
> I've been testing this series on FVP and get a crash when returning from 
> __kvm_vcpu_run_vhe because the autiasp is failing.

Ah, the joy of testing with older guests. I guess i should upgrade by
test rig and play with some newer guests at L1.

> 
> The problem is when the L1 boots and during EL2 setup sets hcr_el2 to 
> HCR_HOST_NVHE_FLAGS and so enables HCR_APK|HCR_API.  Then the guest 
> enter+exit logic in L0 starts performing the key save restore, but as we 
> didn't go through __hyp_handle_ptrauth, we haven't saved the host keys 
> and invoked vcpu_ptrauth_enable() so restore the host keys back to 0.
> 
> I wonder if the pointer auth keys should be saved+restored 
> unconditionally for a guest when running nested rather than the lazy 
> faulting that we have today?

I'd like to try and avoid that in order to keep the basic logic as
simple as possible for the time being, and as close to the tried and
trusted flow we have today.

> Alternatively we would need to duplicate
> the lazy logic for hcr_el2 writes.  A quick hack of saving the host keys 
> in __kvm_vcpu_run_vhe before sysreg_save_host_state_vhe is enough to 
> allow me to boot an L1 with --nested and then an L2.
>
> Do we also need to filter out HCR_APK|HCR_API for hcr_el2 writes when 
> pointer authentication hasn't been exposed to the guest?  I haven't yet 
> tried making ptrauth visible to the L1.

I think this is the real thing. We should never propagate trap bits
for features we don't want to support in guests. The L1 kernel sets
these bits unconditionally, despite PtrAuth never being advertised,
which trips the host code.

Could you try the untested hack below?

Thanks,

	M.

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index ce682bcce56f..54301d5ce58c 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -82,6 +82,7 @@
 			 HCR_BSU_IS | HCR_FB | HCR_TAC | \
 			 HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
 			 HCR_FMO | HCR_IMO | HCR_PTW )
+#define HCR_GUEST_NV_FILTER_FLAGS (HCR_ATA | HCR_API | HCR_APK | HCR_RW)
 #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
 #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
 #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 67f8b7d89db6..bf39bf4ef63c 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -64,6 +64,8 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 			 */
 			u64 vhcr_el2 = __vcpu_sys_reg(vcpu, HCR_EL2);
 
+			vhcr_el2 &= ~HCR_GUEST_NV_FILTER_FLAGS;
+
 			/*
 			 * We already set TVM to handle set/way cache maint
 			 * ops traps, this somewhat collides with the nested
@@ -91,7 +93,10 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 				       SYS_VNCR_EL2);
 		}
 	} else if (nested_virt_in_use(vcpu)) {
-		hcr |= __vcpu_sys_reg(vcpu, HCR_EL2);
+		u64 vhcr_el2 = __vcpu_sys_reg(vcpu, HCR_EL2);
+
+		vhcr_el2 &= ~HCR_GUEST_NV_FILTER_FLAGS;
+		hcr |= vhcr_el2;
 	}
 
 	___activate_traps(vcpu, hcr);

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