| Message ID | 20211123210109.1605642-4-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Emulate the OS lock | expand |
On Tue, 23 Nov 2021 21:01:06 +0000, Oliver Upton <oupton@google.com> 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 | 6 ++++++ > arch/arm64/kvm/sys_regs.c | 33 ++++++++++++++++++++++++++------- > 2 files changed, 32 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 16b3f1a1d468..9fad61a82047 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -129,7 +129,13 @@ > #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_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..5dbdb45d6d44 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,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > if (err) > return err; > > - if (val != rd->val) > + if ((val & ~SYS_OSLSR_OSLK) != rd->val) > return -EINVAL; This looks odd. It means that once I have set the lock from userspace, I can't clear it? Thanks, M.
On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 23 Nov 2021 21:01:06 +0000, > Oliver Upton <oupton@google.com> 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 | 6 ++++++ > > arch/arm64/kvm/sys_regs.c | 33 ++++++++++++++++++++++++++------- > > 2 files changed, 32 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > > index 16b3f1a1d468..9fad61a82047 100644 > > --- a/arch/arm64/include/asm/sysreg.h > > +++ b/arch/arm64/include/asm/sysreg.h > > @@ -129,7 +129,13 @@ > > #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_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..5dbdb45d6d44 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,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > if (err) > > return err; > > > > - if (val != rd->val) > > + if ((val & ~SYS_OSLSR_OSLK) != rd->val) > > return -EINVAL; > > This looks odd. It means that once I have set the lock from userspace, > I can't clear it? This does read weird, but I believe it makes sense still. rd->val is the value of the register after warm reset, and does not store the current value of the actual register. The true value is stashed in kvm_cpu_context. Really, what I'm asserting here is that the only RW bit is the OSLK bit. If any of the other bits are changed it should return an error. I can either add a comment or make a macro for the expected register value (or both) to make this more clear. -- Thanks, Oliver
Hi Oliver, On Mon, 06 Dec 2021 17:39:05 +0000, Oliver Upton <oupton@google.com> wrote: > > On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 23 Nov 2021 21:01:06 +0000, > > Oliver Upton <oupton@google.com> 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 | 6 ++++++ > > > arch/arm64/kvm/sys_regs.c | 33 ++++++++++++++++++++++++++------- > > > 2 files changed, 32 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > > > index 16b3f1a1d468..9fad61a82047 100644 > > > --- a/arch/arm64/include/asm/sysreg.h > > > +++ b/arch/arm64/include/asm/sysreg.h > > > @@ -129,7 +129,13 @@ > > > #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_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..5dbdb45d6d44 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,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > > if (err) > > > return err; > > > > > > - if (val != rd->val) > > > + if ((val & ~SYS_OSLSR_OSLK) != rd->val) > > > return -EINVAL; > > > > This looks odd. It means that once I have set the lock from userspace, > > I can't clear it? > > This does read weird, but I believe it makes sense still. rd->val is > the value of the register after warm reset, and does not store the > current value of the actual register. The true value is stashed in > kvm_cpu_context. Really, what I'm asserting here is that the only RW > bit is the OSLK bit. If any of the other bits are changed it should > return an error. Ah, the beauty of reading code in patches only. Of course, rd->val is only the reset value. And isn't called that, just to be confusing. Apologies for the noise. > I can either add a comment or make a macro for the expected register > value (or both) to make this more clear. A macro for the reset value would certainly be beneficial. But also, why not check the value against the current state and ignore the reset state altogether, since by the time you can poke at the vcpu, it has already been reset? It would certainly be more idiomatic. Thanks, M.
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 16b3f1a1d468..9fad61a82047 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -129,7 +129,13 @@ #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_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..5dbdb45d6d44 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,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, if (err) return err; - if (val != rd->val) + if ((val & ~SYS_OSLSR_OSLK) != rd->val) return -EINVAL; + __vcpu_sys_reg(vcpu, rd->reg) = val; return 0; } @@ -1180,10 +1203,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,7 +1482,7 @@ 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_OSLAR_EL1), trap_oslar_el1 }, { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008, .set_user = set_oslsr_el1, }, { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi }, @@ -1937,7 +1956,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 },