Re: [PATCH v3 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel