On Sat, Jul 18, 2020 at 10:00:30AM +0100, Marc Zyngier wrote:
> Hi Andrew,
>
> On Wed, 15 Jul 2020 19:44:08 +0100,
> Andrew Scull <ascull@xxxxxxxxxx> wrote:
> >
> > When exiting a guest, just check whether there is an SError pending and
> > set the bit in the exit code. The fixup then initiates the ceremony
> > should it be required.
> >
> > The separation allows for easier choices to be made as to whether the
> > demonic consultation should proceed.
>
> Such as?
It's used in the next patch to keep host SErrors pending and left for
the host to handle when reentering the host vcpu. IIUC, this matches the
previous behaviour since hyp would mask SErrors.
We wanted to avoid the need to convert host SErrors into virtual ones
and I opted for this approach to keep as much of the logic/policy as
possible in C.
Let me know if you'd prefer a different split of the patches or there
should be different design goals.
> >
> > Signed-off-by: Andrew Scull <ascull@xxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/kvm_hyp.h | 2 ++
> > arch/arm64/kvm/hyp/entry.S | 27 +++++++++++++++++--------
> > arch/arm64/kvm/hyp/hyp-entry.S | 1 -
> > arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++++
> > 4 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> > index 07745d9c49fc..50a774812761 100644
> > --- a/arch/arm64/include/asm/kvm_hyp.h
> > +++ b/arch/arm64/include/asm/kvm_hyp.h
> > @@ -91,6 +91,8 @@ void deactivate_traps_vhe_put(void);
> >
> > u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
> >
> > +void __vaxorcize_serror(void);
>
> I think a VAXorsist reference in the comments is plenty. The code can
> definitely stay architectural. Something like "__handle_SEI()" should
> be good. I'm not *that* fun.
Fine... ;)
> > +
> > void __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt);
> > #ifdef __KVM_NVHE_HYPERVISOR__
> > void __noreturn __hyp_do_panic(unsigned long, ...);
> > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> > index 6a641fcff4f7..dc4e3e7e7407 100644
> > --- a/arch/arm64/kvm/hyp/entry.S
> > +++ b/arch/arm64/kvm/hyp/entry.S
> > @@ -174,18 +174,31 @@ alternative_if ARM64_HAS_RAS_EXTN
> > mrs_s x2, SYS_DISR_EL1
> > str x2, [x1, #(VCPU_FAULT_DISR - VCPU_CONTEXT)]
> > cbz x2, 1f
> > - msr_s SYS_DISR_EL1, xzr
> > orr x0, x0, #(1<<ARM_EXIT_WITH_SERROR_BIT)
> > -1: ret
> > + nop
> > +1:
> > alternative_else
> > dsb sy // Synchronize against in-flight ld/st
> > isb // Prevent an early read of side-effect free ISR
> > mrs x2, isr_el1
> > - tbnz x2, #8, 2f // ISR_EL1.A
> > - ret
> > - nop
> > + tbz x2, #8, 2f // ISR_EL1.A
> > + orr x0, x0, #(1<<ARM_EXIT_WITH_SERROR_BIT)
> > 2:
> > alternative_endif
> > +
> > + ret
> > +SYM_FUNC_END(__guest_enter)
> > +
> > +/*
> > + * void __vaxorcize_serror(void);
> > + */
> > +SYM_FUNC_START(__vaxorcize_serror)
> > +
> > +alternative_if ARM64_HAS_RAS_EXTN
> > + msr_s SYS_DISR_EL1, xzr
> > + ret
> > +alternative_else_nop_endif
> > +
> > // We know we have a pending asynchronous abort, now is the
> > // time to flush it out. From your VAXorcist book, page 666:
> > // "Threaten me not, oh Evil one! For I speak with
> > @@ -193,7 +206,6 @@ alternative_endif
> > mrs x2, elr_el2
> > mrs x3, esr_el2
> > mrs x4, spsr_el2
> > - mov x5, x0
> >
> > msr daifclr, #4 // Unmask aborts
> >
> > @@ -217,6 +229,5 @@ abort_guest_exit_end:
> > msr elr_el2, x2
> > msr esr_el2, x3
> > msr spsr_el2, x4
> > - orr x0, x0, x5
> > 1: ret
> > -SYM_FUNC_END(__guest_enter)
> > +SYM_FUNC_END(__vaxorcize_serror)
> > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> > index e727bee8e110..c441aabb8ab0 100644
> > --- a/arch/arm64/kvm/hyp/hyp-entry.S
> > +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> > @@ -177,7 +177,6 @@ el2_error:
> > adr x1, abort_guest_exit_end
> > ccmp x0, x1, #4, ne
> > b.ne __hyp_panic
> > - mov x0, #(1 << ARM_EXIT_WITH_SERROR_BIT)
> > eret
> > sb
> >
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index 0511af14dc81..14a774d1a35a 100644
> > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > @@ -405,6 +405,10 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu)
> > */
> > static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> > {
> > + /* Flush guest SErrors. */
> > + if (ARM_SERROR_PENDING(*exit_code))
> > + __vaxorcize_serror();
> > +
> > if (ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ)
> > vcpu->arch.fault.esr_el2 = read_sysreg_el2(SYS_ESR);
> >
> > --
> > 2.27.0.389.gc38d7665816-goog
> >
> >
>
> I'm not against this patch, but I wonder whether it actually helps
> with anything. It spreads the handling across multiple paths, making
> it harder to read. Could you explain the rational beyond "it's
> easier"?
The earlier reply should cover most of this. I claim it's easier to make
choices as the predicate isn't stuck in assembly. Maybe others feel
differently and I should use less provocative language.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
