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@xxxxxxx>
>>> ---
>>> 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...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
