On 12/10/17 11:41, Christoffer Dall wrote:
> VHE actually doesn't rely on clearing the VTTBR when returning to the
> hsot kernel, and that is the current key mechanism of hyp_panic to
host
> figure out how to attempt to return to a state good enough to print a
> panic statement.
>
> Therefore, we split the hyp_panic function into two functions, a VHE and
> a non-VHE, keeping the non-VHE version intact, but changing the VHE
> behavior.
>
> The vttbr_el2 check on VHE doesn't really make that much sense, because
> the only situation where we can get here on VHE is when the hypervisor
> assembly code actually caleld into hyp_panic, which only happens when
called
> VBAR_EL2 has been set to the KVM exception vectors. On VHE, we can
> always safely disable the traps and restore the host registers at this
> point, so we simply do that unconditionally and call into the panic
> function directly.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> ---
> arch/arm64/kvm/hyp/switch.c | 45 +++++++++++++++++++++++----------------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index a0123ad..a50ddf3 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -394,10 +394,20 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
>
> static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par,
> - struct kvm_vcpu *vcpu)
> + struct kvm_cpu_context *__host_ctxt)
> {
> + struct kvm_vcpu *vcpu;
> unsigned long str_va;
>
> + vcpu = __host_ctxt->__hyp_running_vcpu;
> +
> + if (read_sysreg(vttbr_el2)) {
> + __timer_disable_traps(vcpu);
> + __deactivate_traps(vcpu);
> + __deactivate_vm(vcpu);
> + __sysreg_restore_host_state(__host_ctxt);
> + }
> +
> /*
> * Force the panic string to be loaded from the literal pool,
> * making sure it is a kernel address and not a PC-relative
> @@ -411,40 +421,31 @@ static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par,
> read_sysreg(hpfar_el2), par, vcpu);
> }
>
> -static void __hyp_text __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
> - struct kvm_vcpu *vcpu)
> +static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
> + struct kvm_cpu_context *host_ctxt)
> {
> + struct kvm_vcpu *vcpu;
> + vcpu = host_ctxt->__hyp_running_vcpu;
> +
> + __deactivate_traps_vhe();
Is there a reason why we can't just call __deactivate_traps(), rather
than the VHE-specific subset? It doesn't really matter, as we're about
to panic, but still...
> + __sysreg_restore_host_state(host_ctxt);
> +
> panic(__hyp_panic_string,
> spsr, elr,
> read_sysreg_el2(esr), read_sysreg_el2(far),
> read_sysreg(hpfar_el2), par, vcpu);
> }
>
> -static hyp_alternate_select(__hyp_call_panic,
> - __hyp_call_panic_nvhe, __hyp_call_panic_vhe,
> - ARM64_HAS_VIRT_HOST_EXTN);
> -
> void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *__host_ctxt)
> {
> - struct kvm_vcpu *vcpu = NULL;
> -
> u64 spsr = read_sysreg_el2(spsr);
> u64 elr = read_sysreg_el2(elr);
> u64 par = read_sysreg(par_el1);
>
> - if (read_sysreg(vttbr_el2)) {
> - struct kvm_cpu_context *host_ctxt;
> -
> - host_ctxt = __host_ctxt;
> - vcpu = host_ctxt->__hyp_running_vcpu;
> - __timer_disable_traps(vcpu);
> - __deactivate_traps(vcpu);
> - __deactivate_vm(vcpu);
> - __sysreg_restore_host_state(host_ctxt);
> - }
> -
> - /* Call panic for real */
> - __hyp_call_panic()(spsr, elr, par, vcpu);
> + if (!has_vhe())
> + __hyp_call_panic_nvhe(spsr, elr, par, __host_ctxt);
> + else
> + __hyp_call_panic_vhe(spsr, elr, par, __host_ctxt);
>
> unreachable();
> }
>
Otherwise looks good.
M.
--
Jazz is not dead. It just smells funny...
