On 8/13/2020 6:02 PM, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 11:05:58AM +0200, Andrew Jones wrote: >> On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote: >>> To emulate the ID registers, we need a place to storage the values of >>> the ID regsiters. Maybe putting in kvm_arch_vcpu is a good idea. >>> >>> This commit has no functional changes but only code refactor. When >>> initializing a vcpu, get the values of the ID registers from >>> arm64_ftr_regs and storage them in kvm_arch_vcpu. And we just read >>> the value from kvm_arch_vcpu when getting/setting the value of the ID >>> regs. >>> >>> Signed-off-by: zhanghailiang <[email protected]> >>> Signed-off-by: Peng Liang <[email protected]> >>> --- >>> arch/arm64/include/asm/kvm_host.h | 2 ++ >>> arch/arm64/kvm/arm.c | 20 ++++++++++++++++++++ >>> arch/arm64/kvm/sys_regs.c | 27 +++++++++++++++++++++++---- >>> include/uapi/linux/kvm.h | 11 +++++++++++ >>> 4 files changed, 56 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_host.h >>> b/arch/arm64/include/asm/kvm_host.h >>> index f81151ad3d3c..7f7bd36702f7 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch { >>> u64 last_steal; >>> gpa_t base; >>> } steal; >>> + >>> + struct id_registers idregs; >>> }; >>> >>> /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ >>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >>> index 73e12869afe3..18ebbe1c64ee 100644 >>> --- a/arch/arm64/kvm/arm.c >>> +++ b/arch/arm64/kvm/arm.c >>> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned >>> int id) >>> return 0; >>> } >>> >>> +static int get_cpu_ftr(u32 id, u64 val, void *argp) >>> +{ >>> + struct id_registers *idregs = argp; >>> + >>> + /* >>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), >>> + * where 1<=crm<8, 0<=op2<8. >>> + */ >>> + if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && >>> + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) { >>> + idregs->regs[idregs->num].sys_id = id; >>> + idregs->regs[idregs->num].sys_val = val; >>> + idregs->num++; >> >> This num++ means we should ensure get_cpu_ftr() is only used once per >> VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2 >> >>> + } >>> + >>> + return 0; >>> +} >>> + >>> int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) >>> { >>> int err; >>> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) >>> if (err) >>> return err; >>> >>> + arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs); >>> + >>> return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP); >>> } >>> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index 138961d7ebe3..776c2757a01e 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, >>> return true; >>> } >>> >>> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < vcpu->arch.idregs.num; ++i) { >>> + if (vcpu->arch.idregs.regs[i].sys_id == id) >>> + return &vcpu->arch.idregs.regs[i]; >> >> With a derived index we don't need to search. Just do >> >> if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 || >> sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0) >> return NULL; >> >> return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; >> >> >>> + } >>> + return NULL; >>> +} >>> + >>> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) >>> +{ >>> + struct id_reg_info *ri = kvm_id_reg(vcpu, id); >>> + >>> + BUG_ON(!ri); >>> + return ri->sys_val; >>> +} >>> + >>> /* Read a sanitised cpufeature ID register by sys_reg_desc */ >>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> +static u64 read_id_reg(struct kvm_vcpu *vcpu, >>> struct sys_reg_desc const *r, bool raz) >>> { >>> u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, >>> (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); >>> - u64 val = raz ? 0 : read_sanitised_ftr_reg(id); >>> + u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id); >>> >>> if (id == SYS_ID_AA64PFR0_EL1) { >>> if (!vcpu_has_sve(vcpu)) >>> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, >>> * are stored, and for set_id_reg() we don't allow the effective value >>> * to be changed. >>> */ >>> -static int __get_id_reg(const struct kvm_vcpu *vcpu, >>> +static int __get_id_reg(struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *rd, void __user *uaddr, >>> bool raz) >>> { >>> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu, >>> return reg_to_user(uaddr, &val, id); >>> } >>> >>> -static int __set_id_reg(const struct kvm_vcpu *vcpu, >>> +static int __set_id_reg(struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *rd, void __user *uaddr, >>> bool raz) >>> { >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>> index f6d86033c4fa..1029444d04aa 100644 >>> --- a/include/uapi/linux/kvm.h >>> +++ b/include/uapi/linux/kvm.h >>> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce { >>> __s32 tablefd; >>> }; >>> >>> +#define ID_REG_MAX_NUMS 64 >>> +struct id_reg_info { >>> + uint64_t sys_id; >>> + uint64_t sys_val; >> >> I'm not sure the 'sys_' prefix is necessary. >> >>> +}; >>> + >>> +struct id_registers { >>> + struct id_reg_info regs[ID_REG_MAX_NUMS]; >>> + uint64_t num; >>> +}; >>> + >> >> This is arch specific, so there should be ARMv8 in the names. > > Also, why are id_reg_info and id_registers UAPI? > > Thanks, > drew >
id_reg_info is for information of an ID register, and id_registers contains all the ID registers. Thanks, Peng >> >>> /* >>> * ioctls for VM fds >>> */ >>> -- >>> 2.18.4 >>> >> > > . > _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
