On Wed, Aug 31, 2022 at 7:42 AM Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
>
> On Tue, Aug 30, 2022 at 08:29:37PM -0700, Reiji Watanabe wrote:
> > Hi Oliver,
> >
> > On Wed, Aug 17, 2022 at 2:48 PM Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
> > >
> > > We're about to ignore writes to AArch32 ID registers on AArch64-only
> > > systems. Add a bit to indicate a register is handled as write ignore
> > > when accessed from userspace.
> > >
> > > Signed-off-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> > > ---
> > > arch/arm64/kvm/sys_regs.c | 3 +++
> > > arch/arm64/kvm/sys_regs.h | 7 +++++++
> > > 2 files changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 26210f3a0b27..9f06c85f26b8 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1232,6 +1232,9 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > {
> > > bool raz = sysreg_visible_as_raz(vcpu, rd);
> > >
> > > + if (sysreg_user_write_ignore(vcpu, rd))
> > > + return 0;
> >
> > Since the visibility flags are not ID register specific,
> > have you considered checking REG_USER_WI from kvm_sys_reg_set_user()
> > rather than the ID register specific function ?
>
> Yeah, that's definitely a better place to wire it in.
>
> > This patch made me reconsider my comment for the patch-2.
> > Perhaps it might be more appropriate to check RAZ visibility from
> > kvm_sys_reg_get_user() rather than the ID register specific function ?
>
> REG_RAZ hides the register value from the guest as well as userspace, so it
> might be better to leave it in place. REG_RAZ also has implications for
> writing a register from userspace, as we still apply the expectation of
> invariance to ID registers that set this flag.
>
> It all 'just works' right now with the check buried in the ID register
> accessors. Going the other way around would require sprinkling the check
> in several locations.
Ah, I see the handling of REG_RAZ is a bit tricky...
I kind of suspect that REG_RAZ won't probably be used for any registers
other than ID registers even in the future...
Anyway, yes, it might be better to leave it in place at least for now.
Thank you,
Reiji
