[PATCH 02/37] KVM: arm64: Rework hyp_panic for VHE and non-VHE

[cdall@linaro.org (Christoffer Dall)]

On Thu, Oct 12, 2017 at 04:55:16PM +0100, Marc Zyngier wrote:
> 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@linaro.org>
> > ---
> >  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...
> 

It doesn't really matter, especially as later patches will change this,
but this patch would be slightly nicer keeping the __deactivate_traps.
I don't mind changing that around in the next version.


> > +	__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.
> 

Thanks,
-Christoffer