| Message ID | 1553864452-15080-12-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
On Fri, Mar 29, 2019 at 01:00:36PM +0000, Dave Martin wrote: > Some optional features of the Arm architecture add new system > registers that are not present in the base architecture. > > Where these features are optional for the guest, the visibility of > these registers may need to depend on some runtime configuration, > such as a flag passed to KVM_ARM_VCPU_INIT. > > For example, ZCR_EL1 and ID_AA64ZFR0_EL1 need to be hidden if SVE > is not enabled for the guest, even though these registers may be > present in the hardware and visible to the host at EL2. > > Adding special-case checks all over the place for individual > registers is going to get messy as the number of conditionally- > visible registers grows. > > In order to help solve this problem, this patch adds a new sysreg > method visibility() that can be used to hook in any needed runtime > visibility checks. This method can currently return > REG_HIDDEN_USER to inhibit enumeration and ioctl access to the > register for userspace, and REG_HIDDEN_GUEST to inhibit runtime > access by the guest using MSR/MRS. Wrappers are added to allow > these flags to be conveniently queried. > > This approach allows a conditionally modified view of individual > system registers such as the CPU ID registers, in addition to > completely hiding register where appropriate. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > --- > > Changes since v5: > > * Rename the visibility override flags, add some comments, and rename/ > introduce helpers to make the purpose of this code clearer. > --- > arch/arm64/kvm/sys_regs.c | 24 +++++++++++++++++++++--- > arch/arm64/kvm/sys_regs.h | 25 +++++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index a5d14b5..c86a7b0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1927,6 +1927,12 @@ static void perform_access(struct kvm_vcpu *vcpu, > { > trace_kvm_sys_access(*vcpu_pc(vcpu), params, r); > > + /* Check for regs disabled by runtime config */ > + if (sysreg_hidden_from_guest(vcpu, r)) { > + kvm_inject_undefined(vcpu); > + return; > + } > + > /* > * Not having an accessor means that we have configured a trap > * that we don't know how to handle. This certainly qualifies > @@ -2438,6 +2444,10 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg > if (!r) > return get_invariant_sys_reg(reg->id, uaddr); > > + /* Check for regs disabled by runtime config */ > + if (sysreg_hidden_from_user(vcpu, r)) > + return -ENOENT; > + > if (r->get_user) > return (r->get_user)(vcpu, r, reg, uaddr); > > @@ -2459,6 +2469,10 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg > if (!r) > return set_invariant_sys_reg(reg->id, uaddr); > > + /* Check for regs disabled by runtime config */ > + if (sysreg_hidden_from_user(vcpu, r)) > + return -ENOENT; > + > if (r->set_user) > return (r->set_user)(vcpu, r, reg, uaddr); > > @@ -2515,7 +2529,8 @@ static bool copy_reg_to_user(const struct sys_reg_desc *reg, u64 __user **uind) > return true; > } > > -static int walk_one_sys_reg(const struct sys_reg_desc *rd, > +static int walk_one_sys_reg(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd, > u64 __user **uind, > unsigned int *total) > { > @@ -2526,6 +2541,9 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd, > if (!(rd->reg || rd->get_user)) > return 0; > > + if (sysreg_hidden_from_user(vcpu, rd)) > + return 0; > + > if (!copy_reg_to_user(rd, uind)) > return -EFAULT; > > @@ -2554,9 +2572,9 @@ static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind) > int cmp = cmp_sys_reg(i1, i2); > /* target-specific overrides generic entry. */ > if (cmp <= 0) > - err = walk_one_sys_reg(i1, &uind, &total); > + err = walk_one_sys_reg(vcpu, i1, &uind, &total); > else > - err = walk_one_sys_reg(i2, &uind, &total); > + err = walk_one_sys_reg(vcpu, i2, &uind, &total); > > if (err) > return err; > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index 3b1bc7f..2be9950 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -64,8 +64,15 @@ struct sys_reg_desc { > const struct kvm_one_reg *reg, void __user *uaddr); > int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr); > + > + /* Return mask of REG_* runtime visibility overrides */ > + unsigned int (*visibility)(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd); > }; > > +#define REG_HIDDEN_USER (1 << 0) /* hidden from userspace ioctls */ > +#define REG_HIDDEN_GUEST (1 << 1) /* hidden from guest */ > + > static inline void print_sys_reg_instr(const struct sys_reg_params *p) > { > /* Look, we even formatted it for you to paste into the table! */ > @@ -102,6 +109,24 @@ static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r > __vcpu_sys_reg(vcpu, r->reg) = r->val; > } > > +static inline bool sysreg_hidden_from_guest(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + if (likely(!r->visibility)) > + return false; > + > + return r->visibility(vcpu, r) & REG_HIDDEN_GUEST; > +} > + > +static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + if (likely(!r->visibility)) > + return false; > + > + return r->visibility(vcpu, r) & REG_HIDDEN_USER; > +} > + > static inline int cmp_sys_reg(const struct sys_reg_desc *i1, > const struct sys_reg_desc *i2) > { > -- > 2.1.4 > Reviewed-by: Andrew Jones <drjones@redhat.com>
Dave Martin <Dave.Martin@arm.com> writes: > Some optional features of the Arm architecture add new system > registers that are not present in the base architecture. > > Where these features are optional for the guest, the visibility of > these registers may need to depend on some runtime configuration, > such as a flag passed to KVM_ARM_VCPU_INIT. This combined with... > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -64,8 +64,15 @@ struct sys_reg_desc { > const struct kvm_one_reg *reg, void __user *uaddr); > int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr); > + > + /* Return mask of REG_* runtime visibility overrides */ > + unsigned int (*visibility)(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd); > }; this makes me wonder what sort of machines will see different register visibility depending on which vcpu you are running on? Otherwise is looks good to me: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On Wed, Apr 24, 2019 at 09:39:31AM +0000, Alex Bennée wrote: > > Dave Martin <Dave.Martin@arm.com> writes: > > > Some optional features of the Arm architecture add new system > > registers that are not present in the base architecture. > > > > Where these features are optional for the guest, the visibility of > > these registers may need to depend on some runtime configuration, > > such as a flag passed to KVM_ARM_VCPU_INIT. > > This combined with... > > --- a/arch/arm64/kvm/sys_regs.h > > +++ b/arch/arm64/kvm/sys_regs.h > > @@ -64,8 +64,15 @@ struct sys_reg_desc { > > const struct kvm_one_reg *reg, void __user *uaddr); > > int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > const struct kvm_one_reg *reg, void __user *uaddr); > > + > > + /* Return mask of REG_* runtime visibility overrides */ > > + unsigned int (*visibility)(const struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc *rd); > > }; > > this makes me wonder what sort of machines will see different register > visibility depending on which vcpu you are running on? Userspace can consciously set KVM_ARM_VCPU_SVE in vcpu_init.features[0] for some vcpus and not for others. For one thing, this is useful for testing how the guest responds to that configuration. This will for example allow Linux's handling of these weird configurations to be tested directly rather than by hacking the guest kernel to fake it up -- which is what I've had to do up to now. > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Thanks! (Note, Marc already picked the patches into kvmarm/next, so new tags won't be applied. But the review is still appreciated, and if you spot any issues I'd certainly like to know :) Cheers ---Dave
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index a5d14b5..c86a7b0 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1927,6 +1927,12 @@ static void perform_access(struct kvm_vcpu *vcpu, { trace_kvm_sys_access(*vcpu_pc(vcpu), params, r); + /* Check for regs disabled by runtime config */ + if (sysreg_hidden_from_guest(vcpu, r)) { + kvm_inject_undefined(vcpu); + return; + } + /* * Not having an accessor means that we have configured a trap * that we don't know how to handle. This certainly qualifies @@ -2438,6 +2444,10 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg if (!r) return get_invariant_sys_reg(reg->id, uaddr); + /* Check for regs disabled by runtime config */ + if (sysreg_hidden_from_user(vcpu, r)) + return -ENOENT; + if (r->get_user) return (r->get_user)(vcpu, r, reg, uaddr); @@ -2459,6 +2469,10 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg if (!r) return set_invariant_sys_reg(reg->id, uaddr); + /* Check for regs disabled by runtime config */ + if (sysreg_hidden_from_user(vcpu, r)) + return -ENOENT; + if (r->set_user) return (r->set_user)(vcpu, r, reg, uaddr); @@ -2515,7 +2529,8 @@ static bool copy_reg_to_user(const struct sys_reg_desc *reg, u64 __user **uind) return true; } -static int walk_one_sys_reg(const struct sys_reg_desc *rd, +static int walk_one_sys_reg(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd, u64 __user **uind, unsigned int *total) { @@ -2526,6 +2541,9 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd, if (!(rd->reg || rd->get_user)) return 0; + if (sysreg_hidden_from_user(vcpu, rd)) + return 0; + if (!copy_reg_to_user(rd, uind)) return -EFAULT; @@ -2554,9 +2572,9 @@ static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind) int cmp = cmp_sys_reg(i1, i2); /* target-specific overrides generic entry. */ if (cmp <= 0) - err = walk_one_sys_reg(i1, &uind, &total); + err = walk_one_sys_reg(vcpu, i1, &uind, &total); else - err = walk_one_sys_reg(i2, &uind, &total); + err = walk_one_sys_reg(vcpu, i2, &uind, &total); if (err) return err; diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index 3b1bc7f..2be9950 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -64,8 +64,15 @@ struct sys_reg_desc { const struct kvm_one_reg *reg, void __user *uaddr); int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr); + + /* Return mask of REG_* runtime visibility overrides */ + unsigned int (*visibility)(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd); }; +#define REG_HIDDEN_USER (1 << 0) /* hidden from userspace ioctls */ +#define REG_HIDDEN_GUEST (1 << 1) /* hidden from guest */ + static inline void print_sys_reg_instr(const struct sys_reg_params *p) { /* Look, we even formatted it for you to paste into the table! */ @@ -102,6 +109,24 @@ static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r __vcpu_sys_reg(vcpu, r->reg) = r->val; } +static inline bool sysreg_hidden_from_guest(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + if (likely(!r->visibility)) + return false; + + return r->visibility(vcpu, r) & REG_HIDDEN_GUEST; +} + +static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + if (likely(!r->visibility)) + return false; + + return r->visibility(vcpu, r) & REG_HIDDEN_USER; +} + static inline int cmp_sys_reg(const struct sys_reg_desc *i1, const struct sys_reg_desc *i2) {