Hi Marc,
On Wed, May 17, 2023 at 12:41 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Wed, 03 May 2023 18:16:14 +0100,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > 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/id_regs.c | 46 +++++++++++++++++++++++++------
> > arch/arm64/kvm/sys_regs.c | 11 +++++++-
> > arch/arm64/kvm/sys_regs.h | 3 +-
> > 5 files changed, 69 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index bcd774d74f34..a7d4d9e093e3 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -177,6 +177,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
>
> You already have this as part of patch #1 in another include file, and
> then move it here. Surely you can do that in one go. I'd also like it
> to be defined in terms of encodings, and not as a raw value.
Sure, will do.
>
> > +#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)]
>
> Missing brackets around 'kvm'.
Thanks, will fix.
>
> > +struct kvm_idregs {
> > + u64 regs[KVM_ARM_ID_REG_NUM];
> > +};
> > +
> > typedef unsigned int pkvm_handle_t;
> >
> > struct kvm_protected_vm {
> > @@ -243,6 +258,9 @@ struct kvm_arch {
> > /* Hypercall features firmware registers' descriptor */
> > struct kvm_smccc_features smccc_feat;
> >
> > + /* 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.
> > @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> > struct kvm_arm_copy_mte_tags *copy_tags);
> >
> > +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 4b2e16e696a8..e34744c36406 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -153,6 +153,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/id_regs.c b/arch/arm64/kvm/id_regs.c
> > index 96b4c43a5100..e769223bcee2 100644
> > --- a/arch/arm64/kvm/id_regs.c
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> > }
> > }
> >
> > -/* Read a sanitised cpufeature ID register by sys_reg_desc */
>
> Why getting rid of this comment instead of moving it next to the
> (re-implemented) function?
>
Right, will move it to the re-implemented function.
> > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> > +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:
> > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> > return val;
> > }
> >
> > +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));
> > +}
> > +
> > /* cpufeature ID register access trap handlers */
> >
> > static bool access_id_reg(struct kvm_vcpu *vcpu,
> > @@ -458,3 +459,30 @@ 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.
> > + */
> > +void kvm_arm_init_id_regs(struct kvm *kvm)
> > +{
> > + u64 val;
> > + u32 id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> > + id = reg_to_encoding(&id_reg_descs[i]);
> > +
> > + /*
> > + * 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;
>
> Are you sure? What about other visibility attributes that are normally
> evaluated at runtime? This may work as a short term hack, but I'm not
> sure this is the correct long-term solution...
Yes, this is a short term hack. It would be replaced by checking the
reset() function of idregs in patch #5 in this series.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
