Hi Marc,
On Mon, Mar 27, 2023 at 4:28 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Fri, 17 Mar 2023 05:06:36 +0000,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > Introduce an ID feature register specific descriptor to include ID
> > register specific fields and callbacks besides its corresponding
> > general system register descriptor.
> > New fields for ID register descriptor would be added later when it
> > is necessary to support a writable ID register.
>
> What would these be? Could they make sense for "normal" sysregs as
> well?
As you know from the later patch, some fields are added only
applicable to idregs.
As you suggested, some of those fields may not be necessary, I'll try
to improve the data idregs specific data structures based on your
comments or even don't use a new structure if it is possible.
>
> >
> > No functional change intended.
> >
> > Co-developed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/id_regs.c | 187 +++++++++++++++++++++++++++-----------
> > arch/arm64/kvm/sys_regs.c | 2 +-
> > arch/arm64/kvm/sys_regs.h | 1 +
> > 3 files changed, 138 insertions(+), 52 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > index 3a87a3d2390d..9956c99d20f7 100644
> > --- a/arch/arm64/kvm/id_regs.c
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -18,6 +18,10 @@
> >
> > #include "sys_regs.h"
> >
> > +struct id_reg_desc {
> > + const struct sys_reg_desc reg_desc;
> > +};
> > +
>
> What is the advantage in having this wrapping structure that forces us
> to reinvent the wheel (the structure is different) over an additional
> pointer or even a side table?
As stated in the last comment, I'll try to improve the data structure
or don't use it at all if possible.
>
> > static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
> > {
> > if (kvm_vcpu_has_pmu(vcpu))
> > @@ -334,21 +338,25 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > }
> >
> > /* sys_reg_desc initialiser for known cpufeature ID registers */
> > -#define ID_SANITISED(name) { \
> > - SYS_DESC(SYS_##name), \
> > - .access = access_id_reg, \
> > - .get_user = get_id_reg, \
> > - .set_user = set_id_reg, \
> > - .visibility = id_visibility, \
> > +#define ID_SANITISED(name) { \
> > + .reg_desc = { \
> > + SYS_DESC(SYS_##name), \
> > + .access = access_id_reg, \
> > + .get_user = get_id_reg, \
> > + .set_user = set_id_reg, \
> > + .visibility = id_visibility, \
> > + }, \
> > }
> >
> > /* sys_reg_desc initialiser for known cpufeature ID registers */
> > -#define AA32_ID_SANITISED(name) { \
> > - SYS_DESC(SYS_##name), \
> > - .access = access_id_reg, \
> > - .get_user = get_id_reg, \
> > - .set_user = set_id_reg, \
> > - .visibility = aa32_id_visibility, \
> > +#define AA32_ID_SANITISED(name) { \
> > + .reg_desc = { \
> > + SYS_DESC(SYS_##name), \
> > + .access = access_id_reg, \
> > + .get_user = get_id_reg, \
> > + .set_user = set_id_reg, \
> > + .visibility = aa32_id_visibility, \
> > + }, \
> > }
> >
> > /*
> > @@ -356,12 +364,14 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
> > * (1 <= crm < 8, 0 <= Op2 < 8).
> > */
> > -#define ID_UNALLOCATED(crm, op2) { \
> > - Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \
> > - .access = access_id_reg, \
> > - .get_user = get_id_reg, \
> > - .set_user = set_id_reg, \
> > - .visibility = raz_visibility \
> > +#define ID_UNALLOCATED(crm, op2) { \
> > + .reg_desc = { \
> > + Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \
> > + .access = access_id_reg, \
> > + .get_user = get_id_reg, \
> > + .set_user = set_id_reg, \
> > + .visibility = raz_visibility \
> > + }, \
> > }
> >
> > /*
> > @@ -369,15 +379,17 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > * For now, these are exposed just like unallocated ID regs: they appear
> > * RAZ for the guest.
> > */
> > -#define ID_HIDDEN(name) { \
> > - SYS_DESC(SYS_##name), \
> > - .access = access_id_reg, \
> > - .get_user = get_id_reg, \
> > - .set_user = set_id_reg, \
> > - .visibility = raz_visibility, \
> > +#define ID_HIDDEN(name) { \
> > + .reg_desc = { \
> > + SYS_DESC(SYS_##name), \
> > + .access = access_id_reg, \
> > + .get_user = get_id_reg, \
> > + .set_user = set_id_reg, \
> > + .visibility = raz_visibility, \
> > + }, \
> > }
> >
> > -static const struct sys_reg_desc id_reg_descs[] = {
> > +static const struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
> > /*
> > * ID regs: all ID_SANITISED() entries here must have corresponding
> > * entries in arm64_ftr_regs[].
> > @@ -387,9 +399,13 @@ static const struct sys_reg_desc id_reg_descs[] = {
> > /* CRm=1 */
> > AA32_ID_SANITISED(ID_PFR0_EL1),
> > AA32_ID_SANITISED(ID_PFR1_EL1),
> > - { SYS_DESC(SYS_ID_DFR0_EL1), .access = access_id_reg,
> > - .get_user = get_id_reg, .set_user = set_id_dfr0_el1,
> > - .visibility = aa32_id_visibility, },
> > + { .reg_desc = {
> > + SYS_DESC(SYS_ID_DFR0_EL1),
> > + .access = access_id_reg,
> > + .get_user = get_id_reg,
> > + .set_user = set_id_dfr0_el1,
> > + .visibility = aa32_id_visibility, },
> > + },
> > ID_HIDDEN(ID_AFR0_EL1),
> > AA32_ID_SANITISED(ID_MMFR0_EL1),
> > AA32_ID_SANITISED(ID_MMFR1_EL1),
> > @@ -418,8 +434,12 @@ static const struct sys_reg_desc id_reg_descs[] = {
> >
> > /* AArch64 ID registers */
> > /* CRm=4 */
> > - { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg,
> > - .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, },
> > + { .reg_desc = {
> > + SYS_DESC(SYS_ID_AA64PFR0_EL1),
> > + .access = access_id_reg,
> > + .get_user = get_id_reg,
> > + .set_user = set_id_aa64pfr0_el1, },
> > + },
> > ID_SANITISED(ID_AA64PFR1_EL1),
> > ID_UNALLOCATED(4, 2),
> > ID_UNALLOCATED(4, 3),
> > @@ -429,8 +449,12 @@ static const struct sys_reg_desc id_reg_descs[] = {
> > ID_UNALLOCATED(4, 7),
> >
> > /* CRm=5 */
> > - { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
> > - .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
> > + { .reg_desc = {
> > + SYS_DESC(SYS_ID_AA64DFR0_EL1),
> > + .access = access_id_reg,
> > + .get_user = get_id_reg,
> > + .set_user = set_id_aa64dfr0_el1, },
> > + },
> > ID_SANITISED(ID_AA64DFR1_EL1),
> > ID_UNALLOCATED(5, 2),
> > ID_UNALLOCATED(5, 3),
> > @@ -469,12 +493,12 @@ static const struct sys_reg_desc id_reg_descs[] = {
> > */
> > int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
> > {
> > - const struct sys_reg_desc *r;
> > + u32 id;
> >
> > - r = find_reg(params, id_reg_descs, ARRAY_SIZE(id_reg_descs));
> > + id = reg_to_encoding(params);
> >
> > - if (likely(r)) {
> > - perform_access(vcpu, params, r);
> > + if (likely(is_id_reg(id))) {
> > + perform_access(vcpu, params, &id_reg_descs[IDREG_IDX(id)].reg_desc);
>
> How about minimising the diff and making the whole thing less verbose?
>
> static const struct sys_reg_desc *id_to_id_reg_desc(struct sys_reg_params *params)
> {
> u32 id;
>
> id = reg_to_encoding(params);
> if (is_id_reg(id))
> return &id_reg_descs[IDREG_IDX(id)].reg_desc;
>
> return NULL;
> }
>
> int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
> {
> const struct sys_reg_desc *r;
>
> r = id_to_id_reg_desc(params);
> [...]
> }
>
> And use the helper everywhere?
Sure, will do.
>
> > } else {
> > print_sys_reg_msg(params,
> > "Unsupported guest id_reg access at: %lx [%08lx]\n",
> > @@ -491,38 +515,102 @@ 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]);
> > + if (id_reg_descs[i].reg_desc.reset)
> > + id_reg_descs[i].reg_desc.reset(vcpu, &id_reg_descs[i].reg_desc);
> > }
> >
> > 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));
> > + u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> > + const struct sys_reg_desc *r;
> > + struct sys_reg_params params;
> > + u64 val;
> > + int ret;
> > + u32 id;
> > +
> > + if (!index_to_params(reg->id, ¶ms))
> > + return -ENOENT;
> > + id = reg_to_encoding(¶ms);
> > +
> > + if (!is_id_reg(id))
> > + return -ENOENT;
> > +
> > + r = &id_reg_descs[IDREG_IDX(id)].reg_desc;
> > + if (r->get_user) {
> > + ret = (r->get_user)(vcpu, r, &val);
> > + } else {
> > + ret = 0;
> > + val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)];
> > + }
> > +
> > + if (!ret)
> > + ret = put_user(val, uaddr);
>
> How about the visibility? Why isn't it checked?
The visibility check is done in get_user()->get_id_reg()->read_id_reg().
>
> > +
> > + return ret;
> > }
> >
> > 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));
> > + u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> > + const struct sys_reg_desc *r;
> > + struct sys_reg_params params;
> > + u64 val;
> > + int ret;
> > + u32 id;
> > +
> > + if (!index_to_params(reg->id, ¶ms))
> > + return -ENOENT;
> > + id = reg_to_encoding(¶ms);
> > +
> > + if (!is_id_reg(id))
> > + return -ENOENT;
> > +
> > + if (get_user(val, uaddr))
> > + return -EFAULT;
> > +
> > + r = &id_reg_descs[IDREG_IDX(id)].reg_desc;
> > +
> > + if (sysreg_user_write_ignore(vcpu, r))
> > + return 0;
> > +
> > + if (r->set_user) {
> > + ret = (r->set_user)(vcpu, r, val);
> > + } else {
> > + WARN_ONCE(1, "ID register set_user callback is NULL\n");
>
> Why the shouting? We didn't do that before. What's changed?
It was added according to one of Reiji's comments from
https://lore.kernel.org/all/CAAeT=Fz-G_EUmh=Pj3UHA7pnKKYi7UyYuedziJxfmSoKpntw3Q@xxxxxxxxxxxxxx.
WDYT?
>
> > + ret = 0;
> > + }
> > +
> > + return ret;
> > }
> >
> > bool kvm_arm_check_idreg_table(void)
> > {
> > - return check_sysreg_table(id_reg_descs, ARRAY_SIZE(id_reg_descs), false);
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> > + const struct sys_reg_desc *r = &id_reg_descs[i].reg_desc;
> > +
> > + if (!is_id_reg(reg_to_encoding(r))) {
> > + kvm_err("id_reg table %pS entry %d not set correctly\n",
> > + &id_reg_descs[i].reg_desc, i);
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > }
> >
> > int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> > {
> > - const struct sys_reg_desc *i2, *end2;
> > + const struct id_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);
> > + for (; i2 != end2; i2++) {
> > + err = walk_one_sys_reg(vcpu, &(i2->reg_desc), &uind, &total);
> > if (err)
> > return err;
> > }
> > @@ -540,12 +628,9 @@ void kvm_arm_set_default_id_regs(struct kvm *kvm)
> > u64 val;
> >
> > for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> > - id = reg_to_encoding(&id_reg_descs[i]);
> > - if (WARN_ON_ONCE(!is_id_reg(id)))
> > - /* Shouldn't happen */
> > - continue;
> > + id = reg_to_encoding(&id_reg_descs[i].reg_desc);
>
> Why have you dropped that check? If it shouldn't happen before, it
> still shouldn't happen.
Since the check was done in kvm_arm_check_idreg_table() now.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
