Re: [PATCH v2 07/13] KVM: arm64: nv: Honor HFGITR_EL2.ERET being set

[Joey Gouly <joey.gouly@arm.com>]

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@arm.com> 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@kernel.org>
> > > ---
> > >  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@arm.com>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel