Hi Mark,
Sorry for the delay on my end..
On Wed, Dec 15, 2021 at 4:15 AM Mark Rutland <mark.rutland@xxxxxxx> 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
