On Fri, 17 Mar 2023 05:06:32 +0000,
Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> Create a new file id_regs.c for CPU ID feature registers emulation code,
> which are moved from sys_regs.c and tweak sys_regs code accordingly.
>
> No functional change intended.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/id_regs.c | 506 ++++++++++++++++++++++++++++++++++++++
> arch/arm64/kvm/sys_regs.c | 464 ++--------------------------------
> arch/arm64/kvm/sys_regs.h | 41 +++
> 4 files changed, 575 insertions(+), 438 deletions(-)
> create mode 100644 arch/arm64/kvm/id_regs.c
>
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index c0c050e53157..a6a315fcd81e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -13,7 +13,7 @@ obj-$(CONFIG_KVM) += hyp/
> kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> inject_fault.o va_layout.o handle_exit.o \
> guest.o debug.o reset.o sys_regs.o stacktrace.o \
> - vgic-sys-reg-v3.o fpsimd.o pkvm.o \
> + vgic-sys-reg-v3.o fpsimd.o pkvm.o id_regs.o \
> arch_timer.o trng.o vmid.o emulate-nested.o nested.o \
> vgic/vgic.o vgic/vgic-init.o \
> vgic/vgic-irqfd.o vgic/vgic-v2.o \
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> new file mode 100644
> index 000000000000..08b738852955
> --- /dev/null
> +++ b/arch/arm64/kvm/id_regs.c
[...]
> +/**
> + * emulate_id_reg - Emulate a guest access to an AArch64 CPU ID feature register
> + * @vcpu: The VCPU pointer
> + * @params: Decoded system register parameters
> + *
> + * Return: true if the ID register access was successful, false otherwise.
> + */
> +int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
> +{
> + const struct sys_reg_desc *r;
> +
> + r = find_reg(params, id_reg_descs, ARRAY_SIZE(id_reg_descs));
> +
> + if (likely(r)) {
> + perform_access(vcpu, params, r);
> + } else {
> + print_sys_reg_msg(params,
> + "Unsupported guest id_reg access at: %lx [%08lx]\n",
> + *vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
> + kvm_inject_undefined(vcpu);
> + }
> +
> + return 1;
> +}
> +
> +
> +void kvm_arm_reset_id_regs(struct kvm_vcpu *vcpu)
> +{
> + unsigned long i;
> +
> + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++)
> + if (id_reg_descs[i].reset)
> + id_reg_descs[i].reset(vcpu, &id_reg_descs[i]);
> +}
What does this mean? None of the idregs have a reset function, given
that they are global. Maybe this will make sense in the following
patches, but definitely not here.
> +
> +int kvm_arm_get_id_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> + return kvm_sys_reg_get_user(vcpu, reg,
> + id_reg_descs, ARRAY_SIZE(id_reg_descs));
> +}
> +
> +int kvm_arm_set_id_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> +{
> + return kvm_sys_reg_set_user(vcpu, reg,
> + id_reg_descs, ARRAY_SIZE(id_reg_descs));
> +}
> +
> +bool kvm_arm_check_idreg_table(void)
> +{
> + return check_sysreg_table(id_reg_descs, ARRAY_SIZE(id_reg_descs), false);
> +}
All these helpers are called from sys_regs.c and directly call back
into it. Why not simply have a helper that gets the base and size of
the array, and stick to pure common code?
> +
> +int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> +{
> + const struct sys_reg_desc *i2, *end2;
> + unsigned int total = 0;
> + int err;
> +
> + i2 = id_reg_descs;
> + end2 = id_reg_descs + ARRAY_SIZE(id_reg_descs);
> +
> + while (i2 != end2) {
> + err = walk_one_sys_reg(vcpu, i2++, &uind, &total);
> + if (err)
> + return err;
> + }
> + return total;
> +}
This is an exact copy of walk_sys_regs. Surely this can be made common
code.
[...]
> @@ -2912,6 +2482,8 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> {
> unsigned long i;
>
> + kvm_arm_reset_id_regs(vcpu);
> +
> for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++)
> if (sys_reg_descs[i].reset)
> sys_reg_descs[i].reset(vcpu, &sys_reg_descs[i]);
> @@ -2932,6 +2504,9 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
> params = esr_sys64_to_params(esr);
> params.regval = vcpu_get_reg(vcpu, Rt);
>
> + if (is_id_reg(reg_to_encoding(¶ms)))
> + return emulate_id_reg(vcpu, ¶ms);
> +
> if (!emulate_sys_reg(vcpu, ¶ms))
> return 1;
>
> @@ -3160,6 +2735,10 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
> if (err != -ENOENT)
> return err;
>
> + err = kvm_arm_get_id_reg(vcpu, reg);
Why not check for the encoding here? or in the helpers? It feels that
this is an overhead that would be easy to reduce, given that we have
fewer idregs than normal sysregs.
> + if (err != -ENOENT)
> + return err;
> +
> return kvm_sys_reg_get_user(vcpu, reg,
> sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> }
> @@ -3204,6 +2783,10 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
> if (err != -ENOENT)
> return err;
>
> + err = kvm_arm_set_id_reg(vcpu, reg);
Same here.
> + if (err != -ENOENT)
> + return err;
> +
> return kvm_sys_reg_set_user(vcpu, reg,
> sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> }
> @@ -3250,10 +2833,10 @@ 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 kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd,
> - u64 __user **uind,
> - unsigned int *total)
> +int walk_one_sys_reg(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd,
> + u64 __user **uind,
> + unsigned int *total)
> {
> /*
> * Ignore registers we trap but don't save,
> @@ -3294,6 +2877,7 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
> {
> return ARRAY_SIZE(invariant_sys_regs)
> + num_demux_regs()
> + + kvm_arm_walk_id_regs(vcpu, (u64 __user *)NULL)
> + walk_sys_regs(vcpu, (u64 __user *)NULL);
> }
>
> @@ -3309,6 +2893,11 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> uindices++;
> }
>
> + err = kvm_arm_walk_id_regs(vcpu, uindices);
> + if (err < 0)
> + return err;
> + uindices += err;
> +
> err = walk_sys_regs(vcpu, uindices);
> if (err < 0)
> return err;
> @@ -3323,6 +2912,7 @@ int __init kvm_sys_reg_table_init(void)
> unsigned int i;
>
> /* Make sure tables are unique and in order. */
> + valid &= kvm_arm_check_idreg_table();
> valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false);
> valid &= check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs), true);
> valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), true);
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 6b11f2cc7146..ad41305348f7 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -210,6 +210,35 @@ find_reg(const struct sys_reg_params *params, const struct sys_reg_desc table[],
> return __inline_bsearch((void *)pval, table, num, sizeof(table[0]), match_sys_reg);
> }
>
> +static inline unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *r)
> +{
> + return REG_RAZ;
> +}
No, please. This is used as a function pointer. You now potentially
force the compiler to emit as many copy of this as there are pointers.
> +
> +static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params,
> + const struct sys_reg_desc *r)
> +{
> + WARN_ONCE(1, "Unexpected sys_reg write to read-only register\n");
> + print_sys_reg_instr(params);
> + kvm_inject_undefined(vcpu);
> + return false;
> +}
Please make this common code, and not an inline function.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
