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

[Marc Zyngier]

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:
>>> The entire tail of fixup_guest_exit() is contained in if statements
>>> of the form if (x && *exit_code == ARM_EXCEPTION_TRAP).  As a result,
>>> we can check just once and bail out of the function early, allowing
>>> the remaining if conditions to be simplified.
>>>
>>> The only awkward case is where *exit_code is changed to
>>> ARM_EXCEPTION_EL1_SERROR in the case of an illegal GICv2 CPU
>>> interface access: in that case, the GICv3 trap handling code is
>>> skipped using a goto.  This avoids pointlessly evaluating the
>>> static branch check for the GICv3 case, even though we can't have
>>> vgic_v2_cpuif_trap and vgic_v3_cpuif_trap true simultaneously
>>> unless we have a GICv3 and GICv2 on the host: that sounds stupid,
>>> but I haven't satisfied myself that it can't happen.
>>
>> Indeed, this cannot happen, unless we decided to trap access to the
>> memory-mapped interface of a GICv3 implementation. We don't do that.
>>
>> But I guess the goto also serves a visual clue that the two cases are
>> mutually exclusives. Small nit below though:
>>
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Dave Martin <Dave.Martin at arm.com>
>>> ---
>>>  arch/arm64/kvm/hyp/switch.c | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> 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.

Thanks,

	N,
-- 
Jazz is not dead. It just smells funny...