| Message ID | 20240226100601.2379693-8-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/arm64: Add NV support for ERET and PAuth | expand |
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 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! I need to find the time to get an NV setup set-up, so I can do some experiments myself. Thanks, Joey
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; } > 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.
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.
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);
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(-)