Re: [PATCH v9 1/5] KVM: arm64: Save ID registers' sanitized value per guest

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

 



HI Shameerali,

On Fri, May 19, 2023 at 1:08 AM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Jing Zhang [mailto:jingzhangos@xxxxxxxxxx]
> > Sent: 18 May 2023 20:49
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> > Cc: KVM <kvm@xxxxxxxxxxxxxxx>; KVMARM <kvmarm@xxxxxxxxxxxxxxx>;
> > ARMLinux <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Marc Zyngier
> > <maz@xxxxxxxxxx>; Oliver Upton <oupton@xxxxxxxxxx>; Will Deacon
> > <will@xxxxxxxxxx>; Paolo Bonzini <pbonzini@xxxxxxxxxx>; James Morse
> > <james.morse@xxxxxxx>; Alexandru Elisei <alexandru.elisei@xxxxxxx>;
> > Suzuki K Poulose <suzuki.poulose@xxxxxxx>; Fuad Tabba
> > <tabba@xxxxxxxxxx>; Reiji Watanabe <reijiw@xxxxxxxxxx>; Raghavendra
> > Rao Ananta <rananta@xxxxxxxxxx>
> > Subject: Re: [PATCH v9 1/5] KVM: arm64: Save ID registers' sanitized value
> > per guest
> >
> > Hi Shameerali,
> >
> > On Thu, May 18, 2023 at 12:17 AM Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jing Zhang [mailto:jingzhangos@xxxxxxxxxx]
> > > > Sent: 17 May 2023 07:10
> > > > To: KVM <kvm@xxxxxxxxxxxxxxx>; KVMARM <kvmarm@xxxxxxxxxxxxxxx>;
> > > > ARMLinux <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Marc Zyngier
> > > > <maz@xxxxxxxxxx>; Oliver Upton <oupton@xxxxxxxxxx>
> > > > Cc: Will Deacon <will@xxxxxxxxxx>; Paolo Bonzini
> > <pbonzini@xxxxxxxxxx>;
> > > > James Morse <james.morse@xxxxxxx>; Alexandru Elisei
> > > > <alexandru.elisei@xxxxxxx>; Suzuki K Poulose
> > <suzuki.poulose@xxxxxxx>;
> > > > Fuad Tabba <tabba@xxxxxxxxxx>; Reiji Watanabe <reijiw@xxxxxxxxxx>;
> > > > Raghavendra Rao Ananta <rananta@xxxxxxxxxx>; Jing Zhang
> > > > <jingzhangos@xxxxxxxxxx>
> > > > Subject: [PATCH v9 1/5] KVM: arm64: Save ID registers' sanitized value
> > per
> > > > guest
> > > >
> > > > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> > > > and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> > > > Use the saved ones when ID registers are read by the guest or
> > > > userspace (via KVM_GET_ONE_REG).
> > > >
> > > > 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/include/asm/kvm_host.h | 20 +++++++++
> > > >  arch/arm64/kvm/arm.c              |  1 +
> > > >  arch/arm64/kvm/sys_regs.c         | 69
> > > > +++++++++++++++++++++++++------
> > > >  arch/arm64/kvm/sys_regs.h         |  7 ++++
> > > >  4 files changed, 85 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > b/arch/arm64/include/asm/kvm_host.h
> > > > index 7e7e19ef6993..949a4a782844 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -178,6 +178,21 @@ struct kvm_smccc_features {
> > > >       unsigned long vendor_hyp_bmap;
> > > >  };
> > > >
> > > > +/*
> > > > + * Emulated CPU ID registers per VM
> > > > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> > > > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > > > + *
> > > > + * These emulated idregs are VM-wide, but accessed from the context of
> > a
> > > > vCPU.
> > > > + * Access to id regs are guarded by kvm_arch.config_lock.
> > > > + */
> > > > +#define KVM_ARM_ID_REG_NUM   56
> > > > +#define IDREG_IDX(id)                (((sys_reg_CRm(id) - 1) << 3) |
> > sys_reg_Op2(id))
> > > > +#define IDREG(kvm, id)
> > ((kvm)->arch.idregs.regs[IDREG_IDX(id)])
> > > > +struct kvm_idregs {
> > > > +     u64 regs[KVM_ARM_ID_REG_NUM];
> > > > +};
> > > >
> > >
> > > Not sure we really need this struct here. Why can't this array be moved to
> > > struct kvm_arch directly?
> > It was put in kvm_arch directly before, then got into its own
> > structure in v5 according to the comments here:
> > https://lore.kernel.org/all/861qlaxzyw.wl-maz@xxxxxxxxxx/#t
>
> Ok.
>
> > > >  typedef unsigned int pkvm_handle_t;
> > > >
> > > >  struct kvm_protected_vm {
> > > > @@ -253,6 +268,9 @@ struct kvm_arch {
> > > >       struct kvm_smccc_features smccc_feat;
> > > >       struct maple_tree smccc_filter;
> > > >
> > > > +     /* Emulated CPU ID registers */
> > > > +     struct kvm_idregs idregs;
> > > > +
> > > >       /*
> > > >        * For an untrusted host VM, 'pkvm.handle' is used to lookup
> > > >        * the associated pKVM instance in the hypervisor.
> > > > @@ -1045,6 +1063,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm
> > > > *kvm,
> > > >  int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
> > > >                                   struct kvm_arm_counter_offset
> > *offset);
> > > >
> > > > +void kvm_arm_init_id_regs(struct kvm *kvm);
> > > > +
> > > >  /* Guest/host FPSIMD coordination helpers */
> > > >  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > > >  void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index 14391826241c..774656a0718d 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -163,6 +163,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
> > > > long type)
> > > >
> > > >       set_default_spectre(kvm);
> > > >       kvm_arm_init_hypercalls(kvm);
> > > > +     kvm_arm_init_id_regs(kvm);
> > > >
> > > >       /*
> > > >        * Initialise the default PMUver before there is a chance to
> > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > > index 71b12094d613..d2ee3a1c7f03 100644
> > > > --- a/arch/arm64/kvm/sys_regs.c
> > > > +++ b/arch/arm64/kvm/sys_regs.c
> > > > @@ -41,6 +41,7 @@
> > > >   * 64bit interface.
> > > >   */
> > > >
> > > > +static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
> > > >  static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > >
> > > >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> > > > @@ -364,7 +365,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> > > >                         struct sys_reg_params *p,
> > > >                         const struct sys_reg_desc *r)
> > > >  {
> > > > -     u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> > > > +     u64 val = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1);
> > > >       u32 sr = reg_to_encoding(r);
> > > >
> > > >       if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
> > > > @@ -1208,16 +1209,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> > > >       }
> > > >  }
> > > >
> > > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */
> > > > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc
> > > > const *r)
> > > > +static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> > > >  {
> > > > -     u32 id = reg_to_encoding(r);
> > > > -     u64 val;
> > > > -
> > > > -     if (sysreg_visible_as_raz(vcpu, r))
> > > > -             return 0;
> > > > -
> > > > -     val = read_sanitised_ftr_reg(id);
> > > > +     u64 val = IDREG(vcpu->kvm, id);
> > > >
> > > >       switch (id) {
> > > >       case SYS_ID_AA64PFR0_EL1:
> > > > @@ -1280,6 +1274,26 @@ static u64 read_id_reg(const struct
> > kvm_vcpu
> > > > *vcpu, struct sys_reg_desc const *r
> > > >       return val;
> > > >  }
> > > >
> > > > +/* Read a sanitised cpufeature ID register by sys_reg_desc */
> > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct
> > sys_reg_desc
> > > > const *r)
> > > > +{
> > > > +     if (sysreg_visible_as_raz(vcpu, r))
> > > > +             return 0;
> > > > +
> > > > +     return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
> > > > +}
> > > > +
> > > > +/*
> > > > + * Return true if the register's (Op0, Op1, CRn, CRm, Op2) is
> > > > + * (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > > > + */
> > > > +static inline bool is_id_reg(u32 id)
> > > > +{
> > > > +     return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> > > > +             sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
> > > > +             sys_reg_CRm(id) < 8);
> > > > +}
> > > > +
> > > >  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > > >                                 const struct sys_reg_desc *r)
> > > >  {
> > > > @@ -2244,8 +2258,8 @@ static bool trap_dbgdidr(struct kvm_vcpu
> > *vcpu,
> > > >       if (p->is_write) {
> > > >               return ignore_write(vcpu, p);
> > > >       } else {
> > > > -             u64 dfr =
> > read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > > > -             u64 pfr =
> > read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
> > > > +             u64 dfr = kvm_arm_read_id_reg(vcpu,
> > SYS_ID_AA64DFR0_EL1);
> > > > +             u64 pfr = kvm_arm_read_id_reg(vcpu,
> > SYS_ID_AA64PFR0_EL1);
> > >
> > > Does this change the behavior slightly as now within the
> > kvm_arm_read_id_reg()
> > > the val will be further adjusted based on KVM/vCPU?
> > That's a good question. Although the actual behavior would be the same
> > no matter read idreg with read_sanitised_ftr_reg or
> > kvm_arm_read_id_reg, it is possible that the behavior would change
> > potentially in the future.
> > Since now every guest has its own idregs, for every guest, the idregs
> > should be read from kvm_arm_read_id_reg instead of
> > read_sanitised_ftr_reg.
> > The point is, for trap_dbgdidr, we should read AA64DFR0/AA64PFR0 from
> > host or the VM-scope?
>
> Ok. I was just double checking whether it changes the behavior now itself or
> not as we claim no functional changes in this series. As far as host vs VM
> scope, I am not sure as well. From a quick look through the history of debug
> support, couldn’t find anything that mandates host values though.
>
> Thanks,
> Shameer
>
Thanks for the investigation. Let's keep it this way now and see if
there are any other comments.

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