| Message ID | 20220817214818.3243383-5-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system | expand |
Hi Oliver, On Wed, Aug 17, 2022 at 2:48 PM Oliver Upton <oliver.upton@linux.dev> 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@linux.dev> > --- > 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 ? 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_HIDDEN is already checked from kvm_sys_reg_{s,g}et_user() (indirectly). Thank you, Reiji > + > /* This is what we mean by invariant: you can't change it. */ > if (val != read_id_reg(vcpu, rd, raz)) > return -EINVAL; > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index e78b51059622..e4ebb3a379fd 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -86,6 +86,7 @@ struct sys_reg_desc { > > #define REG_HIDDEN (1 << 0) /* hidden from userspace and guest */ > #define REG_RAZ (1 << 1) /* RAZ from userspace and guest */ > +#define REG_USER_WI (1 << 2) /* WI from userspace only */ > > static __printf(2, 3) > inline void print_sys_reg_msg(const struct sys_reg_params *p, > @@ -157,6 +158,12 @@ static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu, > return sysreg_visibility(vcpu, r) & REG_RAZ; > } > > +static inline bool sysreg_user_write_ignore(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + return sysreg_visibility(vcpu, r) & REG_USER_WI; > +} > + > static inline int cmp_sys_reg(const struct sys_reg_desc *i1, > const struct sys_reg_desc *i2) > { > -- > 2.37.1.595.g718a3a8f04-goog > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
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@linux.dev> 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@linux.dev> > > --- > > 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. -- Thanks, Oliver
On Wed, Aug 31, 2022 at 7:42 AM Oliver Upton <oliver.upton@linux.dev> 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@linux.dev> 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@linux.dev> > > > --- > > > 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
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; + /* This is what we mean by invariant: you can't change it. */ if (val != read_id_reg(vcpu, rd, raz)) return -EINVAL; diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index e78b51059622..e4ebb3a379fd 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -86,6 +86,7 @@ struct sys_reg_desc { #define REG_HIDDEN (1 << 0) /* hidden from userspace and guest */ #define REG_RAZ (1 << 1) /* RAZ from userspace and guest */ +#define REG_USER_WI (1 << 2) /* WI from userspace only */ static __printf(2, 3) inline void print_sys_reg_msg(const struct sys_reg_params *p, @@ -157,6 +158,12 @@ static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu, return sysreg_visibility(vcpu, r) & REG_RAZ; } +static inline bool sysreg_user_write_ignore(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + return sysreg_visibility(vcpu, r) & REG_USER_WI; +} + static inline int cmp_sys_reg(const struct sys_reg_desc *i1, const struct sys_reg_desc *i2) {
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@linux.dev> --- arch/arm64/kvm/sys_regs.c | 3 +++ arch/arm64/kvm/sys_regs.h | 7 +++++++ 2 files changed, 10 insertions(+)