| Message ID | 20250210154953.27002-2-sebott@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: writable MIDR/REVIDR | expand |
Hi Sebastian, On Mon, Feb 10, 2025 at 04:49:50PM +0100, Sebastian Ott wrote: > Enable VMMs to write MIDR_EL1 by treating it as a VM ID register. > Since MIDR_EL1 is not handled as a proper arm64_ftr_reg apply only > a sanity check against the writable mask to ensure the reserved > bits are 0. How exactly does the VMM's MIDR_EL1 find its way to the guest? VPIDR_EL2 is still set to the hardware value. > @@ -2021,7 +2037,11 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return ret; > } > > - ret = arm64_check_features(vcpu, rd, val); > + if (skip_feature_check(id)) > + ret = arm64_check_mask(rd, val); > + else > + ret = arm64_check_features(vcpu, rd, val); > + Can you add a new implementation of ->set_user() for MIDR/REVIDR/AIDR instead? > @@ -2542,6 +2571,8 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 }, > > + { ID_DESC(MIDR_EL1), .set_user = set_id_reg, .visibility = id_visibility, > + .reset = reset_midr_el1, .val = (u32)-1 }, nit: GENMASK() instead of truncation by casting.
Hi Oliver, On Mon, 10 Feb 2025, Oliver Upton wrote: > On Mon, Feb 10, 2025 at 04:49:50PM +0100, Sebastian Ott wrote: >> Enable VMMs to write MIDR_EL1 by treating it as a VM ID register. >> Since MIDR_EL1 is not handled as a proper arm64_ftr_reg apply only >> a sanity check against the writable mask to ensure the reserved >> bits are 0. > > How exactly does the VMM's MIDR_EL1 find its way to the guest? VPIDR_EL2 > is still set to the hardware value. Ouch. Completely missed that part, sry. > >> @@ -2021,7 +2037,11 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, >> return ret; >> } >> >> - ret = arm64_check_features(vcpu, rd, val); >> + if (skip_feature_check(id)) >> + ret = arm64_check_mask(rd, val); >> + else >> + ret = arm64_check_features(vcpu, rd, val); >> + > > Can you add a new implementation of ->set_user() for MIDR/REVIDR/AIDR > instead? Yes, sure. >> @@ -2542,6 +2571,8 @@ static const struct sys_reg_desc sys_reg_descs[] = { >> >> { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 }, >> >> + { ID_DESC(MIDR_EL1), .set_user = set_id_reg, .visibility = id_visibility, >> + .reset = reset_midr_el1, .val = (u32)-1 }, > > nit: GENMASK() instead of truncation by casting. All done. I add a test and send out V2. Thanks a lot! Sebastian
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7cfa024de4e3..3db8c773339e 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -373,6 +373,7 @@ struct kvm_arch { #define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1) u64 id_regs[KVM_ARM_ID_REG_NUM]; + u64 midr_el1; u64 ctr_el0; /* Masks for VNCR-backed and general EL2 sysregs */ @@ -1469,6 +1470,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg) switch (reg) { case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7): return &ka->id_regs[IDREG_IDX(reg)]; + case SYS_MIDR_EL1: + return &ka->midr_el1; case SYS_CTR_EL0: return &ka->ctr_el0; default: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 82430c1e1dd0..cc94bed7299d 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1666,7 +1666,7 @@ static bool is_feature_id_reg(u32 encoding) */ static inline bool is_vm_ftr_id_reg(u32 id) { - if (id == SYS_CTR_EL0) + if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1) return true; return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && @@ -1999,6 +1999,22 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return 0; } +static bool skip_feature_check(u32 reg) +{ + return (reg == SYS_MIDR_EL1); +} + +/* + * For non ftr regs do a limited test against the writable mask only. + */ +static int arm64_check_mask(const struct sys_reg_desc *rd, u64 val) +{ + if ((rd->val & val) != val) + return -EINVAL; + + return 0; +} + static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, u64 val) { @@ -2021,7 +2037,11 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, return ret; } - ret = arm64_check_features(vcpu, rd, val); + if (skip_feature_check(id)) + ret = arm64_check_mask(rd, val); + else + ret = arm64_check_features(vcpu, rd, val); + if (!ret) kvm_set_vm_id_reg(vcpu->kvm, id, val); @@ -2493,6 +2513,15 @@ static bool access_mdcr(struct kvm_vcpu *vcpu, return true; } +#define FUNCTION_RESET(reg) \ + static u64 reset_##reg(struct kvm_vcpu *v, \ + const struct sys_reg_desc *r) \ + { \ + return read_sysreg(reg); \ + } + +FUNCTION_RESET(midr_el1) + /* * Architected system registers. @@ -2542,6 +2571,8 @@ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 }, + { ID_DESC(MIDR_EL1), .set_user = set_id_reg, .visibility = id_visibility, + .reset = reset_midr_el1, .val = (u32)-1 }, { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 }, /* @@ -4594,13 +4625,11 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, return ((struct sys_reg_desc *)r)->val; \ } -FUNCTION_INVARIANT(midr_el1) FUNCTION_INVARIANT(revidr_el1) FUNCTION_INVARIANT(aidr_el1) /* ->val is filled in by kvm_sys_reg_table_init() */ static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = { - { SYS_DESC(SYS_MIDR_EL1), NULL, reset_midr_el1 }, { SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 }, { SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 }, };
Enable VMMs to write MIDR_EL1 by treating it as a VM ID register. Since MIDR_EL1 is not handled as a proper arm64_ftr_reg apply only a sanity check against the writable mask to ensure the reserved bits are 0. Signed-off-by: Sebastian Ott <sebott@redhat.com> --- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/sys_regs.c | 37 +++++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 4 deletions(-)