Re: [PATCH v6 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Reiji,

On Tue, Apr 18, 2023 at 9:09 PM Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> Hi Jing,
>
> On Tue, Apr 04, 2023 at 03:53:43AM +0000, Jing Zhang wrote:
> > Since reset() and val are not used for idreg in sys_reg_desc, they would
> > be used with other purposes for idregs.
> > The callback reset() would be used to return KVM sanitised id register
> > values. The u64 val would be used as mask for writable fields in idregs.
> > Only bits with 1 in val are writable from userspace.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> >  arch/arm64/kvm/id_regs.c  | 44 +++++++++++++++++++----------
> >  arch/arm64/kvm/sys_regs.c | 59 +++++++++++++++++++++++++++------------
> >  arch/arm64/kvm/sys_regs.h | 10 ++++---
> >  3 files changed, 77 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > index 6f65d30693fe..fe37b6786b4c 100644
> > --- a/arch/arm64/kvm/id_regs.c
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -55,6 +55,11 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> >       }
> >  }
> >
> > +static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> > +{
> > +     return read_sanitised_ftr_reg(reg_to_encoding(rd));
> > +}
> > +
> >  u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> >  {
> >       u64 val = IDREG(vcpu->kvm, id);
> > @@ -324,6 +329,17 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >       return 0;
> >  }
> >
> > +/*
> > + * Since reset() callback and field val are not used for idregs, they will be
> > + * used for specific purposes for idregs.
> > + * The reset() would return KVM sanitised register value. The value would be the
> > + * same as the host kernel sanitised value if there is no KVM sanitisation.
> > + * The val would be used as a mask indicating writable fields for the idreg.
> > + * Only bits with 1 are writable from userspace. This mask might not be
>
> Nit: This comment update seems to be in the next patch,
> since 'val' for AA64PFR0, AA64DFR0 and DFR0 is zero yet.
Even the val is all zero in this commit, but it is used the first time
here. I guess it is okay to have the comment here.
>
>
> > + * necessary in the future whenever all ID registers are enabled as writable
> > + * from userspace.
> > + */
> > +
> >  /* sys_reg_desc initialiser for known cpufeature ID registers */
> >  #define ID_SANITISED(name) {                 \
> >       SYS_DESC(SYS_##name),                   \
> > @@ -331,6 +347,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >       .get_user = get_id_reg,                 \
> >       .set_user = set_id_reg,                 \
> >       .visibility = id_visibility,            \
> > +     .reset = general_read_kvm_sanitised_reg,\
> > +     .val = 0,                               \
> >  }
> >
> >  /* sys_reg_desc initialiser for known cpufeature ID registers */
> > @@ -340,6 +358,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >       .get_user = get_id_reg,                 \
> >       .set_user = set_id_reg,                 \
> >       .visibility = aa32_id_visibility,       \
> > +     .reset = general_read_kvm_sanitised_reg,\
> > +     .val = 0,                               \
> >  }
> >
> >  /*
> > @@ -352,7 +372,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >       .access = access_id_reg,                        \
> >       .get_user = get_id_reg,                         \
> >       .set_user = set_id_reg,                         \
> > -     .visibility = raz_visibility                    \
> > +     .visibility = raz_visibility,                   \
> > +     .reset = NULL,                                  \
> > +     .val = 0,                                       \
> >  }
> >
> >  /*
> > @@ -366,6 +388,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >       .get_user = get_id_reg,                 \
> >       .set_user = set_id_reg,                 \
> >       .visibility = raz_visibility,           \
> > +     .reset = NULL,                          \
> > +     .val = 0,                               \
> >  }
> >
> >  const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
> > @@ -476,10 +500,7 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
> >       return 1;
> >  }
> >
> > -/*
> > - * Set the guest's ID registers that are defined in id_reg_descs[]
> > - * with ID_SANITISED() to the host's sanitized value.
> > - */
> > +/* Initialize the guest's ID registers with KVM sanitised values. */
> >  void kvm_arm_init_id_regs(struct kvm *kvm)
> >  {
> >       int i;
> > @@ -492,16 +513,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
> >                       /* Shouldn't happen */
> >                       continue;
> >
> > -             /*
> > -              * Some hidden ID registers which are not in arm64_ftr_regs[]
> > -              * would cause warnings from read_sanitised_ftr_reg().
> > -              * Skip those ID registers to avoid the warnings.
> > -              */
> > -             if (id_reg_descs[i].visibility == raz_visibility)
> > -                     /* Hidden or reserved ID register */
> > -                     continue;
> > +             val = 0;
> > +             /* Read KVM sanitised register value if available */
> > +             if (id_reg_descs[i].reset)
> > +                     val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]);
> >
> > -             val = read_sanitised_ftr_reg(id);
> >               IDREG(kvm, id) = val;
> >       }
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 15979c2b87ab..703cf833345a 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >       return 0;
> >  }
> >
> > -static void reset_bvr(struct kvm_vcpu *vcpu,
> > +static u64 reset_bvr(struct kvm_vcpu *vcpu,
> >                     const struct sys_reg_desc *rd)
> >  {
> >       vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
> > +     return rd->val;
> >  }
> >
> >  static bool trap_bcr(struct kvm_vcpu *vcpu,
> > @@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >       return 0;
> >  }
> >
> > -static void reset_bcr(struct kvm_vcpu *vcpu,
> > +static u64 reset_bcr(struct kvm_vcpu *vcpu,
> >                     const struct sys_reg_desc *rd)
> >  {
> >       vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
> > +     return rd->val;
> >  }
> >
> >  static bool trap_wvr(struct kvm_vcpu *vcpu,
> > @@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >       return 0;
> >  }
> >
> > -static void reset_wvr(struct kvm_vcpu *vcpu,
> > +static u64 reset_wvr(struct kvm_vcpu *vcpu,
> >                     const struct sys_reg_desc *rd)
> >  {
> >       vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
> > +     return rd->val;
> >  }
> >
> >  static bool trap_wcr(struct kvm_vcpu *vcpu,
> > @@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >       return 0;
> >  }
> >
> > -static void reset_wcr(struct kvm_vcpu *vcpu,
> > +static u64 reset_wcr(struct kvm_vcpu *vcpu,
> >                     const struct sys_reg_desc *rd)
> >  {
> >       vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
> > +     return rd->val;
> >  }
> >
> > -static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 amair = read_sysreg(amair_el1);
> >       vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
> > +     return amair;
> >  }
> >
> > -static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 actlr = read_sysreg(actlr_el1);
> >       vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
> > +     return actlr;
> >  }
> >
> > -static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 mpidr;
> >
> > @@ -681,7 +687,10 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >       mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> >       mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> >       mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> > -     vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> > +     mpidr |= (1ULL << 31);
> > +     vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
> > +
> > +     return mpidr;
> >  }
> >
> >  static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> > @@ -693,13 +702,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> >       return REG_HIDDEN;
> >  }
> >
> > -static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> >
> >       /* No PMU available, any PMU reg may UNDEF... */
> >       if (!kvm_arm_support_pmu_v3())
> > -             return;
> > +             return 0;
> >
> >       n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> >       n &= ARMV8_PMU_PMCR_N_MASK;
> > @@ -708,33 +717,41 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >
> >       reset_unknown(vcpu, r);
> >       __vcpu_sys_reg(vcpu, r->reg) &= mask;
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> > -static void reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       reset_unknown(vcpu, r);
> >       __vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0);
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> > -static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       reset_unknown(vcpu, r);
> >       __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK;
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> > -static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       reset_unknown(vcpu, r);
> >       __vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK;
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> > -static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 pmcr;
> >
> >       /* No PMU available, PMCR_EL0 may UNDEF... */
> >       if (!kvm_arm_support_pmu_v3())
> > -             return;
> > +             return 0;
> >
> >       /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> >       pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > @@ -742,6 +759,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >               pmcr |= ARMV8_PMU_PMCR_LC;
> >
> >       __vcpu_sys_reg(vcpu, r->reg) = pmcr;
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> >  static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
> > @@ -1221,7 +1240,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >   * Fabricate a CLIDR_EL1 value instead of using the real value, which can vary
> >   * by the physical CPU which the vcpu currently resides in.
> >   */
> > -static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> >       u64 clidr;
> > @@ -1269,6 +1288,8 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >               clidr |= 2 << CLIDR_TTYPE_SHIFT(loc);
> >
> >       __vcpu_sys_reg(vcpu, r->reg) = clidr;
> > +
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> >  static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > @@ -2622,19 +2643,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
> >   */
> >
> >  #define FUNCTION_INVARIANT(reg)                                              \
> > -     static void get_##reg(struct kvm_vcpu *v,                       \
> > +     static u64 get_##reg(struct kvm_vcpu *v,                        \
> >                             const struct sys_reg_desc *r)             \
> >       {                                                               \
> >               ((struct sys_reg_desc *)r)->val = read_sysreg(reg);     \
> > +             return ((struct sys_reg_desc *)r)->val;                 \
> >       }
> >
> >  FUNCTION_INVARIANT(midr_el1)
> >  FUNCTION_INVARIANT(revidr_el1)
> >  FUNCTION_INVARIANT(aidr_el1)
> >
> > -static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
> > +static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
> >  {
> >       ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> > +     return ((struct sys_reg_desc *)r)->val;
> >  }
> >
> >  /* ->val is filled in by kvm_sys_reg_table_init() */
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index e88fd77309b2..21869319f6e1 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -65,12 +65,12 @@ struct sys_reg_desc {
> >                      const struct sys_reg_desc *);
> >
> >       /* Initialization for vcpu. */
> > -     void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
> > +     u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
>
> Could you add a comment what is return from reset() ?
The returned value for idregs is described both in the commit message
and the comment block in previous change.
>
> Thank you,
> Reiji
>
> >
> >       /* Index into sys_reg[], or 0 if we don't need to save it. */
> >       int reg;
> >
> > -     /* Value (usually reset value) */
> > +     /* Value (usually reset value), or write mask for idregs */
> >       u64 val;
> >
> >       /* Custom get/set_user functions, fallback to generic if NULL */
> > @@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
> >  }
> >
> >  /* Reset functions */
> > -static inline void reset_unknown(struct kvm_vcpu *vcpu,
> > +static inline u64 reset_unknown(struct kvm_vcpu *vcpu,
> >                                const struct sys_reg_desc *r)
> >  {
> >       BUG_ON(!r->reg);
> >       BUG_ON(r->reg >= NR_SYS_REGS);
> >       __vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL;
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> > -static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  {
> >       BUG_ON(!r->reg);
> >       BUG_ON(r->reg >= NR_SYS_REGS);
> >       __vcpu_sys_reg(vcpu, r->reg) = r->val;
> > +     return __vcpu_sys_reg(vcpu, r->reg);
> >  }
> >
> >  static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,
> > --
> > 2.40.0.348.gf938b09366-goog
> >
Thanks,
Jing




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux