| Message ID | 1550519559-15915-13-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
On 18/02/2019 19:52, 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 restrictions() that can be used to hook in any needed > runtime visibility checks. This method can currently return > REG_NO_USER to inhibit enumeration and ioctl access to the register > for userspace, and REG_NO_GUEST to inhibit runtime access by the > guest using MSR/MRS. > > This 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> Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > --- > > Changes since v4: > > * Move from a boolean sysreg property that just suppresses register > enumeration via KVM_GET_REG_LIST, to a multi-flag property that > allows independent runtime control of MRS/MSR and user ioctl access. > > This allows registers to be either hidden completely, or to have > hybrid behaviours (such as the not-enumerated, RAZ, WAZ behaviour of > "non-present" CPU ID regs). > --- > arch/arm64/kvm/sys_regs.c | 24 +++++++++++++++++++++--- > arch/arm64/kvm/sys_regs.h | 13 +++++++++++++ > 2 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 71c5825..3f1243e 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1863,6 +1863,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 (restrictions(vcpu, r) & REG_NO_GUEST) { > + 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 > @@ -2370,6 +2376,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 (restrictions(vcpu, r) & REG_NO_USER) > + return -ENOENT; > + > if (r->get_user) > return (r->get_user)(vcpu, r, reg, uaddr); > > @@ -2391,6 +2401,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 (restrictions(vcpu, r) & REG_NO_USER) > + return -ENOENT; > + > if (r->set_user) > return (r->set_user)(vcpu, r, reg, uaddr); > > @@ -2447,7 +2461,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) > { > @@ -2458,6 +2473,9 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd, > if (!(rd->reg || rd->get_user)) > return 0; > > + if (restrictions(vcpu, rd) & REG_NO_USER) > + return 0; > + > if (!copy_reg_to_user(rd, uind)) > return -EFAULT; > > @@ -2486,9 +2504,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 174ffc0..12f196f 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -69,8 +69,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 restriction flags */ > + unsigned int (*restrictions)(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd); > }; > > +#define REG_NO_USER (1 << 0) /* hidden from userspace ioctl interface */ > +#define REG_NO_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! */ > @@ -109,6 +116,12 @@ 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 unsigned int restrictions(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + return unlikely(r->restrictions) ? r->restrictions(vcpu, r) : 0; > +} > + > static inline int cmp_sys_reg(const struct sys_reg_desc *i1, > const struct sys_reg_desc *i2) > { >
On Mon, Feb 18, 2019 at 07:52:25PM +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 restrictions() that can be used to hook in any needed > runtime visibility checks. This method can currently return > REG_NO_USER to inhibit enumeration and ioctl access to the register > for userspace, and REG_NO_GUEST to inhibit runtime access by the > guest using MSR/MRS. > > This 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> > > --- > > Changes since v4: > > * Move from a boolean sysreg property that just suppresses register > enumeration via KVM_GET_REG_LIST, to a multi-flag property that > allows independent runtime control of MRS/MSR and user ioctl access. > > This allows registers to be either hidden completely, or to have > hybrid behaviours (such as the not-enumerated, RAZ, WAZ behaviour of > "non-present" CPU ID regs). Sorry for bikeshedding... > + /* Check for regs disabled by runtime config */ > + if (restrictions(vcpu, r) & REG_NO_GUEST) { Maybe it's worth wrapping this as something like reg_runtime_hidden_from_guest(vcpu, r) ... and avoid exposing the raw flags to all the places we have to check? [...] > +#define REG_NO_USER (1 << 0) /* hidden from userspace ioctl interface */ > +#define REG_NO_GUEST (1 << 1) /* hidden from guest */ Perhaps REG_USER_HIDDEN and REG_GUEST_HIDDEN? Thanks, Mark.
On Wed, Feb 20, 2019 at 02:33:44PM +0000, Julien Thierry wrote: > > > On 18/02/2019 19:52, 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 restrictions() that can be used to hook in any needed > > runtime visibility checks. This method can currently return > > REG_NO_USER to inhibit enumeration and ioctl access to the register > > for userspace, and REG_NO_GUEST to inhibit runtime access by the > > guest using MSR/MRS. > > > > This 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> > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> Thanks ---Dave
On Wed, Feb 20, 2019 at 03:37:26PM +0000, Mark Rutland wrote: > On Mon, Feb 18, 2019 at 07:52:25PM +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 restrictions() that can be used to hook in any needed > > runtime visibility checks. This method can currently return > > REG_NO_USER to inhibit enumeration and ioctl access to the register > > for userspace, and REG_NO_GUEST to inhibit runtime access by the > > guest using MSR/MRS. > > > > This 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> > > > > --- > > > > Changes since v4: > > > > * Move from a boolean sysreg property that just suppresses register > > enumeration via KVM_GET_REG_LIST, to a multi-flag property that > > allows independent runtime control of MRS/MSR and user ioctl access. > > > > This allows registers to be either hidden completely, or to have > > hybrid behaviours (such as the not-enumerated, RAZ, WAZ behaviour of > > "non-present" CPU ID regs). > > Sorry for bikeshedding... > > > + /* Check for regs disabled by runtime config */ > > + if (restrictions(vcpu, r) & REG_NO_GUEST) { > > Maybe it's worth wrapping this as something like > > reg_runtime_hidden_from_guest(vcpu, r) > > ... and avoid exposing the raw flags to all the places we have to check? > > [...] > > > +#define REG_NO_USER (1 << 0) /* hidden from userspace ioctl interface */ > > +#define REG_NO_GUEST (1 << 1) /* hidden from guest */ > > Perhaps REG_USER_HIDDEN and REG_GUEST_HIDDEN? I'm not attached to any particular naming, so I'm not opposed to making changes similar to those you suggest. There are some anomalies right now: 1) Currently, we can express REG_NO_GUEST by itself, which is a of an odd thing to have. I'm not sure whether that's a problem or not. Keeping the flags as-is at least keeps the code simple. 2) These flags do not quite have the obvious semantics: these are overrides rather than determining precisely when a reg is/isn't accessible. So, REG_NO_USER means "don't even call this reg's get/set_user(): forbid user access unconditionally", whereas lack of this flag means "call the appropriate get/set_user() function to find out what to do, which may or may not result in forbidding the access". Maybe this subtlety is just a question of clear commenting. I can't think of obviously-correct names that won't be stupidly verbose... Thoughts? Cheers ---Dave
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 71c5825..3f1243e 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1863,6 +1863,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 (restrictions(vcpu, r) & REG_NO_GUEST) { + 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 @@ -2370,6 +2376,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 (restrictions(vcpu, r) & REG_NO_USER) + return -ENOENT; + if (r->get_user) return (r->get_user)(vcpu, r, reg, uaddr); @@ -2391,6 +2401,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 (restrictions(vcpu, r) & REG_NO_USER) + return -ENOENT; + if (r->set_user) return (r->set_user)(vcpu, r, reg, uaddr); @@ -2447,7 +2461,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) { @@ -2458,6 +2473,9 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd, if (!(rd->reg || rd->get_user)) return 0; + if (restrictions(vcpu, rd) & REG_NO_USER) + return 0; + if (!copy_reg_to_user(rd, uind)) return -EFAULT; @@ -2486,9 +2504,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 174ffc0..12f196f 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -69,8 +69,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 restriction flags */ + unsigned int (*restrictions)(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd); }; +#define REG_NO_USER (1 << 0) /* hidden from userspace ioctl interface */ +#define REG_NO_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! */ @@ -109,6 +116,12 @@ 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 unsigned int restrictions(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + return unlikely(r->restrictions) ? r->restrictions(vcpu, r) : 0; +} + static inline int cmp_sys_reg(const struct sys_reg_desc *i1, const struct sys_reg_desc *i2) {
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 restrictions() that can be used to hook in any needed runtime visibility checks. This method can currently return REG_NO_USER to inhibit enumeration and ioctl access to the register for userspace, and REG_NO_GUEST to inhibit runtime access by the guest using MSR/MRS. This 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> --- Changes since v4: * Move from a boolean sysreg property that just suppresses register enumeration via KVM_GET_REG_LIST, to a multi-flag property that allows independent runtime control of MRS/MSR and user ioctl access. This allows registers to be either hidden completely, or to have hybrid behaviours (such as the not-enumerated, RAZ, WAZ behaviour of "non-present" CPU ID regs). --- arch/arm64/kvm/sys_regs.c | 24 +++++++++++++++++++++--- arch/arm64/kvm/sys_regs.h | 13 +++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-)