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