On Tue, 25 Feb 2025 00:53:57 +0000, Oliver Upton oliver.upton@linux.dev wrote:
commit 90807748ca3a ("KVM: arm64: Hide SME system registers from guests") added trap handling for SMIDR_EL1, treating it as UNDEFINED as KVM does not support SME. This is right for the most part, however KVM needs to set HCR_EL2.TID1 to _actually_ trap the register.
Unfortunately, this comes with some collateral damage as TID1 forces REVIDR_EL1 and AIDR_EL1 to trap as well. KVM has long treated these registers as "invariant" which is an awful term for the following:
Userspace sees the boot CPU values on all vCPUs
The guest sees the hardware values of the CPU on which a vCPU is scheduled
Keep the plates spinning by adding trap handling for the affected registers and repaint all of the "invariant" crud into terms of identifying an implementation. Yes, at this point we only need to set TID1 on SME hardware, but REVIDR_EL1 and AIDR_EL1 are about to become mutable anyway.
Cc: Mark Brown broonie@kernel.org Cc: stable@vger.kernel.org Fixes: 90807748ca3a ("KVM: arm64: Hide SME system registers from guests") Signed-off-by: Oliver Upton oliver.upton@linux.dev
arch/arm64/include/asm/kvm_arm.h | 4 +- arch/arm64/kvm/sys_regs.c | 175 ++++++++++++++++--------------- 2 files changed, 94 insertions(+), 85 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 8d94a6c0ed5c..b01c01e55de5 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -92,12 +92,12 @@
- SWIO: Turn set/way invalidates into set/way clean+invalidate
- PTW: Take a stage2 fault if a stage1 walk steps in device memory
- TID3: Trap EL1 reads of group 3 ID registers
- TID2: Trap CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and CSSELR_EL1
*/
- TID1: Trap REVIDR_EL1, AIDR_EL1, and SMIDR_EL1
#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ HCR_BSU_IS | HCR_FB | HCR_TACR | \ HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3)
HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 | HCR_TID1)#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA) #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC) #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index f6cd1ea7fb55..f25a157622e3 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2483,6 +2483,93 @@ static bool access_mdcr(struct kvm_vcpu *vcpu, return true; } +/*
- For historical (ahem ABI) reasons, KVM treats MIDR_EL1, REVIDR_EL1, and
- AIDR_EL1 as "invariant" registers, meaning userspace cannot change them.
For consistency, using the past tense would make a lot more sense.
- The values made visible to userspace were the register values of the boot
- CPU.
- At the same time, reads from these registers at EL1 previously were not
- trapped, allowing the guest to read the actual hardware value. On big-little
- machines, this means the VM can see different values depending on where a
- given vCPU got scheduled.
- These registers are now trapped as collateral damage from SME, and what
- follows attempts to give a user / guest view consistent with the existing
- ABI.
- */
+static bool access_imp_id_reg(struct kvm_vcpu *vcpu,
struct sys_reg_params *p,const struct sys_reg_desc *r)+{
- if (p->is_write)
return write_to_read_only(vcpu, p, r);- switch (reg_to_encoding(r)) {
- case SYS_REVIDR_EL1:
p->regval = read_sysreg(revidr_el1);break;- case SYS_AIDR_EL1:
p->regval = read_sysreg(aidr_el1);break;- default:
WARN_ON_ONCE(1);- }
- return true;
+}
+static u64 __ro_after_init boot_cpu_midr_val; +static u64 __ro_after_init boot_cpu_revidr_val; +static u64 __ro_after_init boot_cpu_aidr_val;
+static void init_imp_id_regs(void) +{
- boot_cpu_midr_val = read_sysreg(midr_el1);
- boot_cpu_revidr_val = read_sysreg(revidr_el1);
- boot_cpu_aidr_val = read_sysreg(aidr_el1);
+}
+static int get_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
u64 *val)+{
- switch (reg_to_encoding(r)) {
- case SYS_MIDR_EL1:
*val = boot_cpu_midr_val;break;- case SYS_REVIDR_EL1:
*val = boot_cpu_revidr_val;break;- case SYS_AIDR_EL1:
*val = boot_cpu_aidr_val;break;- default:
WARN_ON_ONCE(1);return -EINVAL;- }
- return 0;
+}
+static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
u64 val)+{
- u64 expected;
- int ret;
- ret = get_imp_id_reg(vcpu, r, &expected);
- if (ret)
return ret;- return (expected == val) ? 0 : -EINVAL;
+}
+#define IMPLEMENTATION_ID(reg) { \
- SYS_DESC(SYS_##reg), \
- .access = access_imp_id_reg, \
- .get_user = get_imp_id_reg, \
- .set_user = set_imp_id_reg, \
+} /*
- Architected system registers.
@@ -2532,7 +2619,9 @@ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
- IMPLEMENTATION_ID(MIDR_EL1), { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
- IMPLEMENTATION_ID(REVIDR_EL1),
/* * ID regs: all ID_SANITISED() entries here must have corresponding @@ -2804,6 +2893,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { .set_user = set_clidr, .val = ~CLIDR_EL1_RES0 }, { SYS_DESC(SYS_CCSIDR2_EL1), undef_access }, { SYS_DESC(SYS_SMIDR_EL1), undef_access },
- IMPLEMENTATION_ID(AIDR_EL1), { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 }, ID_FILTERED(CTR_EL0, ctr_el0, CTR_EL0_DIC_MASK |
I don't think this is enough. You also need to augment the cp15[] table to handle trapping for the 32bit versions of REVIDR/AIDR.
Thanks,
M.