* [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-23 11:56 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-23 11:56 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, Will Deacon, kvmarm HCR_EL2.TID3 mandates that access from EL1 to a long list of id registers traps to EL2, and QEMU has so far ignored this requirement. This breaks (among other things) KVM guests that have PtrAuth enabled, while the hypervisor doesn't want to expose the feature to its guest. To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this case), and masks out the unsupported feature. QEMU not honoring the trap request means that the guest observes that the feature is present in the HW, starts using it, and dies a horrible death when KVM injects an UNDEF, because the feature *really* isn't supported. Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. Reported-by: Will Deacon <will@kernel.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- There is a number of other trap bits missing (TID[0-2], for example), but this at least gets a mainline Linux going with cpu=max. target/arm/helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index 027fffbff6..976e96f62b 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5978,6 +5978,26 @@ static const ARMCPRegInfo predinv_reginfo[] = { REGINFO_SENTINEL }; +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { + return CP_ACCESS_TRAP_EL2; + } + + return CP_ACCESS_OK; +} + +static CPAccessResult access_aa32idreg(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ + if (arm_feature(env, ARM_FEATURE_V8)) { + return access_aa64idreg(env, ri, isread); + } + + return CP_ACCESS_OK; +} + void register_cp_regs_for_features(ARMCPU *cpu) { /* Register all the coprocessor registers based on feature bits */ @@ -6001,6 +6021,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_PFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_pfr0 }, /* ID_PFR1 is not a plain ARM_CP_CONST because we don't know * the value of the GIC field until after we define these regs. @@ -6008,63 +6029,78 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_PFR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1, .access = PL1_R, .type = ARM_CP_NO_RAW, + .accessfn = access_aa32idreg, .readfn = id_pfr1_read, .writefn = arm_cp_write_ignore }, { .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_dfr0 }, { .name = "ID_AFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_afr0 }, { .name = "ID_MMFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr0 }, { .name = "ID_MMFR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr1 }, { .name = "ID_MMFR2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr2 }, { .name = "ID_MMFR3", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr3 }, { .name = "ID_ISAR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar0 }, { .name = "ID_ISAR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar1 }, { .name = "ID_ISAR2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar2 }, { .name = "ID_ISAR3", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar3 }, { .name = "ID_ISAR4", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar4 }, { .name = "ID_ISAR5", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar5 }, { .name = "ID_MMFR4", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr4 }, { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar6 }, REGINFO_SENTINEL }; @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, .access = PL1_R, .type = ARM_CP_NO_RAW, + .accessfn = access_aa64idreg, .readfn = id_aa64pfr0_read, .writefn = arm_cp_write_ignore }, { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64pfr1}, { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, /* At present, only SVEver == 0 is defined anyway. */ .resetvalue = 0 }, { .name = "ID_AA64PFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64PFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64PFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64dfr0 }, { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64dfr1 }, { .name = "ID_AA64DFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64DFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64afr0 }, { .name = "ID_AA64AFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64afr1 }, { .name = "ID_AA64AFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64AFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64isar0 }, { .name = "ID_AA64ISAR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64isar1 }, { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64mmfr0 }, { .name = "ID_AA64MMFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64mmfr1 }, { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr0 }, { .name = "MVFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr1 }, { .name = "MVFR2_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr2 }, { .name = "MVFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "PMCEID0", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 6, -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-23 11:56 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-23 11:56 UTC (permalink / raw) To: qemu-devel; +Cc: Will Deacon, kvmarm HCR_EL2.TID3 mandates that access from EL1 to a long list of id registers traps to EL2, and QEMU has so far ignored this requirement. This breaks (among other things) KVM guests that have PtrAuth enabled, while the hypervisor doesn't want to expose the feature to its guest. To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this case), and masks out the unsupported feature. QEMU not honoring the trap request means that the guest observes that the feature is present in the HW, starts using it, and dies a horrible death when KVM injects an UNDEF, because the feature *really* isn't supported. Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. Reported-by: Will Deacon <will@kernel.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- There is a number of other trap bits missing (TID[0-2], for example), but this at least gets a mainline Linux going with cpu=max. target/arm/helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index 027fffbff6..976e96f62b 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5978,6 +5978,26 @@ static const ARMCPRegInfo predinv_reginfo[] = { REGINFO_SENTINEL }; +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { + return CP_ACCESS_TRAP_EL2; + } + + return CP_ACCESS_OK; +} + +static CPAccessResult access_aa32idreg(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ + if (arm_feature(env, ARM_FEATURE_V8)) { + return access_aa64idreg(env, ri, isread); + } + + return CP_ACCESS_OK; +} + void register_cp_regs_for_features(ARMCPU *cpu) { /* Register all the coprocessor registers based on feature bits */ @@ -6001,6 +6021,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_PFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_pfr0 }, /* ID_PFR1 is not a plain ARM_CP_CONST because we don't know * the value of the GIC field until after we define these regs. @@ -6008,63 +6029,78 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_PFR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1, .access = PL1_R, .type = ARM_CP_NO_RAW, + .accessfn = access_aa32idreg, .readfn = id_pfr1_read, .writefn = arm_cp_write_ignore }, { .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_dfr0 }, { .name = "ID_AFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_afr0 }, { .name = "ID_MMFR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr0 }, { .name = "ID_MMFR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr1 }, { .name = "ID_MMFR2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr2 }, { .name = "ID_MMFR3", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr3 }, { .name = "ID_ISAR0", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar0 }, { .name = "ID_ISAR1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar1 }, { .name = "ID_ISAR2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar2 }, { .name = "ID_ISAR3", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar3 }, { .name = "ID_ISAR4", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar4 }, { .name = "ID_ISAR5", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar5 }, { .name = "ID_MMFR4", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->id_mmfr4 }, { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa32idreg, .resetvalue = cpu->isar.id_isar6 }, REGINFO_SENTINEL }; @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, .access = PL1_R, .type = ARM_CP_NO_RAW, + .accessfn = access_aa64idreg, .readfn = id_aa64pfr0_read, .writefn = arm_cp_write_ignore }, { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64pfr1}, { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, /* At present, only SVEver == 0 is defined anyway. */ .resetvalue = 0 }, { .name = "ID_AA64PFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64PFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64PFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64dfr0 }, { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64dfr1 }, { .name = "ID_AA64DFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64DFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64afr0 }, { .name = "ID_AA64AFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->id_aa64afr1 }, { .name = "ID_AA64AFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64AFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64isar0 }, { .name = "ID_AA64ISAR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64isar1 }, { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64ISAR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64mmfr0 }, { .name = "ID_AA64MMFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.id_aa64mmfr1 }, { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "ID_AA64MMFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr0 }, { .name = "MVFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr1 }, { .name = "MVFR2_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = cpu->isar.mvfr2 }, { .name = "MVFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 4, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 5, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "MVFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, + .accessfn = access_aa64idreg, .resetvalue = 0 }, { .name = "PMCEID0", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 6, -- 2.20.1 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-23 11:56 ` Marc Zyngier @ 2019-11-25 10:40 ` Will Deacon -1 siblings, 0 replies; 24+ messages in thread From: Will Deacon @ 2019-11-25 10:40 UTC (permalink / raw) To: Marc Zyngier; +Cc: Peter Maydell, qemu-devel, kvmarm On Sat, Nov 23, 2019 at 11:56:18AM +0000, Marc Zyngier wrote: > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. > > target/arm/helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 75 insertions(+) I took your fixes/el2_traps branch for a spin and I no longer get an unexpected undefined instruction trap on first access to the ptrauth key registers during context-switch: Tested-by: Will Deacon <will@kernel.org> Thanks, Will ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 10:40 ` Will Deacon 0 siblings, 0 replies; 24+ messages in thread From: Will Deacon @ 2019-11-25 10:40 UTC (permalink / raw) To: Marc Zyngier; +Cc: qemu-devel, kvmarm On Sat, Nov 23, 2019 at 11:56:18AM +0000, Marc Zyngier wrote: > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. > > target/arm/helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 75 insertions(+) I took your fixes/el2_traps branch for a spin and I no longer get an unexpected undefined instruction trap on first access to the ptrauth key registers during context-switch: Tested-by: Will Deacon <will@kernel.org> Thanks, Will _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-25 10:40 ` Will Deacon @ 2019-11-25 10:59 ` Marc Zyngier -1 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 10:59 UTC (permalink / raw) To: Will Deacon; +Cc: Peter Maydell, qemu-devel, kvmarm On 2019-11-25 10:40, Will Deacon wrote: > On Sat, Nov 23, 2019 at 11:56:18AM +0000, Marc Zyngier wrote: >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. >> >> target/arm/helper.c | 75 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 75 insertions(+) > > I took your fixes/el2_traps branch for a spin and I no longer get an > unexpected undefined instruction trap on first access to the ptrauth > key > registers during context-switch: > > Tested-by: Will Deacon <will@kernel.org> Thanks for that. I'll post the whole series later today, though the other bits are less critical. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 10:59 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 10:59 UTC (permalink / raw) To: Will Deacon; +Cc: qemu-devel, kvmarm On 2019-11-25 10:40, Will Deacon wrote: > On Sat, Nov 23, 2019 at 11:56:18AM +0000, Marc Zyngier wrote: >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. >> >> target/arm/helper.c | 75 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 75 insertions(+) > > I took your fixes/el2_traps branch for a spin and I no longer get an > unexpected undefined instruction trap on first access to the ptrauth > key > registers during context-switch: > > Tested-by: Will Deacon <will@kernel.org> Thanks for that. I'll post the whole series later today, though the other bits are less critical. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-23 11:56 ` Marc Zyngier @ 2019-11-25 16:21 ` Peter Maydell -1 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-25 16:21 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote: > > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. I guess this ought to go into 4.2, given that it gets recent Linux guest kernels to work. > @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, > .access = PL1_R, .type = ARM_CP_NO_RAW, > + .accessfn = access_aa64idreg, > .readfn = id_aa64pfr0_read, > .writefn = arm_cp_write_ignore }, > { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = cpu->isar.id_aa64pfr1}, > { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, > @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = 0 }, Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? > { .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > /* At present, only SVEver == 0 is defined anyway. */ > .resetvalue = 0 }, > { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = cpu->isar.mvfr0 }, These are the AArch64 accessors for the aarch32 MVFR registers, but this trap bit is also supposed to trap aarch32 accesses to the MVFR registers, which are via the vmrs insn. That needs to be done in trans_VMSR_VMRS(); we can do that with a followup patch, since the mechanism there is completely different. thanks -- PMM ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 16:21 ` Peter Maydell 0 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-25 16:21 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote: > > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. I guess this ought to go into 4.2, given that it gets recent Linux guest kernels to work. > @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, > .access = PL1_R, .type = ARM_CP_NO_RAW, > + .accessfn = access_aa64idreg, > .readfn = id_aa64pfr0_read, > .writefn = arm_cp_write_ignore }, > { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = cpu->isar.id_aa64pfr1}, > { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, > @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = 0 }, Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? > { .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > /* At present, only SVEver == 0 is defined anyway. */ > .resetvalue = 0 }, > { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, > .access = PL1_R, .type = ARM_CP_CONST, > + .accessfn = access_aa64idreg, > .resetvalue = cpu->isar.mvfr0 }, These are the AArch64 accessors for the aarch32 MVFR registers, but this trap bit is also supposed to trap aarch32 accesses to the MVFR registers, which are via the vmrs insn. That needs to be done in trans_VMSR_VMRS(); we can do that with a followup patch, since the mechanism there is completely different. thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-25 16:21 ` Peter Maydell @ 2019-11-25 17:08 ` Marc Zyngier -1 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 17:08 UTC (permalink / raw) To: Peter Maydell; +Cc: Will Deacon, QEMU Developers, kvmarm On 2019-11-25 16:21, Peter Maydell wrote: > On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote: >> >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. > > I guess this ought to go into 4.2, given that it gets recent > Linux guest kernels to work. > > >> @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU >> *cpu) >> { .name = "ID_AA64PFR0_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, >> .access = PL1_R, .type = ARM_CP_NO_RAW, >> + .accessfn = access_aa64idreg, >> .readfn = id_aa64pfr0_read, >> .writefn = arm_cp_write_ignore }, >> { .name = "ID_AA64PFR1_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = cpu->isar.id_aa64pfr1}, >> { .name = "ID_AA64PFR2_EL1_RESERVED", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, >> @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU >> *cpu) >> { .name = "ID_AA64PFR3_EL1_RESERVED", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = 0 }, > > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? Indeed, I oversaw that one. I'll fix it and repost it together with the extra patches to handle TID1 and TID2. > >> { .name = "ID_AA64ZFR0_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> /* At present, only SVEver == 0 is defined anyway. >> */ >> .resetvalue = 0 }, > >> { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = cpu->isar.mvfr0 }, > > These are the AArch64 accessors for the aarch32 MVFR registers, > but this trap bit is also supposed to trap aarch32 accesses to > the MVFR registers, which are via the vmrs insn. That needs > to be done in trans_VMSR_VMRS(); we can do that with a > followup patch, since the mechanism there is completely different. Holy cow! I'm afraid that my newly acquired QEMU-foo is missing a few key options. Does it mean we need to generate a trapping instruction, as opposed to take the exception on access? I'd very much appreciate some guidance here. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 17:08 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 17:08 UTC (permalink / raw) To: Peter Maydell; +Cc: Will Deacon, QEMU Developers, kvmarm On 2019-11-25 16:21, Peter Maydell wrote: > On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote: >> >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. > > I guess this ought to go into 4.2, given that it gets recent > Linux guest kernels to work. > > >> @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU >> *cpu) >> { .name = "ID_AA64PFR0_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, >> .access = PL1_R, .type = ARM_CP_NO_RAW, >> + .accessfn = access_aa64idreg, >> .readfn = id_aa64pfr0_read, >> .writefn = arm_cp_write_ignore }, >> { .name = "ID_AA64PFR1_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = cpu->isar.id_aa64pfr1}, >> { .name = "ID_AA64PFR2_EL1_RESERVED", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2, >> @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU >> *cpu) >> { .name = "ID_AA64PFR3_EL1_RESERVED", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = 0 }, > > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? Indeed, I oversaw that one. I'll fix it and repost it together with the extra patches to handle TID1 and TID2. > >> { .name = "ID_AA64ZFR0_EL1", .state = >> ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> /* At present, only SVEver == 0 is defined anyway. >> */ >> .resetvalue = 0 }, > >> { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64, >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0, >> .access = PL1_R, .type = ARM_CP_CONST, >> + .accessfn = access_aa64idreg, >> .resetvalue = cpu->isar.mvfr0 }, > > These are the AArch64 accessors for the aarch32 MVFR registers, > but this trap bit is also supposed to trap aarch32 accesses to > the MVFR registers, which are via the vmrs insn. That needs > to be done in trans_VMSR_VMRS(); we can do that with a > followup patch, since the mechanism there is completely different. Holy cow! I'm afraid that my newly acquired QEMU-foo is missing a few key options. Does it mean we need to generate a trapping instruction, as opposed to take the exception on access? I'd very much appreciate some guidance here. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-25 17:08 ` Marc Zyngier @ 2019-11-25 17:27 ` Peter Maydell -1 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-25 17:27 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Mon, 25 Nov 2019 at 17:08, Marc Zyngier <maz@kernel.org> wrote: > > On 2019-11-25 16:21, Peter Maydell wrote: > > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? > > Indeed, I oversaw that one. I'll fix it and repost it together with > the extra patches to handle TID1 and TID2. Given that rc3 (last chance, more or less, for bugfixes for 4.2) is tomorrow, I propose that I take this patch with the + .accessfn = access_aa64idreg, line for ID_AA64PFR2_EL1_RESERVED squashed in. I think TID1/TID2 and the MMFR-from-aarch32 parts are less urgent ? > > These are the AArch64 accessors for the aarch32 MVFR registers, > > but this trap bit is also supposed to trap aarch32 accesses to > > the MVFR registers, which are via the vmrs insn. That needs > > to be done in trans_VMSR_VMRS(); we can do that with a > > followup patch, since the mechanism there is completely different. > > Holy cow! I'm afraid that my newly acquired QEMU-foo is missing > a few key options. Does it mean we need to generate a trapping > instruction, as opposed to take the exception on access? We will need to generate a call to a helper function which does the access check (and possibly raises an exception). We can't determine at translate time whether the insn is going to cause an exception, because the TID3 bit is not one of the fairly limited set of information which we put into the TB flags and allow to be baked into the generated code. (In theory we could add it, but we don't have very many TB flags so we only do that for cases where the overhead of doing the check at runtime matters.) thanks -- PMM ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 17:27 ` Peter Maydell 0 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-25 17:27 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Mon, 25 Nov 2019 at 17:08, Marc Zyngier <maz@kernel.org> wrote: > > On 2019-11-25 16:21, Peter Maydell wrote: > > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? > > Indeed, I oversaw that one. I'll fix it and repost it together with > the extra patches to handle TID1 and TID2. Given that rc3 (last chance, more or less, for bugfixes for 4.2) is tomorrow, I propose that I take this patch with the + .accessfn = access_aa64idreg, line for ID_AA64PFR2_EL1_RESERVED squashed in. I think TID1/TID2 and the MMFR-from-aarch32 parts are less urgent ? > > These are the AArch64 accessors for the aarch32 MVFR registers, > > but this trap bit is also supposed to trap aarch32 accesses to > > the MVFR registers, which are via the vmrs insn. That needs > > to be done in trans_VMSR_VMRS(); we can do that with a > > followup patch, since the mechanism there is completely different. > > Holy cow! I'm afraid that my newly acquired QEMU-foo is missing > a few key options. Does it mean we need to generate a trapping > instruction, as opposed to take the exception on access? We will need to generate a call to a helper function which does the access check (and possibly raises an exception). We can't determine at translate time whether the insn is going to cause an exception, because the TID3 bit is not one of the fairly limited set of information which we put into the TB flags and allow to be baked into the generated code. (In theory we could add it, but we don't have very many TB flags so we only do that for cases where the overhead of doing the check at runtime matters.) thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-25 17:27 ` Peter Maydell @ 2019-11-25 17:49 ` Marc Zyngier -1 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 17:49 UTC (permalink / raw) To: Peter Maydell; +Cc: Will Deacon, QEMU Developers, kvmarm On 2019-11-25 17:27, Peter Maydell wrote: > On Mon, 25 Nov 2019 at 17:08, Marc Zyngier <maz@kernel.org> wrote: >> >> On 2019-11-25 16:21, Peter Maydell wrote: >> > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? >> >> Indeed, I oversaw that one. I'll fix it and repost it together with >> the extra patches to handle TID1 and TID2. > > Given that rc3 (last chance, more or less, for bugfixes for 4.2) > is tomorrow, I propose that I take this patch with the > + .accessfn = access_aa64idreg, > line for ID_AA64PFR2_EL1_RESERVED squashed in. I think > TID1/TID2 and the MMFR-from-aarch32 parts are less urgent ? That'd be great, thank you. TID2 is only a nice to have (it allows us to virtualize the cache hierarchy, only a performance optimization for fairly stupid 32bit guests), and TID1 is so far unused by Linux. I also had a look at TID0, but couldn't find any of the Jazelle stuff in QEMU... >> > These are the AArch64 accessors for the aarch32 MVFR registers, >> > but this trap bit is also supposed to trap aarch32 accesses to >> > the MVFR registers, which are via the vmrs insn. That needs >> > to be done in trans_VMSR_VMRS(); we can do that with a >> > followup patch, since the mechanism there is completely different. >> >> Holy cow! I'm afraid that my newly acquired QEMU-foo is missing >> a few key options. Does it mean we need to generate a trapping >> instruction, as opposed to take the exception on access? > > We will need to generate a call to a helper function which > does the access check (and possibly raises an exception). > We can't determine at translate time whether the insn is > going to cause an exception, because the TID3 bit is not one > of the fairly limited set of information which we put into > the TB flags and allow to be baked into the generated code. > (In theory we could add it, but we don't have very many TB > flags so we only do that for cases where the overhead of doing > the check at runtime matters.) Ah! I guess you're referring to the HELPER() stuff in the various target/arm/*_helper.c files? If so, I think I see how to do it. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-25 17:49 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-25 17:49 UTC (permalink / raw) To: Peter Maydell; +Cc: Will Deacon, QEMU Developers, kvmarm On 2019-11-25 17:27, Peter Maydell wrote: > On Mon, 25 Nov 2019 at 17:08, Marc Zyngier <maz@kernel.org> wrote: >> >> On 2019-11-25 16:21, Peter Maydell wrote: >> > Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ? >> >> Indeed, I oversaw that one. I'll fix it and repost it together with >> the extra patches to handle TID1 and TID2. > > Given that rc3 (last chance, more or less, for bugfixes for 4.2) > is tomorrow, I propose that I take this patch with the > + .accessfn = access_aa64idreg, > line for ID_AA64PFR2_EL1_RESERVED squashed in. I think > TID1/TID2 and the MMFR-from-aarch32 parts are less urgent ? That'd be great, thank you. TID2 is only a nice to have (it allows us to virtualize the cache hierarchy, only a performance optimization for fairly stupid 32bit guests), and TID1 is so far unused by Linux. I also had a look at TID0, but couldn't find any of the Jazelle stuff in QEMU... >> > These are the AArch64 accessors for the aarch32 MVFR registers, >> > but this trap bit is also supposed to trap aarch32 accesses to >> > the MVFR registers, which are via the vmrs insn. That needs >> > to be done in trans_VMSR_VMRS(); we can do that with a >> > followup patch, since the mechanism there is completely different. >> >> Holy cow! I'm afraid that my newly acquired QEMU-foo is missing >> a few key options. Does it mean we need to generate a trapping >> instruction, as opposed to take the exception on access? > > We will need to generate a call to a helper function which > does the access check (and possibly raises an exception). > We can't determine at translate time whether the insn is > going to cause an exception, because the TID3 bit is not one > of the fairly limited set of information which we put into > the TB flags and allow to be baked into the generated code. > (In theory we could add it, but we don't have very many TB > flags so we only do that for cases where the overhead of doing > the check at runtime matters.) Ah! I guess you're referring to the HELPER() stuff in the various target/arm/*_helper.c files? If so, I think I see how to do it. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-25 17:49 ` Marc Zyngier @ 2019-11-26 12:46 ` Peter Maydell -1 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-26 12:46 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Mon, 25 Nov 2019 at 17:49, Marc Zyngier <maz@kernel.org> wrote: > I also had a look at TID0, but couldn't find any of the Jazelle > stuff in QEMU... We implement only "minimal Jazelle", ie the minimal set of registers needed to be architecturally compliant for an implementation without Jazelle. thanks -- PMM ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-26 12:46 ` Peter Maydell 0 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-26 12:46 UTC (permalink / raw) To: Marc Zyngier; +Cc: Will Deacon, QEMU Developers, kvmarm On Mon, 25 Nov 2019 at 17:49, Marc Zyngier <maz@kernel.org> wrote: > I also had a look at TID0, but couldn't find any of the Jazelle > stuff in QEMU... We implement only "minimal Jazelle", ie the minimal set of registers needed to be architecturally compliant for an implementation without Jazelle. thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-23 11:56 ` Marc Zyngier @ 2019-11-26 10:12 ` Richard Henderson -1 siblings, 0 replies; 24+ messages in thread From: Richard Henderson @ 2019-11-26 10:12 UTC (permalink / raw) To: Marc Zyngier, qemu-devel; +Cc: Peter Maydell, Will Deacon, kvmarm On 11/23/19 11:56 AM, Marc Zyngier wrote: > +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, > + bool isread) > +{ > + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { > + return CP_ACCESS_TRAP_EL2; > + } > + > + return CP_ACCESS_OK; > +} > + The only thing I would suggest is to call this access_aa64_tid3, because tid{0,1,2} also need to be handled in a similar way, and we'll need little helper functions for those too. r~ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-26 10:12 ` Richard Henderson 0 siblings, 0 replies; 24+ messages in thread From: Richard Henderson @ 2019-11-26 10:12 UTC (permalink / raw) To: Marc Zyngier, qemu-devel; +Cc: Will Deacon, kvmarm On 11/23/19 11:56 AM, Marc Zyngier wrote: > +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, > + bool isread) > +{ > + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { > + return CP_ACCESS_TRAP_EL2; > + } > + > + return CP_ACCESS_OK; > +} > + The only thing I would suggest is to call this access_aa64_tid3, because tid{0,1,2} also need to be handled in a similar way, and we'll need little helper functions for those too. r~ _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-26 10:12 ` Richard Henderson @ 2019-11-26 13:19 ` Peter Maydell -1 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-26 13:19 UTC (permalink / raw) To: Richard Henderson; +Cc: Marc Zyngier, Will Deacon, QEMU Developers, kvmarm On Tue, 26 Nov 2019 at 10:12, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 11/23/19 11:56 AM, Marc Zyngier wrote: > > +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, > > + bool isread) > > +{ > > + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { > > + return CP_ACCESS_TRAP_EL2; > > + } > > + > > + return CP_ACCESS_OK; > > +} > > + > > The only thing I would suggest is to call this access_aa64_tid3, because > tid{0,1,2} also need to be handled in a similar way, and we'll need little > helper functions for those too. Good idea, I will make that change also. -- PMM ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-26 13:19 ` Peter Maydell 0 siblings, 0 replies; 24+ messages in thread From: Peter Maydell @ 2019-11-26 13:19 UTC (permalink / raw) To: Richard Henderson; +Cc: Marc Zyngier, Will Deacon, QEMU Developers, kvmarm On Tue, 26 Nov 2019 at 10:12, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 11/23/19 11:56 AM, Marc Zyngier wrote: > > +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo *ri, > > + bool isread) > > +{ > > + if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) { > > + return CP_ACCESS_TRAP_EL2; > > + } > > + > > + return CP_ACCESS_OK; > > +} > > + > > The only thing I would suggest is to call this access_aa64_tid3, because > tid{0,1,2} also need to be handled in a similar way, and we'll need little > helper functions for those too. Good idea, I will make that change also. -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-23 11:56 ` Marc Zyngier @ 2019-11-26 21:04 ` Richard Henderson -1 siblings, 0 replies; 24+ messages in thread From: Richard Henderson @ 2019-11-26 21:04 UTC (permalink / raw) To: Marc Zyngier, qemu-devel; +Cc: Peter Maydell, Will Deacon, kvmarm On 11/23/19 11:56 AM, Marc Zyngier wrote: > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. BTW, Peter, this appears to have been the bug that was causing me so many problems on my VHE branch. Probably *exactly* this bug wrt ptrauth, since that would also be included with -cpu max. I am now able to boot a kvm guest kernel to the point of the no rootfs panic, which I wasn't before. I can only think that I mis-identified the true cause in Lyon. Anyway, thanks Marc! r~ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-26 21:04 ` Richard Henderson 0 siblings, 0 replies; 24+ messages in thread From: Richard Henderson @ 2019-11-26 21:04 UTC (permalink / raw) To: Marc Zyngier, qemu-devel; +Cc: Will Deacon, kvmarm On 11/23/19 11:56 AM, Marc Zyngier wrote: > HCR_EL2.TID3 mandates that access from EL1 to a long list of id > registers traps to EL2, and QEMU has so far ignored this requirement. > > This breaks (among other things) KVM guests that have PtrAuth enabled, > while the hypervisor doesn't want to expose the feature to its guest. > To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this > case), and masks out the unsupported feature. > > QEMU not honoring the trap request means that the guest observes > that the feature is present in the HW, starts using it, and dies > a horrible death when KVM injects an UNDEF, because the feature > *really* isn't supported. > > Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > There is a number of other trap bits missing (TID[0-2], for example), > but this at least gets a mainline Linux going with cpu=max. BTW, Peter, this appears to have been the bug that was causing me so many problems on my VHE branch. Probably *exactly* this bug wrt ptrauth, since that would also be included with -cpu max. I am now able to boot a kvm guest kernel to the point of the no rootfs panic, which I wasn't before. I can only think that I mis-identified the true cause in Lyon. Anyway, thanks Marc! r~ _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements 2019-11-26 21:04 ` Richard Henderson @ 2019-11-27 9:13 ` Marc Zyngier -1 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-27 9:13 UTC (permalink / raw) To: Richard Henderson; +Cc: Peter Maydell, Will Deacon, qemu-devel, kvmarm On 2019-11-26 21:04, Richard Henderson wrote: > On 11/23/19 11:56 AM, Marc Zyngier wrote: >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. > > BTW, Peter, this appears to have been the bug that was causing me so > many > problems on my VHE branch. Probably *exactly* this bug wrt ptrauth, > since that would also be included with -cpu max. > > I am now able to boot a kvm guest kernel to the point of the no > rootfs panic, > which I wasn't before. > > I can only think that I mis-identified the true cause in Lyon. > > Anyway, thanks Marc! Hehe, glad it fixed more than just my pet issue! :-) M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements @ 2019-11-27 9:13 ` Marc Zyngier 0 siblings, 0 replies; 24+ messages in thread From: Marc Zyngier @ 2019-11-27 9:13 UTC (permalink / raw) To: Richard Henderson; +Cc: Will Deacon, qemu-devel, kvmarm On 2019-11-26 21:04, Richard Henderson wrote: > On 11/23/19 11:56 AM, Marc Zyngier wrote: >> HCR_EL2.TID3 mandates that access from EL1 to a long list of id >> registers traps to EL2, and QEMU has so far ignored this >> requirement. >> >> This breaks (among other things) KVM guests that have PtrAuth >> enabled, >> while the hypervisor doesn't want to expose the feature to its >> guest. >> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in >> this >> case), and masks out the unsupported feature. >> >> QEMU not honoring the trap request means that the guest observes >> that the feature is present in the HW, starts using it, and dies >> a horrible death when KVM injects an UNDEF, because the feature >> *really* isn't supported. >> >> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set. >> >> Reported-by: Will Deacon <will@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> There is a number of other trap bits missing (TID[0-2], for >> example), >> but this at least gets a mainline Linux going with cpu=max. > > BTW, Peter, this appears to have been the bug that was causing me so > many > problems on my VHE branch. Probably *exactly* this bug wrt ptrauth, > since that would also be included with -cpu max. > > I am now able to boot a kvm guest kernel to the point of the no > rootfs panic, > which I wasn't before. > > I can only think that I mis-identified the true cause in Lyon. > > Anyway, thanks Marc! Hehe, glad it fixed more than just my pet issue! :-) M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-11-27 9:14 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-23 11:56 [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements Marc Zyngier 2019-11-23 11:56 ` Marc Zyngier 2019-11-25 10:40 ` Will Deacon 2019-11-25 10:40 ` Will Deacon 2019-11-25 10:59 ` Marc Zyngier 2019-11-25 10:59 ` Marc Zyngier 2019-11-25 16:21 ` Peter Maydell 2019-11-25 16:21 ` Peter Maydell 2019-11-25 17:08 ` Marc Zyngier 2019-11-25 17:08 ` Marc Zyngier 2019-11-25 17:27 ` Peter Maydell 2019-11-25 17:27 ` Peter Maydell 2019-11-25 17:49 ` Marc Zyngier 2019-11-25 17:49 ` Marc Zyngier 2019-11-26 12:46 ` Peter Maydell 2019-11-26 12:46 ` Peter Maydell 2019-11-26 10:12 ` Richard Henderson 2019-11-26 10:12 ` Richard Henderson 2019-11-26 13:19 ` Peter Maydell 2019-11-26 13:19 ` Peter Maydell 2019-11-26 21:04 ` Richard Henderson 2019-11-26 21:04 ` Richard Henderson 2019-11-27 9:13 ` Marc Zyngier 2019-11-27 9:13 ` Marc Zyngier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.