Re: [PATCH v3 04/12] KVM: arm64: nVHE: Remove __guest_exit_panic path

From: Will Deacon <will@kernel.org>
To: "Pierre-Clément Tosi" <ptosi@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Vincent Donnefort <vdonnefort@google.com>
Subject: Re: [PATCH v3 04/12] KVM: arm64: nVHE: Remove __guest_exit_panic path
Date: Mon, 13 May 2024 15:27:22 +0100	[thread overview]
Message-ID: <20240513142721.GD28749@willie-the-truck> (raw)
In-Reply-To: <20240510112645.3625702-5-ptosi@google.com>

On Fri, May 10, 2024 at 12:26:33PM +0100, Pierre-Clément Tosi wrote:
> In invalid_host_el2_vect (i.e. EL2{t,h} handlers in nVHE guest context),
> remove the duplicate vCPU context check that __guest_exit_panic also
> performs, allowing an unconditional branch to it.
> 
> Rename __guest_exit_panic to __hyp_panic to better reflect that it might
> not exit through the guest but will always (directly or indirectly) end
> up executing hyp_panic(). Fix its wrong (probably bitrotten) ABI doc to
> reflect the ABI expected by VHE and (now) nVHE.
>
> Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().
> 
> Restore x0, x1 before calling hyp_panic when __hyp_panic is executed in
> host context (i.e. called from __kvm_hyp_vector).

Please don't mix cosmetic changes with functional changes. It really
makes the reviewer's life more difficult, especially when the diff is
nearly all in asm!

> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index bcaaf1a11b4e..6a1ce9d21e5b 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -83,7 +83,7 @@ alternative_else_nop_endif
>  	eret
>  	sb
>  
> -SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
> +SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL)
>  	// x0-x29,lr: hyp regs
>  
>  	stp	x0, x1, [sp, #-16]!
> @@ -92,13 +92,15 @@ SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
>  	msr	elr_el2, x0
>  	ldp	x0, x1, [sp], #16
>  
> -SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
> -	// x2-x29,lr: vcpu regs
> -	// vcpu x0-x1 on the stack
> +SYM_INNER_LABEL(__hyp_panic, SYM_L_GLOBAL)
> +	// x0-x29,lr: vcpu regs
> +
> +	stp	x0, x1, [sp, #-16]!
>  
>  	// If the hyp context is loaded, go straight to hyp_panic
>  	get_loaded_vcpu x0, x1
>  	cbnz	x0, 1f
> +	ldp	x0, x1, [sp], #16
>  	b	hyp_panic

Aren't these new stack accesses unnecessary for the case where the vector
is valid?

>  
>  1:
> @@ -110,10 +112,12 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
>  	// accurate if the guest had been completely restored.
>  	adr_this_cpu x0, kvm_hyp_ctxt, x1
>  	adr_l	x1, hyp_panic
> -	str	x1, [x0, #CPU_XREG_OFFSET(30)]
> +	str	x1, [x0, #CPU_LR_OFFSET]
>  
>  	get_vcpu_ptr	x1, x0
>  
> +	// Keep x0-x1 on the stack for __guest_exit

Didn't these get pushed twice? Once by the valid_vect macro and then
again by your stp in __hyp_panic?

I feel like I must be missing something here, but I don't really see why
this patch is needed.

Will

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