Re: [PATCH v4 3/6] KVM: arm64: Allow guest to set the OSLK bit
From: Mark Rutland <mark.rutland@arm.com>
Date: 2021-12-15 12:16:40
Also in:
kvm, kvmarm
On Tue, Dec 14, 2021 at 05:28:09PM +0000, Oliver Upton wrote:
quoted hunk ↗ jump to hunk
Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with the value for now. Reviewed-by: Reiji Watanabe <redacted> Signed-off-by: Oliver Upton <redacted> --- arch/arm64/include/asm/sysreg.h | 9 ++++++++ arch/arm64/kvm/sys_regs.c | 39 ++++++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 8 deletions(-)diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 16b3f1a1d468..46f800bda045 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h@@ -129,7 +129,16 @@ #define SYS_DBGWCRn_EL1(n) sys_reg(2, 0, 0, n, 7) #define SYS_MDRAR_EL1 sys_reg(2, 0, 1, 0, 0) #define SYS_OSLAR_EL1 sys_reg(2, 0, 1, 0, 4) + +#define SYS_OSLAR_OSLK BIT(0) + #define SYS_OSLSR_EL1 sys_reg(2, 0, 1, 1, 4) + +#define SYS_OSLSR_OSLK BIT(1) + +#define SYS_OSLSR_OSLM_MASK (BIT(3) | BIT(0)) +#define SYS_OSLSR_OSLM BIT(3)
Since `OSLM` is the field as a whole, I think this should have another level of hierarchy, e.g. #define SYS_OSLSR_OSLM_MASK (BIT(3) | BIT(0)) #define SYS_OSLSR_OSLM_NI 0 #define SYS_OSLSR_OSLM_OSLK BIT(3) [...]
+static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ u64 oslsr;
+
+ if (!p->is_write)
+ return read_from_write_only(vcpu, p, r);
+
+ /* Forward the OSLK bit to OSLSR */
+ oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
+ if (p->regval & SYS_OSLAR_OSLK)
+ oslsr |= SYS_OSLSR_OSLK;
+
+ __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
+ return true;
+}Does changing this affect existing userspace? Previosuly it could read OSLAR_EL1 as 0, whereas now that should be rejected. That might be fine, and if so, it would be good to call that out in the commit message. [...]
quoted hunk ↗ jump to hunk
@@ -309,9 +331,14 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, if (err) return err; - if (val != rd->val) + /* + * The only modifiable bit is the OSLK bit. Refuse the write if + * userspace attempts to change any other bit in the register. + */ + if ((val & ~SYS_OSLSR_OSLK) != SYS_OSLSR_OSLM) return -EINVAL;
How about: if ((val ^ rd->val) & ~SYS_OSLSR_OSLK) return -EINVAL; ... so that we don't need to hard-code the expected value here, and can more easily change it in future? [...]
quoted hunk ↗ jump to hunk
@@ -1463,8 +1486,8 @@ static const struct sys_reg_desc sys_reg_descs[] = { DBG_BCR_BVR_WCR_WVR_EL1(15), { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi }, - { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi }, - { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008, + { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 }, + { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, SYS_OSLSR_OSLM, .set_user = set_oslsr_el1, }, { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi }, { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },@@ -1937,7 +1960,7 @@ static const struct sys_reg_desc cp14_regs[] = { DBGBXVR(0), /* DBGOSLAR */ - { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi }, + { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
As above, I have a slight concern that this could adversely affect existing userspace, but I can also believe that's fine. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel