On Fri, Mar 01, 2024 at 07:14:00PM +0000, Marc Zyngier wrote:
> Hi Joey,
>
> On Fri, 01 Mar 2024 18:07:34 +0000,
> Joey Gouly <joey.gouly@xxxxxxx> wrote:
> >
> > Got a question about this one,
> >
> > On Mon, Feb 26, 2024 at 10:05:55AM +0000, Marc Zyngier wrote:
> > > If the L1 hypervisor decides to trap ERETs while running L2,
> > > make sure we don't try to emulate it, just like we wouldn't
> > > if it had its NV bit set.
> > >
> > > The exception will be reinjected from the core handler.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > > arch/arm64/kvm/hyp/vhe/switch.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> > > index eaf242b8e0cf..3ea9bdf6b555 100644
> > > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > > @@ -220,7 +220,8 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code)
> > > * Unless the trap has to be forwarded further down the line,
> > > * of course...
> > > */
> > > - if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_NV)
> > > + if ((__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_NV) ||
> > > + (__vcpu_sys_reg(vcpu, HFGITR_EL2) & HFGITR_EL2_ERET))
> > > return false;
> > >
> > > spsr = read_sysreg_el1(SYS_SPSR);
> >
> > Are we missing a forward_traps() call in kvm_emulated_nested_eret() for the
> > HFGITR case?
> >
> > Trying to follow the code path here, and I'm unsure of where else the
> > HFIGTR_EL2_ERET trap would be forwarded:
> >
> > kvm_arm_vcpu_enter_exit ->
> > ERET executes in guest
> > fixup_guest_exit ->
> > kvm_hyp_handle_eret (returns false)
> >
> > handle_exit ->
> > kvm_handle_eret ->
> > kvm_emulated_nested_eret
> > if HCR_NV
> > forward traps
> > else
> > emulate ERET
>
> There's a bit more happening in kvm_handle_eret().
>
> >
> >
> > And if the answer is that it is being reinjected somewhere, putting that
> > function name in the commit instead of 'core handler' would help with
> > understanding!
>
> Let's look at the code:
>
> static int kvm_handle_eret(struct kvm_vcpu *vcpu)
> {
> [...]
>
> if (is_hyp_ctxt(vcpu))
> kvm_emulate_nested_eret(vcpu);
>
> If we're doing an ERET from guest EL2, then we just emulate it,
> because there is nothing else to do. Crucially, HFGITR_EL2.ERET
> doesn't apply to EL2.
>
> else
> kvm_inject_nested_sync(vcpu, kvm_vcpu_get_esr(vcpu));
>
> In any other case, we simply reinject the trap into the guest EL2,
> because that's the only possible outcome. And that's what you were
> missing.
>
> return 1;
> }
>
Thanks, that makes sense now! I was forgetting about the crucial fact that
HFGITR_EL2.ERET applies to EL1, which is !is_hyp_ctxt(), so we take the other
branch.
With that cleared up:
Reviewed-by: Joey Gouly <joey.gouly@xxxxxxx>
Thanks,
Joey
>
> > I need to find the time to get an NV setup set-up, so I can do some experiments
> > myself.
>
> The FVP should be a good enough environment if you can bare the
> glacial speed. Other than that, I hear that QEMU has grown some NV
> support lately, but I haven't tried it yet. HW-wise, M2 is the only
> machine that can be bought by a human being (everything else is
> vapourware, or they would have already taken my money).
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
