| Message ID | 20211214172812.2894560-4-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Emulate the OS Lock | expand |
On Tue, Dec 14, 2021 at 05:28:09PM +0000, Oliver Upton wrote: > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with > the value for now. > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Oliver Upton <oupton@google.com> > --- > 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. [...] > @@ -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? [...] > @@ -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.
Hi Mark, Sorry for the delay on my end.. On Wed, Dec 15, 2021 at 4:15 AM Mark Rutland <mark.rutland@arm.com> wrote: > > +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. I do not believe we expose OSLAR_EL1 to userspace. Attempts to read it return -ENOENT. The access will go through get_invariant_sys_reg(), which cannot find a corresponding entry in the invariant_sys_regs array. [...] > > @@ -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? Nice and clean. Thanks! -- Best, Oliver
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) + #define SYS_OSDLR_EL1 sys_reg(2, 0, 1, 3, 4) #define SYS_DBGPRCR_EL1 sys_reg(2, 0, 1, 4, 4) #define SYS_DBGCLAIMSET_EL1 sys_reg(2, 0, 7, 8, 6) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 7bf350b3d9cd..5188a74095e3 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -44,6 +44,10 @@ * 64bit interface. */ +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id); +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id); +static u64 sys_reg_to_index(const struct sys_reg_desc *reg); + static bool read_from_write_only(struct kvm_vcpu *vcpu, struct sys_reg_params *params, const struct sys_reg_desc *r) @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, return trap_raz_wi(vcpu, p, r); } +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; +} + static bool trap_oslsr_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) @@ -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; + __vcpu_sys_reg(vcpu, rd->reg) = val; return 0; } @@ -1180,10 +1207,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu, return __access_id_reg(vcpu, p, r, true); } -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id); -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id); -static u64 sys_reg_to_index(const struct sys_reg_desc *reg); - /* Visibility overrides for SVE-specific control registers */ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) @@ -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 }, DBGBXVR(1), /* DBGOSLSR */ { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },