On Thu, 03 Jun 2021 09:39:09 +0100,
Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx> 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);
>
FWIW, I can boot a 5.13-rc4 kernel as a L1 guest without any visible
issue with this patch (and another guest as L2). I've folded this into
the series and pushed the result out (with a rebase on -rc4 for a good
measure).
Thanks again,
M.
--
Without deviation from the norm, progress is not possible.
