Hi Marc,
On Mon, Mar 27, 2023 at 3:14 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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.
>
You are right. It actually does nothing for idregs which have no reset function.
Will remove this.
> > +
> > +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?
>
As you know from the later patches in this series, a per VM idregs
array and an idregs specific structure are used. All these functions
would be implemented based on that.
> > +
> > +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.
The reason for not using common code is the same as last comment. An
idregs specific data structure would be used.
>
> [...]
>
> > @@ -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.
Sure, will move the encoding check here.
>
> > + 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.
Agreed.
>
> > + 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.
>
Thanks, will fix this.
> > +
> > +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.
Sure, will do.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
