On 01/02/16 15:21, Christoffer Dall wrote: > On Mon, Jan 25, 2016 at 03:53:53PM +0000, Marc Zyngier wrote: >> The fault decoding process (including computing the IPA in the case >> of a permission fault) would be much better done in C code, as we >> have a reasonable infrastructure to deal with the VHE/non-VHE >> differences. >> >> Let's move the whole thing to C, including the workaround for >> erratum 834220, and just patch the odd ESR_EL2 access remaining >> in hyp-entry.S. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >> --- >> arch/arm64/kernel/asm-offsets.c | 3 -- >> arch/arm64/kvm/hyp/hyp-entry.S | 69 +++-------------------------------------- >> arch/arm64/kvm/hyp/switch.c | 54 ++++++++++++++++++++++++++++++++ >> 3 files changed, 59 insertions(+), 67 deletions(-) >> >> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c >> index fffa4ac6..b0ab4e9 100644 >> --- a/arch/arm64/kernel/asm-offsets.c >> +++ b/arch/arm64/kernel/asm-offsets.c >> @@ -110,9 +110,6 @@ int main(void) >> DEFINE(CPU_USER_PT_REGS, offsetof(struct kvm_regs, regs)); >> DEFINE(CPU_FP_REGS, offsetof(struct kvm_regs, fp_regs)); >> DEFINE(VCPU_FPEXC32_EL2, offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2])); >> - DEFINE(VCPU_ESR_EL2, offsetof(struct kvm_vcpu, arch.fault.esr_el2)); >> - DEFINE(VCPU_FAR_EL2, offsetof(struct kvm_vcpu, arch.fault.far_el2)); >> - DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2)); >> DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context)); >> #endif >> #ifdef CONFIG_CPU_PM >> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S >> index 9e0683f..213de52 100644 >> --- a/arch/arm64/kvm/hyp/hyp-entry.S >> +++ b/arch/arm64/kvm/hyp/hyp-entry.S >> @@ -19,7 +19,6 @@ >> >> #include <asm/alternative.h> >> #include <asm/assembler.h> >> -#include <asm/asm-offsets.h> >> #include <asm/cpufeature.h> >> #include <asm/kvm_arm.h> >> #include <asm/kvm_asm.h> >> @@ -67,7 +66,11 @@ ENDPROC(__vhe_hyp_call) >> el1_sync: // Guest trapped into EL2 >> save_x0_to_x3 >> >> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN >> mrs x1, esr_el2 >> +alternative_else >> + mrs x1, esr_el1 >> +alternative_endif > > I suppose this is not technically part of what the patch description > says it does, but ok... That's what the "... and just patch the odd ESR_EL2 access remaining in hyp-entry.S." meant. Would you prefer this as a separate patch? > >> lsr x2, x1, #ESR_ELx_EC_SHIFT >> >> cmp x2, #ESR_ELx_EC_HVC64 >> @@ -103,72 +106,10 @@ el1_trap: >> cmp x2, #ESR_ELx_EC_FP_ASIMD >> b.eq __fpsimd_guest_restore >> >> - cmp x2, #ESR_ELx_EC_DABT_LOW >> - mov x0, #ESR_ELx_EC_IABT_LOW >> - ccmp x2, x0, #4, ne >> - b.ne 1f // Not an abort we care about >> - >> - /* This is an abort. Check for permission fault */ >> -alternative_if_not ARM64_WORKAROUND_834220 >> - and x2, x1, #ESR_ELx_FSC_TYPE >> - cmp x2, #FSC_PERM >> - b.ne 1f // Not a permission fault >> -alternative_else >> - nop // Use the permission fault path to >> - nop // check for a valid S1 translation, >> - nop // regardless of the ESR value. >> -alternative_endif >> - >> - /* >> - * Check for Stage-1 page table walk, which is guaranteed >> - * to give a valid HPFAR_EL2. >> - */ >> - tbnz x1, #7, 1f // S1PTW is set >> - >> - /* Preserve PAR_EL1 */ >> - mrs x3, par_el1 >> - stp x3, xzr, [sp, #-16]! >> - >> - /* >> - * Permission fault, HPFAR_EL2 is invalid. >> - * Resolve the IPA the hard way using the guest VA. >> - * Stage-1 translation already validated the memory access rights. >> - * As such, we can use the EL1 translation regime, and don't have >> - * to distinguish between EL0 and EL1 access. >> - */ >> - mrs x2, far_el2 >> - at s1e1r, x2 >> - isb >> - >> - /* Read result */ >> - mrs x3, par_el1 >> - ldp x0, xzr, [sp], #16 // Restore PAR_EL1 from the stack >> - msr par_el1, x0 >> - tbnz x3, #0, 3f // Bail out if we failed the translation >> - ubfx x3, x3, #12, #36 // Extract IPA >> - lsl x3, x3, #4 // and present it like HPFAR >> - b 2f >> - >> -1: mrs x3, hpfar_el2 >> - mrs x2, far_el2 >> - >> -2: mrs x0, tpidr_el2 >> - str w1, [x0, #VCPU_ESR_EL2] >> - str x2, [x0, #VCPU_FAR_EL2] >> - str x3, [x0, #VCPU_HPFAR_EL2] >> - >> + mrs x0, tpidr_el2 >> mov x1, #ARM_EXCEPTION_TRAP >> b __guest_exit >> >> - /* >> - * Translation failed. Just return to the guest and >> - * let it fault again. Another CPU is probably playing >> - * behind our back. >> - */ >> -3: restore_x0_to_x3 >> - >> - eret >> - >> el1_irq: >> save_x0_to_x3 >> mrs x0, tpidr_el2 >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 0cadb7f..df2cce9 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -15,6 +15,7 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include <linux/types.h> >> #include <asm/kvm_asm.h> >> >> #include "hyp.h" >> @@ -150,6 +151,55 @@ static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu) >> __vgic_call_restore_state()(vcpu); >> } >> >> +static hyp_alternate_value(__check_arm_834220, >> + false, true, >> + ARM64_WORKAROUND_834220); >> + >> +static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu) >> +{ >> + u64 esr = read_sysreg_el2(esr); >> + u8 ec = esr >> ESR_ELx_EC_SHIFT; >> + u64 hpfar, far; >> + >> + vcpu->arch.fault.esr_el2 = esr; >> + >> + if (ec != ESR_ELx_EC_DABT_LOW && ec != ESR_ELx_EC_IABT_LOW) >> + return true; >> + >> + far = read_sysreg_el2(far); >> + >> + if (!(esr & ESR_ELx_S1PTW) && >> + (__check_arm_834220() || (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) { > > this is really hard to read. How do you feel about putting the below > block into its own function and changing to something like this: > > /* > * The HPFAR can be invalid if the stage 2 fault did not happen during a > * stage 1 page table walk (the ESR_EL2.S1PTW bit is clear) and one of > * the two following cases are true: > * 1. The fault was due to a permission fault > * 2. The processor carries errata 834220 > * > * Therefore, for all non S1PTW faults where we either have a permission > * fault or the errata workaround is enabled, we resolve the IPA using > * the AT instruction. > */ > if (!(esr & ESR_ELx_S1PTW) && > (__check_arm_834220() || (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) { > if (!__translate_far_to_ipa(&hpfar)) > return false; /* Translation failed, back to guest */ > } else { > hpfar = read_sysreg(hpfar_el2); > } > > not sure if it helps that much, perhaps it's just complicated by nature. > >> + u64 par, tmp; >> + >> + /* >> + * Permission fault, HPFAR_EL2 is invalid. Resolve the >> + * IPA the hard way using the guest VA. >> + * Stage-1 translation already validated the memory >> + * access rights. As such, we can use the EL1 >> + * translation regime, and don't have to distinguish >> + * between EL0 and EL1 access. >> + */ >> + par = read_sysreg(par_el1); > > in any cas I think we also need the comment about preserving par_el1 > here, which is only something we do because we may return early, IIUC. Not only. At that point, we still haven't saved the vcpu sysregs, so we most save/restore it in order to save it later for good. Not the fastest thing, but I guess that everything sucks so much when we take a page fault that it really doesn't matter. > >> + asm volatile("at s1e1r, %0" : : "r" (far)); >> + isb(); >> + >> + tmp = read_sysreg(par_el1); >> + write_sysreg(par, par_el1); >> + >> + if (unlikely(tmp & 1)) >> + return false; /* Translation failed, back to guest */ >> + > > nit: add comment /* Convert PAR to HPFAR format */ > >> + hpfar = ((tmp >> 12) & ((1UL << 36) - 1)) << 4; >> + } else { >> + hpfar = read_sysreg(hpfar_el2); >> + } >> + >> + vcpu->arch.fault.far_el2 = far; >> + vcpu->arch.fault.hpfar_el2 = hpfar; >> + return true; >> +} >> + >> static int __hyp_text __guest_run(struct kvm_vcpu *vcpu) >> { >> struct kvm_cpu_context *host_ctxt; >> @@ -181,9 +231,13 @@ static int __hyp_text __guest_run(struct kvm_vcpu *vcpu) >> __debug_restore_state(vcpu, kern_hyp_va(vcpu->arch.debug_ptr), guest_ctxt); >> >> /* Jump in the fire! */ >> +again: >> exit_code = __guest_enter(vcpu, host_ctxt); >> /* And we're baaack! */ >> >> + if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu)) >> + goto again; >> + >> fp_enabled = __fpsimd_enabled(); >> >> __sysreg_save_guest_state(guest_ctxt); >> -- >> 2.1.4 >> > The good news are that I couldn't find any bugs in the code. Right. So I've applied most of your comments directly, because they definitely made sense. let's see how it looks on round 3. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
