[PATCH v5 13/14] KVM: arm64: Fold redundant exit code checks out of fixup_guest_exit()

[Dave Martin]

On Tue, May 08, 2018 at 12:59:39PM +0100, Marc Zyngier wrote:
> On 08/05/18 12:30, Dave Martin wrote:
> > On Tue, May 08, 2018 at 11:59:25AM +0100, Marc Zyngier wrote:
> >> On 04/05/18 17:05, Dave Martin wrote:

[...]

> >>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> >>> index 39e9166..be09c52 100644
> >>> --- a/arch/arm64/kvm/hyp/switch.c
> >>> +++ b/arch/arm64/kvm/hyp/switch.c
> >>> @@ -385,11 +385,13 @@ static bool __hyp_text fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> >>>  	 * same PC once the SError has been injected, and replay the
> >>>  	 * trapping instruction.
> >>>  	 */
> >>> -	if (*exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
> >>> +	if (*exit_code != ARM_EXCEPTION_TRAP)
> >>> +		goto exit;
> >>> +
> >>> +	if (!__populate_fault_info(vcpu))
> >>>  		return true;
> >>>  
> >>> -	if (static_branch_unlikely(&vgic_v2_cpuif_trap) &&
> >>> -	    *exit_code == ARM_EXCEPTION_TRAP) {
> >>> +	if (static_branch_unlikely(&vgic_v2_cpuif_trap)) {
> >>>  		bool valid;
> >>>  
> >>>  		valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
> >>> @@ -414,12 +416,12 @@ static bool __hyp_text fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> >>>  				if (!__skip_instr(vcpu))
> >>>  					*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
> >>>  				*exit_code = ARM_EXCEPTION_EL1_SERROR;
> >>> +				goto exit;
> >>
> >> This goto...
> >>
> >>>  			}
> >>
> >> ... should be placed here. If this was a data abort, it cannot be a
> >> system register trap, and the below conditions cannot possibly apply.
> > 
> > That sounds logically sensible, but to be clear, this would be a
> > semantic change to this function, right?
> > 
> > (i.e., it forces skipping of the GICv3 handling code in a case where
> > it previously wasn't forced -- at least not within this function.  The
> > arguments about whether vgic_v2_cpuif_trap and vgic_v3_cpuif_trap can
> > ever be true simultaneously still apply.)
> 
> I agree that this is a slight semantic change, but one that makes sense,
> just like the one you've introduced in patch #12.

Agreed, just wanted to make sure I hadn't missed something in my
original refactoring.

Cheers
---Dave