| Message ID | 20220706164304.1582687-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic-v3 userspace access consolidation (and other goodies) | expand |
Hi Marc, On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@kernel.org> wrote: > > find_reg_by_id() requires a sys_reg_param as input, which most > users provide as a on-stack variable, but don't make any use of > the result. > > Provide a helper that doesn't have this requirement and simplify > the callers (all but one). > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 28 +++++++++++++++++----------- > arch/arm64/kvm/sys_regs.h | 4 ++++ > arch/arm64/kvm/vgic-sys-reg-v3.c | 8 ++------ > 3 files changed, 23 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index c06c0477fab5..1f410283c592 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -2650,21 +2650,29 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, > return find_reg(params, table, num); > } > > +const struct sys_reg_desc *get_reg_by_id(u64 id, > + const struct sys_reg_desc table[], > + unsigned int num) > +{ > + struct sys_reg_params params; > + > + if (!index_to_params(id, ¶ms)) > + return NULL; > + > + return find_reg(¶ms, table, num); > +} Since all the callers of get_reg_id() specify ARRAY_SIZE(array) for the 3rd argument, and I think future callers of it are also likely to do the same, perhaps it might be more convenient if we make get_reg_id() a wrapper macro like below (and rename "get_reg_by_id()" to "__get_reg_by_id()") ? #define get_reg_id(id, table) __get_reg_id(id, table, ARRAY_SIZE(table)) Thanks, Reiji > + > /* Decode an index value, and find the sys_reg_desc entry. */ > static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu, > u64 id) > { > const struct sys_reg_desc *r; > - struct sys_reg_params params; > > /* We only do sys_reg for now. */ > if ((id & KVM_REG_ARM_COPROC_MASK) != KVM_REG_ARM64_SYSREG) > return NULL; > > - if (!index_to_params(id, ¶ms)) > - return NULL; > - > - r = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > + r = get_reg_by_id(id, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > > /* Not saved in the sys_reg array and not otherwise accessible? */ > if (r && !(r->reg || r->get_user)) > @@ -2723,11 +2731,10 @@ static int reg_to_user(void __user *uaddr, const u64 *val, u64 id) > > static int get_invariant_sys_reg(u64 id, void __user *uaddr) > { > - struct sys_reg_params params; > const struct sys_reg_desc *r; > > - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, > - ARRAY_SIZE(invariant_sys_regs)); > + r = get_reg_by_id(id, invariant_sys_regs, > + ARRAY_SIZE(invariant_sys_regs)); > if (!r) > return -ENOENT; > > @@ -2736,13 +2743,12 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr) > > static int set_invariant_sys_reg(u64 id, void __user *uaddr) > { > - struct sys_reg_params params; > const struct sys_reg_desc *r; > int err; > u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */ > > - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, > - ARRAY_SIZE(invariant_sys_regs)); > + r = get_reg_by_id(id, invariant_sys_regs, > + ARRAY_SIZE(invariant_sys_regs)); > if (!r) > return -ENOENT; > > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index aee8ea054f0d..ce30ed9566ae 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -195,6 +195,10 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, > const struct sys_reg_desc table[], > unsigned int num); > > +const struct sys_reg_desc *get_reg_by_id(u64 id, > + const struct sys_reg_desc table[], > + unsigned int num); > + > #define AA32(_x) .aarch32_map = AA32_##_x > #define Op0(_x) .Op0 = _x > #define Op1(_x) .Op1 = _x > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c > index 07d5271e9f05..644acda33c7c 100644 > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c > @@ -263,14 +263,10 @@ static const struct sys_reg_desc gic_v3_icc_reg_descs[] = { > int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id, > u64 *reg) > { > - struct sys_reg_params params; > u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64; > > - params.regval = *reg; > - params.is_write = is_write; > - > - if (find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs, > - ARRAY_SIZE(gic_v3_icc_reg_descs))) > + if (get_reg_by_id(sysreg, gic_v3_icc_reg_descs, > + ARRAY_SIZE(gic_v3_icc_reg_descs))) > return 0; > > return -ENXIO; > -- > 2.34.1 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Wed, Jul 6, 2022 at 9:05 PM Reiji Watanabe <reijiw@google.com> wrote: > > Hi Marc, > > On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@kernel.org> wrote: > > > > find_reg_by_id() requires a sys_reg_param as input, which most > > users provide as a on-stack variable, but don't make any use of > > the result. > > > > Provide a helper that doesn't have this requirement and simplify > > the callers (all but one). > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/sys_regs.c | 28 +++++++++++++++++----------- > > arch/arm64/kvm/sys_regs.h | 4 ++++ > > arch/arm64/kvm/vgic-sys-reg-v3.c | 8 ++------ > > 3 files changed, 23 insertions(+), 17 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index c06c0477fab5..1f410283c592 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -2650,21 +2650,29 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, > > return find_reg(params, table, num); > > } > > > > +const struct sys_reg_desc *get_reg_by_id(u64 id, > > + const struct sys_reg_desc table[], > > + unsigned int num) > > +{ > > + struct sys_reg_params params; > > + > > + if (!index_to_params(id, ¶ms)) > > + return NULL; > > + > > + return find_reg(¶ms, table, num); > > +} > > Since all the callers of get_reg_id() specify ARRAY_SIZE(array) for > the 3rd argument, and I think future callers of it are also likely to > do the same, perhaps it might be more convenient if we make get_reg_id() > a wrapper macro like below (and rename "get_reg_by_id()" to > "__get_reg_by_id()") ? > > #define get_reg_id(id, table) __get_reg_id(id, table, ARRAY_SIZE(table)) Looking at the following patches, let me withdraw this comment... Instead, Reviewed-by: Reiji Watanabe <reijiw@google.com> Thank you, Reiji > > > + > > /* Decode an index value, and find the sys_reg_desc entry. */ > > static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu, > > u64 id) > > { > > const struct sys_reg_desc *r; > > - struct sys_reg_params params; > > > > /* We only do sys_reg for now. */ > > if ((id & KVM_REG_ARM_COPROC_MASK) != KVM_REG_ARM64_SYSREG) > > return NULL; > > > > - if (!index_to_params(id, ¶ms)) > > - return NULL; > > - > > - r = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > > + r = get_reg_by_id(id, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > > > > /* Not saved in the sys_reg array and not otherwise accessible? */ > > if (r && !(r->reg || r->get_user)) > > @@ -2723,11 +2731,10 @@ static int reg_to_user(void __user *uaddr, const u64 *val, u64 id) > > > > static int get_invariant_sys_reg(u64 id, void __user *uaddr) > > { > > - struct sys_reg_params params; > > const struct sys_reg_desc *r; > > > > - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, > > - ARRAY_SIZE(invariant_sys_regs)); > > + r = get_reg_by_id(id, invariant_sys_regs, > > + ARRAY_SIZE(invariant_sys_regs)); > > if (!r) > > return -ENOENT; > > > > @@ -2736,13 +2743,12 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr) > > > > static int set_invariant_sys_reg(u64 id, void __user *uaddr) > > { > > - struct sys_reg_params params; > > const struct sys_reg_desc *r; > > int err; > > u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */ > > > > - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, > > - ARRAY_SIZE(invariant_sys_regs)); > > + r = get_reg_by_id(id, invariant_sys_regs, > > + ARRAY_SIZE(invariant_sys_regs)); > > if (!r) > > return -ENOENT; > > > > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > > index aee8ea054f0d..ce30ed9566ae 100644 > > --- a/arch/arm64/kvm/sys_regs.h > > +++ b/arch/arm64/kvm/sys_regs.h > > @@ -195,6 +195,10 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, > > const struct sys_reg_desc table[], > > unsigned int num); > > > > +const struct sys_reg_desc *get_reg_by_id(u64 id, > > + const struct sys_reg_desc table[], > > + unsigned int num); > > + > > #define AA32(_x) .aarch32_map = AA32_##_x > > #define Op0(_x) .Op0 = _x > > #define Op1(_x) .Op1 = _x > > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c > > index 07d5271e9f05..644acda33c7c 100644 > > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c > > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c > > @@ -263,14 +263,10 @@ static const struct sys_reg_desc gic_v3_icc_reg_descs[] = { > > int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id, > > u64 *reg) > > { > > - struct sys_reg_params params; > > u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64; > > > > - params.regval = *reg; > > - params.is_write = is_write; > > - > > - if (find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs, > > - ARRAY_SIZE(gic_v3_icc_reg_descs))) > > + if (get_reg_by_id(sysreg, gic_v3_icc_reg_descs, > > + ARRAY_SIZE(gic_v3_icc_reg_descs))) > > return 0; > > > > return -ENXIO; > > -- > > 2.34.1 > > > > _______________________________________________ > > kvmarm mailing list > > kvmarm@lists.cs.columbia.edu > > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c06c0477fab5..1f410283c592 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2650,21 +2650,29 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, return find_reg(params, table, num); } +const struct sys_reg_desc *get_reg_by_id(u64 id, + const struct sys_reg_desc table[], + unsigned int num) +{ + struct sys_reg_params params; + + if (!index_to_params(id, ¶ms)) + return NULL; + + return find_reg(¶ms, table, num); +} + /* Decode an index value, and find the sys_reg_desc entry. */ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id) { const struct sys_reg_desc *r; - struct sys_reg_params params; /* We only do sys_reg for now. */ if ((id & KVM_REG_ARM_COPROC_MASK) != KVM_REG_ARM64_SYSREG) return NULL; - if (!index_to_params(id, ¶ms)) - return NULL; - - r = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); + r = get_reg_by_id(id, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); /* Not saved in the sys_reg array and not otherwise accessible? */ if (r && !(r->reg || r->get_user)) @@ -2723,11 +2731,10 @@ static int reg_to_user(void __user *uaddr, const u64 *val, u64 id) static int get_invariant_sys_reg(u64 id, void __user *uaddr) { - struct sys_reg_params params; const struct sys_reg_desc *r; - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, - ARRAY_SIZE(invariant_sys_regs)); + r = get_reg_by_id(id, invariant_sys_regs, + ARRAY_SIZE(invariant_sys_regs)); if (!r) return -ENOENT; @@ -2736,13 +2743,12 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr) static int set_invariant_sys_reg(u64 id, void __user *uaddr) { - struct sys_reg_params params; const struct sys_reg_desc *r; int err; u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */ - r = find_reg_by_id(id, ¶ms, invariant_sys_regs, - ARRAY_SIZE(invariant_sys_regs)); + r = get_reg_by_id(id, invariant_sys_regs, + ARRAY_SIZE(invariant_sys_regs)); if (!r) return -ENOENT; diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index aee8ea054f0d..ce30ed9566ae 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -195,6 +195,10 @@ const struct sys_reg_desc *find_reg_by_id(u64 id, const struct sys_reg_desc table[], unsigned int num); +const struct sys_reg_desc *get_reg_by_id(u64 id, + const struct sys_reg_desc table[], + unsigned int num); + #define AA32(_x) .aarch32_map = AA32_##_x #define Op0(_x) .Op0 = _x #define Op1(_x) .Op1 = _x diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c index 07d5271e9f05..644acda33c7c 100644 --- a/arch/arm64/kvm/vgic-sys-reg-v3.c +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c @@ -263,14 +263,10 @@ static const struct sys_reg_desc gic_v3_icc_reg_descs[] = { int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id, u64 *reg) { - struct sys_reg_params params; u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64; - params.regval = *reg; - params.is_write = is_write; - - if (find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs, - ARRAY_SIZE(gic_v3_icc_reg_descs))) + if (get_reg_by_id(sysreg, gic_v3_icc_reg_descs, + ARRAY_SIZE(gic_v3_icc_reg_descs))) return 0; return -ENXIO;
find_reg_by_id() requires a sys_reg_param as input, which most users provide as a on-stack variable, but don't make any use of the result. Provide a helper that doesn't have this requirement and simplify the callers (all but one). Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/sys_regs.c | 28 +++++++++++++++++----------- arch/arm64/kvm/sys_regs.h | 4 ++++ arch/arm64/kvm/vgic-sys-reg-v3.c | 8 ++------ 3 files changed, 23 insertions(+), 17 deletions(-)