| Message ID | 20240801091955.2066364-5-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for FP8 | expand |
On Thu, Aug 01, 2024 at 10:19:51AM +0100, Marc Zyngier wrote: > index 6af179c6356d..2466dd231362 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -198,6 +198,15 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) > } else { > __fpsimd_save_state(*host_data_ptr(fpsimd_state)); > } > + > + if (kvm_has_fpmr(vcpu->kvm)) { nVHE is faulting for me, apparently on the kvm_has_fpmr() check though I ran out of time to actually figure out where exactly it is going wrong. I'll have a further poke tomorrow. Backtrace below. > + u64 fpmr = read_sysreg_s(SYS_FPMR); > + > + if (unlikely(is_protected_kvm_enabled())) > + *host_data_ptr(fpmr) = fpmr; That looks wrong until you remember what host_data_ptr() does but but it's actually fine. host_data_ptr() is looking inside the struct kvm_host_data for the CPU rather than referencing the locally defined variable fpmr here. I do think it's worth avoiding the name collision though, perhaps just avoid the temporary variable? [ 1610.219274][ T247] kvm [247]: nVHE hyp panic at: [<ffffffc080ce3fc8>] __kvm_ nvhe_$x.262+0x24/0x38! [ 1610.219435][ T247] kvm [247]: nVHE call trace: [ 1610.219509][ T247] kvm [247]: [<ffffffc080ce3c2c>] __kvm_nvhe_hyp_panic+0xb 4/0xf8 [ 1610.219657][ T247] kvm [247]: [<ffffffc080ce3df4>] __kvm_nvhe_$x.238+0x14/0x60 [ 1610.219803][ T247] kvm [247]: [<ffffffc080ce33f8>] __kvm_nvhe_$x.88+0x24/0x23c [ 1610.219942][ T247] kvm [247]: [<ffffffc080ce64b4>] __kvm_nvhe_$x.26+0x8/0x2c [ 1610.220080][ T247] kvm [247]: [<ffffffc080ce617c>] __kvm_nvhe_$x.1+0x9c/0xa4 [ 1610.220216][ T247] kvm [247]: [<ffffffc080ce50fc>] __kvm_nvhe___skip_pauth_save+0x4/0x4 [ 1610.220356][ T247] kvm [247]: ---[ end nVHE call trace ]--- [ 1610.220435][ T247] kvm [247]: Hyp Offset: 0xffffff807fe00000 [ 1610.220566][ T247] Kernel panic - not syncing: HYP panic: [ 1610.220566][ T247] PS:1624023c9 PC:0000004000ee3fc8 ESR:0000000096000004 [ 1610.220566][ T247] FAR:ffffff880115cd1c HPFAR:0000000000000000 PAR:1d00007edbadc8de [ 1610.220566][ T247] VCPU:0000004801e88000 [ 1610.220743][ T247] CPU: 0 UID: 0 PID: 247 Comm: kvm-vcpu-0 Not tainted 6.11.0-rc1+ #247 [ 1610.220878][ T247] Hardware name: FVP Base RevC (DT) [ 1610.220958][ T247] Call trace: [ 1610.221023][ T247] dump_backtrace+0xfc/0x140 [ 1610.221132][ T247] show_stack+0x24/0x38 [ 1610.221239][ T247] dump_stack_lvl+0x3c/0x118 [ 1610.221358][ T247] dump_stack+0x18/0x40 [ 1610.221474][ T247] panic+0x134/0x368 [ 1610.221609][ T247] nvhe_hyp_panic_handler+0x114/0x1a0 [ 1610.221758][ T247] kvm_arm_vcpu_enter_exit+0x54/0xc0 [ 1610.221895][ T247] kvm_arch_vcpu_ioctl_run+0x494/0xa28
On Thu, 01 Aug 2024 20:07:16 +0100, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Thu, Aug 01, 2024 at 10:19:51AM +0100, Marc Zyngier wrote: > > > index 6af179c6356d..2466dd231362 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -198,6 +198,15 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) > > } else { > > __fpsimd_save_state(*host_data_ptr(fpsimd_state)); > > } > > + > > + if (kvm_has_fpmr(vcpu->kvm)) { > > nVHE is faulting for me, apparently on the kvm_has_fpmr() check though I > ran out of time to actually figure out where exactly it is going wrong. > I'll have a further poke tomorrow. Backtrace below. Well, that's actually pretty obvious when you see the crash (FAR:ffffff880115cd1c spills the beans). > > > + u64 fpmr = read_sysreg_s(SYS_FPMR); > > + > > + if (unlikely(is_protected_kvm_enabled())) > > + *host_data_ptr(fpmr) = fpmr; > > That looks wrong until you remember what host_data_ptr() does but but > it's actually fine. host_data_ptr() is looking inside the struct > kvm_host_data for the CPU rather than referencing the locally defined > variable fpmr here. I do think it's worth avoiding the name collision > though, perhaps just avoid the temporary variable? I'll rename the variable if that avoids people getting their brains in a twist. Full potential fix below. Thanks, M. diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 2466dd231362..c0832ca0285b 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -199,13 +199,13 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) __fpsimd_save_state(*host_data_ptr(fpsimd_state)); } - if (kvm_has_fpmr(vcpu->kvm)) { - u64 fpmr = read_sysreg_s(SYS_FPMR); + if (kvm_has_fpmr(kern_hyp_va(vcpu->kvm))) { + u64 val = read_sysreg_s(SYS_FPMR); if (unlikely(is_protected_kvm_enabled())) - *host_data_ptr(fpmr) = fpmr; + *host_data_ptr(fpmr) = val; else - **host_data_ptr(fpmr_ptr) = fpmr; + **host_data_ptr(fpmr_ptr) = val; } }
On Thu, Aug 01, 2024 at 09:09:40PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > nVHE is faulting for me, apparently on the kvm_has_fpmr() check though I > > ran out of time to actually figure out where exactly it is going wrong. > > I'll have a further poke tomorrow. Backtrace below. > Well, that's actually pretty obvious when you see the crash > (FAR:ffffff880115cd1c spills the beans). Ah, good tip. My problems are usually more to do with traps than pointer dereferences. > I'll rename the variable if that avoids people getting their brains in > a twist. Full potential fix below. That's working well for me here.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 021f7a1845f2..a6b684c08fe7 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -599,6 +599,16 @@ struct kvm_host_data { struct cpu_sve_state *sve_state; }; + union { + /* HYP VA pointer to the host storage for FPMR */ + u64 *fpmr_ptr; + /* + * Used by pKVM only, as it needs to provide storage + * for the host + */ + u64 fpmr; + }; + /* Ownership of the FP regs */ enum { FP_STATE_FREE, diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c index 4cb8ad5d69a8..ea5484ce1f3b 100644 --- a/arch/arm64/kvm/fpsimd.c +++ b/arch/arm64/kvm/fpsimd.c @@ -63,6 +63,7 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) */ *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; *host_data_ptr(fpsimd_state) = kern_hyp_va(¤t->thread.uw.fpsimd_state); + *host_data_ptr(fpmr_ptr) = kern_hyp_va(¤t->thread.uw.fpmr); vcpu_clear_flag(vcpu, HOST_SVE_ENABLED); if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN) diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index f59ccfe11ab9..84a135ba21a9 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -404,6 +404,9 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) else __fpsimd_restore_state(&vcpu->arch.ctxt.fp_regs); + if (kvm_has_fpmr(kern_hyp_va(vcpu->kvm))) + write_sysreg_s(__vcpu_sys_reg(vcpu, FPMR), SYS_FPMR); + /* Skip restoring fpexc32 for AArch64 guests */ if (!(read_sysreg(hcr_el2) & HCR_RW)) write_sysreg(__vcpu_sys_reg(vcpu, FPEXC32_EL2), fpexc32_el2); diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index f43d845f3c4e..87692b566d90 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -62,6 +62,8 @@ static void fpsimd_sve_flush(void) static void fpsimd_sve_sync(struct kvm_vcpu *vcpu) { + bool has_fpmr; + if (!guest_owns_fp_regs()) return; @@ -73,11 +75,18 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu) else __fpsimd_save_state(&vcpu->arch.ctxt.fp_regs); + has_fpmr = kvm_has_fpmr(kern_hyp_va(vcpu->kvm)); + if (has_fpmr) + __vcpu_sys_reg(vcpu, FPMR) = read_sysreg_s(SYS_FPMR); + if (system_supports_sve()) __hyp_sve_restore_host(); else __fpsimd_restore_state(*host_data_ptr(fpsimd_state)); + if (has_fpmr) + write_sysreg_s(*host_data_ptr(fpmr), SYS_FPMR); + *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; } diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 6af179c6356d..2466dd231362 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -198,6 +198,15 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) } else { __fpsimd_save_state(*host_data_ptr(fpsimd_state)); } + + if (kvm_has_fpmr(vcpu->kvm)) { + u64 fpmr = read_sysreg_s(SYS_FPMR); + + if (unlikely(is_protected_kvm_enabled())) + *host_data_ptr(fpmr) = fpmr; + else + **host_data_ptr(fpmr_ptr) = fpmr; + } } static const exit_handler_fn hyp_exit_handlers[] = { diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index 77010b76c150..80581b1c3995 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c @@ -312,6 +312,9 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code) static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) { __fpsimd_save_state(*host_data_ptr(fpsimd_state)); + + if (kvm_has_fpmr(vcpu->kvm)) + **host_data_ptr(fpmr_ptr) = read_sysreg_s(SYS_FPMR); } static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
Just like the rest of the FP/SIMD state, FPMR needs to be context switched. The only interesting thing here is that we need to treat the pKVM part a bit differently, as the host FP state is never written back to the vcpu thread, but instead stored locally and eagerly restored. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ arch/arm64/kvm/fpsimd.c | 1 + arch/arm64/kvm/hyp/include/hyp/switch.h | 3 +++ arch/arm64/kvm/hyp/nvhe/hyp-main.c | 9 +++++++++ arch/arm64/kvm/hyp/nvhe/switch.c | 9 +++++++++ arch/arm64/kvm/hyp/vhe/switch.c | 3 +++ 6 files changed, 35 insertions(+)