Hi Jing,
On Sun, Feb 26, 2023 at 7:05 PM Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> Hi Reiji,
>
> On Fri, Feb 24, 2023 at 8:00 PM Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> >
> > Hi Jing,
> >
> > On Sun, Feb 12, 2023 at 1:58 PM 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.
> > >
> > > 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, 144 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > > index 14ae03a1d8d0..15d0338742b6 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;
> > > +};
> > > +
> > > static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
> > > {
> > > if (kvm_vcpu_has_pmu(vcpu))
> > > @@ -329,21 +333,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, \
> > > + }, \
> > > }
> > >
> > > /*
> > > @@ -351,12 +359,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 \
> > > + }, \
> > > }
> > >
> > > /*
> > > @@ -364,15 +374,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[].
> > > @@ -382,9 +394,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),
> > > @@ -413,8 +429,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),
> > > @@ -424,8 +444,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),
> > > @@ -457,7 +481,13 @@ static const struct sys_reg_desc id_reg_descs[] = {
> > >
> > > const struct sys_reg_desc *kvm_arm_find_id_reg(const struct sys_reg_params *params)
> > > {
> > > - return find_reg(params, id_reg_descs, ARRAY_SIZE(id_reg_descs));
> > > + u32 id;
> > > +
> > > + id = reg_to_encoding(params);
> > > + if (!is_id_reg(id))
> > > + return NULL;
> > > +
> > > + return &id_reg_descs[IDREG_IDX(id)].reg_desc;
> > > }
> > >
> > > void kvm_arm_reset_id_regs(struct kvm_vcpu *vcpu)
> > > @@ -465,39 +495,106 @@ 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 = 0;
> >
> > When get_user is NULL, I wonder why you want to treat them RAZ.
> > It can be achieved by using visibility(), which I think might be
> > better to use before calling get_user.
> > Another option would be simply reading from IDREG(), which I would
> > guess might be useful(?) when no special handling is necessary.
> >
> Will simply use the value from IDREG().
> >
> > > + }
> > > +
> > > + if (!ret)
> > > + ret = put_user(val, uaddr);
> > > +
> > > + 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
> > > + ret = 0;
> >
> > This appears to be the same handling as WI.
> > How do you plan to use this set_user == NULL case ?
> > I don't think this shouldn't happen with the current code.
> > You might want to use WARN_ONCE here ?
> Yes, you are right. It won't happen with current code. WIll use WARN_ONCE here.
> >
> > > +
> > > + 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 (r->reg && !r->reset) {
> >
> > I don't think we need to check "!r->reset".
> > If r->reg is not NULL, I believe the entry must be incorrect.
> I am not sure I got your idea here? r->reg usually should not be NULL.
In general (not for ID registers), the r->reg is used as an index of sys_reg[].
In this patch, ID registers are not saved in sys_regs[], but in kvm_arch's
id_regs[]. So, I think the r->reg should be NULL for the ID registers.
(In fact, it is NULL for all ID registers in the current patch, right ?)
Or am I missing something ?
> >
> > > + kvm_err("sys_reg table %pS entry %d lacks reset\n", r, i);
> > > + return false;
> > > + }
> > > +
> > > + if (i && cmp_sys_reg(&id_reg_descs[i-1].reg_desc, r) >= 0) {
> >
> > In this table, each ID register needs to be in the proper place.
> > So, I would think what should be checked would be if each entry
> > in the table includes the right ID register.
> > (e.g. id_reg_descs[0] must be for ID_PFR0_EL1, etc)
> This comparison does the work, I think. It can detect any duplicate or
> non-ID entry, unless all entries are wrong and in the order.
Yes, I agree it does some work. There are some more cases that the
checking can't detect though (e.g. if one ID register is missing
from the table, I don't think the check can detect that).
I would prefer to adjust the checking specifically for this table
(i.e. Instead of checking the ordering, make sure that the entry
is for an ID register, and it is located in the right position).
What do you think ?
Thank you,
Reiji
