Hi Andrew, On Wed, 15 Jul 2020 19:44:07 +0100, Andrew Scull <[email protected]> wrote: > > Allow entry to a vcpu that can handle interrupts if there is an > interrupts pending. Entry will still be aborted if the vcpu cannot > handle interrupts.
This is pretty confusing. All vcpus can handle interrupts, it's just that there are multiple classes of interrupts (physical or virtual). Instead, this should outline *where* physical interrupt are taken. Something like: When entering a vcpu for which physical interrupts are not taken to EL2, don't bother evaluating ISR_EL1 to work out whether we should go back to EL2 early. Instead, just enter the guest without any further ado. This is done by checking HCR_EL2.IMO bit. > > This allows use of __guest_enter to enter into the host. > > Signed-off-by: Andrew Scull <[email protected]> > --- > arch/arm64/kvm/hyp/entry.S | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S > index ee32a7743389..6a641fcff4f7 100644 > --- a/arch/arm64/kvm/hyp/entry.S > +++ b/arch/arm64/kvm/hyp/entry.S > @@ -73,13 +73,17 @@ SYM_FUNC_START(__guest_enter) > save_sp_el0 x1, x2 > > // Now the host state is stored if we have a pending RAS SError it must > - // affect the host. If any asynchronous exception is pending we defer > - // the guest entry. The DSB isn't necessary before v8.2 as any SError > - // would be fatal. > + // affect the host. If physical IRQ interrupts are going to be trapped > + // and there are already asynchronous exceptions pending then we defer > + // the entry. The DSB isn't necessary before v8.2 as any SError would > + // be fatal. > alternative_if ARM64_HAS_RAS_EXTN > dsb nshst > isb > alternative_else_nop_endif > + mrs x1, hcr_el2 > + and x1, x1, #HCR_IMO > + cbz x1, 1f Do we really want to take the overhead of the above DSB/ISB when on the host? We're not even evaluating ISR_EL1, so what is the gain? This also assumes that IMO/FMO/AMO are all set together, which deserves to be documented. Another thing is that you are also restoring registers that the host vcpu expects to be corrupted (the caller-saved registers, X0-17). You probably should just zero them instead if leaking data from EL2 is your concern. Yes, this is a departure from SMCCC 1.1, but I think this is a valid one, as EL2 isn't a fully independent piece of SW. Same goes on the __guest_exit() path. PtrAuth is another concern (I'm pretty sure this doesn't do what we want, but I haven't tried on a model). I've hacked the following patch together, which allowed me to claw back about 10% of the performance loss. I'm pretty sure there are similar places where you have introduced extra overhead, and we should hunt them down. Thanks, M. diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S index 6c3a6b27a96c..2d1a71bd7baa 100644 --- a/arch/arm64/kvm/hyp/entry.S +++ b/arch/arm64/kvm/hyp/entry.S @@ -33,6 +33,10 @@ SYM_FUNC_START(__guest_enter) // Save the hyp's sp_el0 save_sp_el0 x1, x2 + mrs x1, hcr_el2 + and x1, x1, #HCR_IMO + cbz x1, 2f + // Now the hyp state is stored if we have a pending RAS SError it must // affect the hyp. If physical IRQ interrupts are going to be trapped // and there are already asynchronous exceptions pending then we defer @@ -42,9 +46,6 @@ alternative_if ARM64_HAS_RAS_EXTN dsb nshst isb alternative_else_nop_endif - mrs x1, hcr_el2 - and x1, x1, #HCR_IMO - cbz x1, 1f mrs x1, isr_el1 cbz x1, 1f mov x0, #ARM_EXCEPTION_IRQ @@ -81,6 +82,31 @@ alternative_else_nop_endif eret sb +2: + add x29, x0, #VCPU_CONTEXT + + // Macro ptrauth_switch_to_guest format: + // ptrauth_switch_to_guest(guest cxt, tmp1, tmp2, tmp3) + // The below macro to restore guest keys is not implemented in C code + // as it may cause Pointer Authentication key signing mismatch errors + // when this feature is enabled for kernel code. + ptrauth_switch_to_guest x29, x0, x1, x2 + + // Restore the guest's sp_el0 + restore_sp_el0 x29, x0 + + .irp n,4,5,6,7,8,9,10,11,12,13,14,15,16,17 + mov x\n, xzr + .endr + + ldp x0, x1, [x29, #CPU_XREG_OFFSET(0)] + ldp x2, x3, [x29, #CPU_XREG_OFFSET(2)] + + // Restore guest regs x18-x29, lr + restore_callee_saved_regs x29 + eret + sb + SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL) // x0: return code // x1: vcpu @@ -99,6 +125,11 @@ SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL) // Store the guest regs x0-x1 and x4-x17 stp x2, x3, [x1, #CPU_XREG_OFFSET(0)] + + mrs x2, hcr_el2 + and x2, x2, #HCR_IMO + cbz x2, 1f + stp x4, x5, [x1, #CPU_XREG_OFFSET(4)] stp x6, x7, [x1, #CPU_XREG_OFFSET(6)] stp x8, x9, [x1, #CPU_XREG_OFFSET(8)] @@ -107,6 +138,7 @@ SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL) stp x14, x15, [x1, #CPU_XREG_OFFSET(14)] stp x16, x17, [x1, #CPU_XREG_OFFSET(16)] +1: // Store the guest regs x18-x29, lr save_callee_saved_regs x1 -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
