* [RFC 0/4] kvm: arm64: emulate ID registers @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: zhang.zhanghailiang, kvm, maz, will In AArch64, guest will read the same values of the ID regsiters with host. Both of them read the values from arm64_ftr_regs. This patch series add support to emulate and configure ID registers so that we can control the value of ID registers that guest read. Peng Liang (4): arm64: add a helper function to traverse arm64_ftr_regs kvm: arm64: emulate the ID registers kvm: arm64: make ID registers configurable kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kernel/cpufeature.c | 13 ++++++++ arch/arm64/kvm/arm.c | 21 ++++++++++++ arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- include/uapi/linux/kvm.h | 12 +++++++ 6 files changed, 89 insertions(+), 11 deletions(-) -- 2.18.4 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 0/4] kvm: arm64: emulate ID registers @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: kvm, maz, will, zhang.zhanghailiang, xiexiangyou, Peng Liang In AArch64, guest will read the same values of the ID regsiters with host. Both of them read the values from arm64_ftr_regs. This patch series add support to emulate and configure ID registers so that we can control the value of ID registers that guest read. Peng Liang (4): arm64: add a helper function to traverse arm64_ftr_regs kvm: arm64: emulate the ID registers kvm: arm64: make ID registers configurable kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kernel/cpufeature.c | 13 ++++++++ arch/arm64/kvm/arm.c | 21 ++++++++++++ arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- include/uapi/linux/kvm.h | 12 +++++++ 6 files changed, 89 insertions(+), 11 deletions(-) -- 2.18.4 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 1/4] arm64: add a helper function to traverse arm64_ftr_regs 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 6:05 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: zhang.zhanghailiang, kvm, maz, will If we want to emulate ID registers, we need to initialize ID registers firstly. This commit is to add a helper function to traverse arm64_ftr_regs so that we can initialize ID registers from arm64_ftr_regs. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/cpufeature.c | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 89b4f0142c28..2ba7c4f11d8a 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -79,6 +79,8 @@ struct arm64_ftr_reg { extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp); + /* * CPU capabilities: * diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index a389b999482e..eb4c78b85a35 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id) return regp->sys_val; } +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(arm64_ftr_regs); i++) { + ret = (*op)(arm64_ftr_regs[i].sys_id, + arm64_ftr_regs[i].reg->sys_val, argp); + if (ret < 0) + return ret; + } + return 0; +} + #define read_sysreg_case(r) \ case r: return read_sysreg_s(r) -- 2.18.4 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 1/4] arm64: add a helper function to traverse arm64_ftr_regs @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: kvm, maz, will, zhang.zhanghailiang, xiexiangyou, Peng Liang If we want to emulate ID registers, we need to initialize ID registers firstly. This commit is to add a helper function to traverse arm64_ftr_regs so that we can initialize ID registers from arm64_ftr_regs. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/cpufeature.c | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 89b4f0142c28..2ba7c4f11d8a 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -79,6 +79,8 @@ struct arm64_ftr_reg { extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp); + /* * CPU capabilities: * diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index a389b999482e..eb4c78b85a35 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id) return regp->sys_val; } +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(arm64_ftr_regs); i++) { + ret = (*op)(arm64_ftr_regs[i].sys_id, + arm64_ftr_regs[i].reg->sys_val, argp); + if (ret < 0) + return ret; + } + return 0; +} + #define read_sysreg_case(r) \ case r: return read_sysreg_s(r) -- 2.18.4 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 6:05 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: zhang.zhanghailiang, kvm, maz, will 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 <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- 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++; + } + + 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]; + } + 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; +}; + +struct id_registers { + struct id_reg_info regs[ID_REG_MAX_NUMS]; + uint64_t num; +}; + /* * ioctls for VM fds */ -- 2.18.4 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: kvm, maz, will, zhang.zhanghailiang, xiexiangyou, Peng Liang 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 <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- 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++; + } + + 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]; + } + 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; +}; + +struct id_registers { + struct id_reg_info regs[ID_REG_MAX_NUMS]; + uint64_t num; +}; + /* * ioctls for VM fds */ -- 2.18.4 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 9:05 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:05 UTC (permalink / raw) To: Peng Liang; +Cc: zhang.zhanghailiang, kvm, maz, will, kvmarm 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 <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > 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. > /* > * ioctls for VM fds > */ > -- > 2.18.4 > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-13 9:05 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:05 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, maz, will, zhang.zhanghailiang, xiexiangyou 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 <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > 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. > /* > * ioctls for VM fds > */ > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 9:05 ` Andrew Jones @ 2020-08-13 10:02 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 10:02 UTC (permalink / raw) To: Peng Liang; +Cc: zhang.zhanghailiang, kvm, maz, will, kvmarm 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 <zhang.zhanghailiang@huawei.com> > > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > > --- > > 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 > > > /* > > * ioctls for VM fds > > */ > > -- > > 2.18.4 > > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-13 10:02 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 10:02 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, maz, will, zhang.zhanghailiang, xiexiangyou 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 <zhang.zhanghailiang@huawei.com> > > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > > --- > > 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 > > > /* > > * ioctls for VM fds > > */ > > -- > > 2.18.4 > > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 10:02 ` Andrew Jones @ 2020-08-14 11:49 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones; +Cc: Zhanghailiang, kvm, maz, will, kvmarm 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 <zhang.zhanghailiang@huawei.com> >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >>> --- >>> 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 kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-14 11:49 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones Cc: kvmarm, kvm, maz, will, Zhanghailiang, xiexiangyou 00584000, zhukeqian 00502301 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 <zhang.zhanghailiang@huawei.com> >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >>> --- >>> 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 >>> >> > > . > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-14 11:49 ` Peng Liang @ 2020-08-14 12:51 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-14 12:51 UTC (permalink / raw) To: Peng Liang; +Cc: Zhanghailiang, kvm, maz, will, kvmarm On Fri, Aug 14, 2020 at 07:49:43PM +0800, Peng Liang wrote: > 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 <zhang.zhanghailiang@huawei.com> > >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> > >>> --- > >>> 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. Obviously. But why is that UAPI? What user interface is using them? > > Thanks, > Peng > > >> > >>> /* > >>> * ioctls for VM fds > >>> */ > >>> -- > >>> 2.18.4 > >>> > >> > > > > . > > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-14 12:51 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-14 12:51 UTC (permalink / raw) To: Peng Liang Cc: kvmarm, kvm, maz, will, Zhanghailiang, xiexiangyou 00584000, zhukeqian 00502301 On Fri, Aug 14, 2020 at 07:49:43PM +0800, Peng Liang wrote: > 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 <zhang.zhanghailiang@huawei.com> > >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> > >>> --- > >>> 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. Obviously. But why is that UAPI? What user interface is using them? > > Thanks, > Peng > > >> > >>> /* > >>> * ioctls for VM fds > >>> */ > >>> -- > >>> 2.18.4 > >>> > >> > > > > . > > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 9:05 ` Andrew Jones @ 2020-08-14 11:49 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones; +Cc: Zhanghailiang, kvm, maz, will, kvmarm On 8/13/2020 5:05 PM, 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 <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> 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)]; > > Thank you for your suggestions. >> + } >> + 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. Some names are not very suitable, I'll change them. > >> /* >> * ioctls for VM fds >> */ >> -- >> 2.18.4 >> > > . > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-14 11:49 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones Cc: kvmarm, kvm, maz, will, Zhanghailiang, xiexiangyou 00584000, zhukeqian 00502301 On 8/13/2020 5:05 PM, 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 <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> 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)]; > > Thank you for your suggestions. >> + } >> + 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. Some names are not very suitable, I'll change them. > >> /* >> * ioctls for VM fds >> */ >> -- >> 2.18.4 >> > > . > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 6:05 ` Peng Liang (?) (?) @ 2020-08-13 23:10 ` kernel test robot -1 siblings, 0 replies; 37+ messages in thread From: kernel test robot @ 2020-08-13 23:10 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 1486 bytes --] Hi Peng, [FYI, it's a private test report for your RFC patch.] [auto build test ERROR on kvm/linux-next] [also build test ERROR on arm64/for-next/core v5.8 next-20200813] [cannot apply to kvmarm/next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Peng-Liang/kvm-arm64-emulate-ID-registers/20200813-141717 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next config: x86_64-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from <command-line>:32: >> ./usr/include/linux/kvm.h:1277:2: error: unknown type name 'uint64_t' 1277 | uint64_t sys_id; | ^~~~~~~~ ./usr/include/linux/kvm.h:1278:2: error: unknown type name 'uint64_t' 1278 | uint64_t sys_val; | ^~~~~~~~ ./usr/include/linux/kvm.h:1283:2: error: unknown type name 'uint64_t' 1283 | uint64_t num; | ^~~~~~~~ --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 75536 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers 2020-08-13 6:05 ` Peng Liang @ 2020-08-14 12:20 ` Marc Zyngier -1 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2020-08-14 12:20 UTC (permalink / raw) To: Peng Liang; +Cc: will, kvmarm, kvm, zhang.zhanghailiang On 2020-08-13 07:05, 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 <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > 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; System registers are to be stored in the sysreg file. I've already spent enough time moving them out of the various subsystems. > }; > > /* 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++; > + } > + > + 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]; > + } > + 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; > +}; > + > +struct id_registers { > + struct id_reg_info regs[ID_REG_MAX_NUMS]; > + uint64_t num; > +}; > + > /* > * ioctls for VM fds > */ No way this is an acceptable interface. We have the one-reg interface, which takes a system register encoding. M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/4] kvm: arm64: emulate the ID registers @ 2020-08-14 12:20 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2020-08-14 12:20 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, will, zhang.zhanghailiang, xiexiangyou On 2020-08-13 07:05, 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 <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > 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; System registers are to be stored in the sysreg file. I've already spent enough time moving them out of the various subsystems. > }; > > /* 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++; > + } > + > + 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]; > + } > + 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; > +}; > + > +struct id_registers { > + struct id_reg_info regs[ID_REG_MAX_NUMS]; > + uint64_t num; > +}; > + > /* > * ioctls for VM fds > */ No way this is an acceptable interface. We have the one-reg interface, which takes a system register encoding. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 3/4] kvm: arm64: make ID registers configurable 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 6:05 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: zhang.zhanghailiang, kvm, maz, will It's time to make ID registers configurable. When userspace (but not guest) want to set the values of ID registers, save the value in kvm_arch_vcpu so that guest can read the modified values. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 776c2757a01e..f98635489966 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) return ri->sys_val; } +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) +{ + struct id_reg_info *ri = kvm_id_reg(vcpu, id); + + BUG_ON(!ri); + ri->sys_val = value; +} + /* Read a sanitised cpufeature ID register by sys_reg_desc */ static u64 read_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_desc const *r, bool raz) @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, /* * cpufeature ID register user accessors - * - * For now, these registers are immutable for userspace, so no values - * are stored, and for set_id_reg() we don't allow the effective value - * to be changed. */ static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, void __user *uaddr, @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, if (err) return err; - /* This is what we mean by invariant: you can't change it. */ - if (val != read_id_reg(vcpu, rd, raz)) - return -EINVAL; + if (raz) { + if (val != read_id_reg(vcpu, rd, raz)) + return -EINVAL; + } else { + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, + (u32)rd->CRm, (u32)rd->Op2); + kvm_set_id_reg(vcpu, reg_id, val); + } return 0; } -- 2.18.4 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 3/4] kvm: arm64: make ID registers configurable @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: kvm, maz, will, zhang.zhanghailiang, xiexiangyou, Peng Liang It's time to make ID registers configurable. When userspace (but not guest) want to set the values of ID registers, save the value in kvm_arch_vcpu so that guest can read the modified values. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 776c2757a01e..f98635489966 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) return ri->sys_val; } +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) +{ + struct id_reg_info *ri = kvm_id_reg(vcpu, id); + + BUG_ON(!ri); + ri->sys_val = value; +} + /* Read a sanitised cpufeature ID register by sys_reg_desc */ static u64 read_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_desc const *r, bool raz) @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, /* * cpufeature ID register user accessors - * - * For now, these registers are immutable for userspace, so no values - * are stored, and for set_id_reg() we don't allow the effective value - * to be changed. */ static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, void __user *uaddr, @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, if (err) return err; - /* This is what we mean by invariant: you can't change it. */ - if (val != read_id_reg(vcpu, rd, raz)) - return -EINVAL; + if (raz) { + if (val != read_id_reg(vcpu, rd, raz)) + return -EINVAL; + } else { + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, + (u32)rd->CRm, (u32)rd->Op2); + kvm_set_id_reg(vcpu, reg_id, val); + } return 0; } -- 2.18.4 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 9:09 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:09 UTC (permalink / raw) To: Peng Liang; +Cc: zhang.zhanghailiang, kvm, maz, will, kvmarm On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote: > It's time to make ID registers configurable. When userspace (but not > guest) want to set the values of ID registers, save the value in > kvm_arch_vcpu so that guest can read the modified values. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 776c2757a01e..f98635489966 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) > return ri->sys_val; > } > > +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) > +{ > + struct id_reg_info *ri = kvm_id_reg(vcpu, id); > + > + BUG_ON(!ri); > + ri->sys_val = value; > +} > + > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > static u64 read_id_reg(struct kvm_vcpu *vcpu, > struct sys_reg_desc const *r, bool raz) > @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, > > /* > * cpufeature ID register user accessors > - * > - * For now, these registers are immutable for userspace, so no values > - * are stored, and for set_id_reg() we don't allow the effective value > - * to be changed. > */ > static int __get_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, > if (err) > return err; > > - /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(vcpu, rd, raz)) > - return -EINVAL; > + if (raz) { > + if (val != read_id_reg(vcpu, rd, raz)) > + return -EINVAL; > + } else { > + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, > + (u32)rd->CRm, (u32)rd->Op2); > + kvm_set_id_reg(vcpu, reg_id, val); So userspace can set the ID registers to whatever they want? I think each register should have its own sanity checks applied before accepting the input. Thanks, drew > + } > > return 0; > } > -- > 2.18.4 > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable @ 2020-08-13 9:09 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:09 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, maz, will, zhang.zhanghailiang, xiexiangyou On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote: > It's time to make ID registers configurable. When userspace (but not > guest) want to set the values of ID registers, save the value in > kvm_arch_vcpu so that guest can read the modified values. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 776c2757a01e..f98635489966 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) > return ri->sys_val; > } > > +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) > +{ > + struct id_reg_info *ri = kvm_id_reg(vcpu, id); > + > + BUG_ON(!ri); > + ri->sys_val = value; > +} > + > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > static u64 read_id_reg(struct kvm_vcpu *vcpu, > struct sys_reg_desc const *r, bool raz) > @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, > > /* > * cpufeature ID register user accessors > - * > - * For now, these registers are immutable for userspace, so no values > - * are stored, and for set_id_reg() we don't allow the effective value > - * to be changed. > */ > static int __get_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, > if (err) > return err; > > - /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(vcpu, rd, raz)) > - return -EINVAL; > + if (raz) { > + if (val != read_id_reg(vcpu, rd, raz)) > + return -EINVAL; > + } else { > + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, > + (u32)rd->CRm, (u32)rd->Op2); > + kvm_set_id_reg(vcpu, reg_id, val); So userspace can set the ID registers to whatever they want? I think each register should have its own sanity checks applied before accepting the input. Thanks, drew > + } > > return 0; > } > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable 2020-08-13 9:09 ` Andrew Jones @ 2020-08-14 11:49 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones; +Cc: Zhanghailiang, kvm, maz, will, kvmarm On 8/13/2020 5:09 PM, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote: >> It's time to make ID registers configurable. When userspace (but not >> guest) want to set the values of ID registers, save the value in >> kvm_arch_vcpu so that guest can read the modified values. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 776c2757a01e..f98635489966 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) >> return ri->sys_val; >> } >> >> +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) >> +{ >> + struct id_reg_info *ri = kvm_id_reg(vcpu, id); >> + >> + BUG_ON(!ri); >> + ri->sys_val = value; >> +} >> + >> /* Read a sanitised cpufeature ID register by sys_reg_desc */ >> static u64 read_id_reg(struct kvm_vcpu *vcpu, >> struct sys_reg_desc const *r, bool raz) >> @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, >> >> /* >> * cpufeature ID register user accessors >> - * >> - * For now, these registers are immutable for userspace, so no values >> - * are stored, and for set_id_reg() we don't allow the effective value >> - * to be changed. >> */ >> static int __get_id_reg(struct kvm_vcpu *vcpu, >> const struct sys_reg_desc *rd, void __user *uaddr, >> @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, >> if (err) >> return err; >> >> - /* This is what we mean by invariant: you can't change it. */ >> - if (val != read_id_reg(vcpu, rd, raz)) >> - return -EINVAL; >> + if (raz) { >> + if (val != read_id_reg(vcpu, rd, raz)) >> + return -EINVAL; >> + } else { >> + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, >> + (u32)rd->CRm, (u32)rd->Op2); >> + kvm_set_id_reg(vcpu, reg_id, val); > > So userspace can set the ID registers to whatever they want? I think each > register should have its own sanity checks applied before accepting the > input. > > Thanks, > drew > >> + } >> >> return 0; >> } >> -- >> 2.18.4 >> > > . > Yea, sanity checkers are necessary and I'm working on it. I think we should make sure that every ID fields should be checked to match the HW capabilities so that guest will not be confused because of a careless hypervisor. Thanks, Peng _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable @ 2020-08-14 11:49 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones Cc: kvmarm, kvm, maz, will, Zhanghailiang, xiexiangyou 00584000, zhukeqian 00502301 On 8/13/2020 5:09 PM, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote: >> It's time to make ID registers configurable. When userspace (but not >> guest) want to set the values of ID registers, save the value in >> kvm_arch_vcpu so that guest can read the modified values. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 776c2757a01e..f98635489966 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id) >> return ri->sys_val; >> } >> >> +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) >> +{ >> + struct id_reg_info *ri = kvm_id_reg(vcpu, id); >> + >> + BUG_ON(!ri); >> + ri->sys_val = value; >> +} >> + >> /* Read a sanitised cpufeature ID register by sys_reg_desc */ >> static u64 read_id_reg(struct kvm_vcpu *vcpu, >> struct sys_reg_desc const *r, bool raz) >> @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu, >> >> /* >> * cpufeature ID register user accessors >> - * >> - * For now, these registers are immutable for userspace, so no values >> - * are stored, and for set_id_reg() we don't allow the effective value >> - * to be changed. >> */ >> static int __get_id_reg(struct kvm_vcpu *vcpu, >> const struct sys_reg_desc *rd, void __user *uaddr, >> @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, >> if (err) >> return err; >> >> - /* This is what we mean by invariant: you can't change it. */ >> - if (val != read_id_reg(vcpu, rd, raz)) >> - return -EINVAL; >> + if (raz) { >> + if (val != read_id_reg(vcpu, rd, raz)) >> + return -EINVAL; >> + } else { >> + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, >> + (u32)rd->CRm, (u32)rd->Op2); >> + kvm_set_id_reg(vcpu, reg_id, val); > > So userspace can set the ID registers to whatever they want? I think each > register should have its own sanity checks applied before accepting the > input. > > Thanks, > drew > >> + } >> >> return 0; >> } >> -- >> 2.18.4 >> > > . > Yea, sanity checkers are necessary and I'm working on it. I think we should make sure that every ID fields should be checked to match the HW capabilities so that guest will not be confused because of a careless hypervisor. Thanks, Peng ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 9:52 ` Marc Zyngier -1 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2020-08-13 9:52 UTC (permalink / raw) To: Peng Liang; +Cc: will, kvmarm, kvm, zhang.zhanghailiang On 2020-08-13 07:05, Peng Liang wrote: > It's time to make ID registers configurable. When userspace (but not > guest) want to set the values of ID registers, save the value in > kvm_arch_vcpu so that guest can read the modified values. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 776c2757a01e..f98635489966 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, > u64 id) > return ri->sys_val; > } > > +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) > +{ > + struct id_reg_info *ri = kvm_id_reg(vcpu, id); > + > + BUG_ON(!ri); > + ri->sys_val = value; > +} > + > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > static u64 read_id_reg(struct kvm_vcpu *vcpu, > struct sys_reg_desc const *r, bool raz) > @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu > *vcpu, > > /* > * cpufeature ID register user accessors > - * > - * For now, these registers are immutable for userspace, so no values > - * are stored, and for set_id_reg() we don't allow the effective value > - * to be changed. > */ > static int __get_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, > if (err) > return err; > > - /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(vcpu, rd, raz)) > - return -EINVAL; > + if (raz) { > + if (val != read_id_reg(vcpu, rd, raz)) > + return -EINVAL; > + } else { > + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, > + (u32)rd->CRm, (u32)rd->Op2); > + kvm_set_id_reg(vcpu, reg_id, val); > + } > > return 0; > } This cannot work. If userspace can override an idreg, it cannot conflict with anything the HW is capable of. It also cannot conflict with features that the host doesn't want to expose to the guest. Another thing is that you now have features that do not match the MIDR (you can describe an A57 with SVE, for example). This will trigger issues in guests, as the combination isn't expected. And then there is the eternal story about errata workarounds. If you can override the ID regs, how can the guest mitigate errata that you are now hiding from it? Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/4] kvm: arm64: make ID registers configurable @ 2020-08-13 9:52 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2020-08-13 9:52 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, will, zhang.zhanghailiang, xiexiangyou On 2020-08-13 07:05, Peng Liang wrote: > It's time to make ID registers configurable. When userspace (but not > guest) want to set the values of ID registers, save the value in > kvm_arch_vcpu so that guest can read the modified values. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 776c2757a01e..f98635489966 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, > u64 id) > return ri->sys_val; > } > > +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value) > +{ > + struct id_reg_info *ri = kvm_id_reg(vcpu, id); > + > + BUG_ON(!ri); > + ri->sys_val = value; > +} > + > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > static u64 read_id_reg(struct kvm_vcpu *vcpu, > struct sys_reg_desc const *r, bool raz) > @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu > *vcpu, > > /* > * cpufeature ID register user accessors > - * > - * For now, these registers are immutable for userspace, so no values > - * are stored, and for set_id_reg() we don't allow the effective value > - * to be changed. > */ > static int __get_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu, > if (err) > return err; > > - /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(vcpu, rd, raz)) > - return -EINVAL; > + if (raz) { > + if (val != read_id_reg(vcpu, rd, raz)) > + return -EINVAL; > + } else { > + u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn, > + (u32)rd->CRm, (u32)rd->Op2); > + kvm_set_id_reg(vcpu, reg_id, val); > + } > > return 0; > } This cannot work. If userspace can override an idreg, it cannot conflict with anything the HW is capable of. It also cannot conflict with features that the host doesn't want to expose to the guest. Another thing is that you now have features that do not match the MIDR (you can describe an A57 with SVE, for example). This will trigger issues in guests, as the combination isn't expected. And then there is the eternal story about errata workarounds. If you can override the ID regs, how can the guest mitigate errata that you are now hiding from it? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 6:05 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: zhang.zhanghailiang, kvm, maz, will Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM supports to set CPU features in AArch64. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/kvm/arm.c | 1 + include/uapi/linux/kvm.h | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 18ebbe1c64ee..72b9e8fc606f 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: case KVM_CAP_ARM_NISV_TO_USER: case KVM_CAP_ARM_INJECT_EXT_DABT: + case KVM_CAP_ARM_CPU_FEATURE: r = 1; break; case KVM_CAP_ARM_SET_DEVICE_ADDR: diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 1029444d04aa..0eca4f7c7fef 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_LAST_CPU 184 #define KVM_CAP_SMALLER_MAXPHYADDR 185 #define KVM_CAP_S390_DIAG318 186 +#define KVM_CAP_ARM_CPU_FEATURE 187 #ifdef KVM_CAP_IRQ_ROUTING -- 2.18.4 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension @ 2020-08-13 6:05 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-13 6:05 UTC (permalink / raw) To: kvmarm; +Cc: kvm, maz, will, zhang.zhanghailiang, xiexiangyou, Peng Liang Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM supports to set CPU features in AArch64. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- arch/arm64/kvm/arm.c | 1 + include/uapi/linux/kvm.h | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 18ebbe1c64ee..72b9e8fc606f 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: case KVM_CAP_ARM_NISV_TO_USER: case KVM_CAP_ARM_INJECT_EXT_DABT: + case KVM_CAP_ARM_CPU_FEATURE: r = 1; break; case KVM_CAP_ARM_SET_DEVICE_ADDR: diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 1029444d04aa..0eca4f7c7fef 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_LAST_CPU 184 #define KVM_CAP_SMALLER_MAXPHYADDR 185 #define KVM_CAP_S390_DIAG318 186 +#define KVM_CAP_ARM_CPU_FEATURE 187 #ifdef KVM_CAP_IRQ_ROUTING -- 2.18.4 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 9:10 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:10 UTC (permalink / raw) To: Peng Liang; +Cc: zhang.zhanghailiang, kvm, maz, will, kvmarm On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote: > Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM > supports to set CPU features in AArch64. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/arm.c | 1 + > include/uapi/linux/kvm.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 18ebbe1c64ee..72b9e8fc606f 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: > case KVM_CAP_ARM_NISV_TO_USER: > case KVM_CAP_ARM_INJECT_EXT_DABT: > + case KVM_CAP_ARM_CPU_FEATURE: > r = 1; > break; > case KVM_CAP_ARM_SET_DEVICE_ADDR: > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 1029444d04aa..0eca4f7c7fef 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_LAST_CPU 184 > #define KVM_CAP_SMALLER_MAXPHYADDR 185 > #define KVM_CAP_S390_DIAG318 186 > +#define KVM_CAP_ARM_CPU_FEATURE 187 > > #ifdef KVM_CAP_IRQ_ROUTING > > -- > 2.18.4 > All new caps should be documented in Documentation/virt/kvm/api.rst Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension @ 2020-08-13 9:10 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:10 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, kvm, maz, will, zhang.zhanghailiang, xiexiangyou On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote: > Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM > supports to set CPU features in AArch64. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > arch/arm64/kvm/arm.c | 1 + > include/uapi/linux/kvm.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 18ebbe1c64ee..72b9e8fc606f 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: > case KVM_CAP_ARM_NISV_TO_USER: > case KVM_CAP_ARM_INJECT_EXT_DABT: > + case KVM_CAP_ARM_CPU_FEATURE: > r = 1; > break; > case KVM_CAP_ARM_SET_DEVICE_ADDR: > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 1029444d04aa..0eca4f7c7fef 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_LAST_CPU 184 > #define KVM_CAP_SMALLER_MAXPHYADDR 185 > #define KVM_CAP_S390_DIAG318 186 > +#define KVM_CAP_ARM_CPU_FEATURE 187 > > #ifdef KVM_CAP_IRQ_ROUTING > > -- > 2.18.4 > All new caps should be documented in Documentation/virt/kvm/api.rst Thanks, drew ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension 2020-08-13 9:10 ` Andrew Jones @ 2020-08-14 11:49 ` Peng Liang -1 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones; +Cc: Zhanghailiang, kvm, maz, will, kvmarm On 8/13/2020 5:10 PM, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote: >> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM >> supports to set CPU features in AArch64. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> arch/arm64/kvm/arm.c | 1 + >> include/uapi/linux/kvm.h | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index 18ebbe1c64ee..72b9e8fc606f 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: >> case KVM_CAP_ARM_NISV_TO_USER: >> case KVM_CAP_ARM_INJECT_EXT_DABT: >> + case KVM_CAP_ARM_CPU_FEATURE: >> r = 1; >> break; >> case KVM_CAP_ARM_SET_DEVICE_ADDR: >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 1029444d04aa..0eca4f7c7fef 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_LAST_CPU 184 >> #define KVM_CAP_SMALLER_MAXPHYADDR 185 >> #define KVM_CAP_S390_DIAG318 186 >> +#define KVM_CAP_ARM_CPU_FEATURE 187 >> >> #ifdef KVM_CAP_IRQ_ROUTING >> >> -- >> 2.18.4 >> > > All new caps should be documented in Documentation/virt/kvm/api.rst > > Thanks, > drew > > . > Sorry, I'll document it. Thanks, Peng _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension @ 2020-08-14 11:49 ` Peng Liang 0 siblings, 0 replies; 37+ messages in thread From: Peng Liang @ 2020-08-14 11:49 UTC (permalink / raw) To: Andrew Jones Cc: kvmarm, kvm, maz, will, Zhanghailiang, xiexiangyou 00584000, zhukeqian 00502301 On 8/13/2020 5:10 PM, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote: >> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM >> supports to set CPU features in AArch64. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> arch/arm64/kvm/arm.c | 1 + >> include/uapi/linux/kvm.h | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index 18ebbe1c64ee..72b9e8fc606f 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2: >> case KVM_CAP_ARM_NISV_TO_USER: >> case KVM_CAP_ARM_INJECT_EXT_DABT: >> + case KVM_CAP_ARM_CPU_FEATURE: >> r = 1; >> break; >> case KVM_CAP_ARM_SET_DEVICE_ADDR: >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 1029444d04aa..0eca4f7c7fef 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_LAST_CPU 184 >> #define KVM_CAP_SMALLER_MAXPHYADDR 185 >> #define KVM_CAP_S390_DIAG318 186 >> +#define KVM_CAP_ARM_CPU_FEATURE 187 >> >> #ifdef KVM_CAP_IRQ_ROUTING >> >> -- >> 2.18.4 >> > > All new caps should be documented in Documentation/virt/kvm/api.rst > > Thanks, > drew > > . > Sorry, I'll document it. Thanks, Peng ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/4] kvm: arm64: emulate ID registers 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 9:14 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:14 UTC (permalink / raw) To: Peng Liang; +Cc: maz, will, kvmarm, kvm, zhang.zhanghailiang On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote: > In AArch64, guest will read the same values of the ID regsiters with > host. Both of them read the values from arm64_ftr_regs. This patch > series add support to emulate and configure ID registers so that we can > control the value of ID registers that guest read. > > Peng Liang (4): > arm64: add a helper function to traverse arm64_ftr_regs > kvm: arm64: emulate the ID registers > kvm: arm64: make ID registers configurable > kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension > > arch/arm64/include/asm/cpufeature.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kernel/cpufeature.c | 13 ++++++++ > arch/arm64/kvm/arm.c | 21 ++++++++++++ > arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- > include/uapi/linux/kvm.h | 12 +++++++ > 6 files changed, 89 insertions(+), 11 deletions(-) > > -- > 2.18.4 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm > Hi Peng, I'm glad to see an effort has started in trying to prepare for CPU models, allowing migration beyond identical hosts. How have you tested this series? I.e. what userspace did you use and with what additional patches? KVM changes like these are also easily tested with KVM selftests[*]. Have you considered posting a selftest? [*] Linux repo: tools/testing/selftests/kvm/ Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/4] kvm: arm64: emulate ID registers @ 2020-08-13 9:14 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 9:14 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, zhang.zhanghailiang, kvm, maz, will On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote: > In AArch64, guest will read the same values of the ID regsiters with > host. Both of them read the values from arm64_ftr_regs. This patch > series add support to emulate and configure ID registers so that we can > control the value of ID registers that guest read. > > Peng Liang (4): > arm64: add a helper function to traverse arm64_ftr_regs > kvm: arm64: emulate the ID registers > kvm: arm64: make ID registers configurable > kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension > > arch/arm64/include/asm/cpufeature.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kernel/cpufeature.c | 13 ++++++++ > arch/arm64/kvm/arm.c | 21 ++++++++++++ > arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- > include/uapi/linux/kvm.h | 12 +++++++ > 6 files changed, 89 insertions(+), 11 deletions(-) > > -- > 2.18.4 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm > Hi Peng, I'm glad to see an effort has started in trying to prepare for CPU models, allowing migration beyond identical hosts. How have you tested this series? I.e. what userspace did you use and with what additional patches? KVM changes like these are also easily tested with KVM selftests[*]. Have you considered posting a selftest? [*] Linux repo: tools/testing/selftests/kvm/ Thanks, drew ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/4] kvm: arm64: emulate ID registers 2020-08-13 6:05 ` Peng Liang @ 2020-08-13 14:19 ` Andrew Jones -1 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 14:19 UTC (permalink / raw) To: Peng Liang; +Cc: maz, will, kvmarm, kvm, zhang.zhanghailiang On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote: > In AArch64, guest will read the same values of the ID regsiters with > host. Both of them read the values from arm64_ftr_regs. This patch > series add support to emulate and configure ID registers so that we can > control the value of ID registers that guest read. > > Peng Liang (4): > arm64: add a helper function to traverse arm64_ftr_regs > kvm: arm64: emulate the ID registers > kvm: arm64: make ID registers configurable > kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension > > arch/arm64/include/asm/cpufeature.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kernel/cpufeature.c | 13 ++++++++ > arch/arm64/kvm/arm.c | 21 ++++++++++++ > arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- > include/uapi/linux/kvm.h | 12 +++++++ > 6 files changed, 89 insertions(+), 11 deletions(-) > > -- > 2.18.4 > Hi Peng, After having looked at the QEMU series I don't believe this is the right approach to CPU models. As stated in my reply [*] this approach appears to just be shifting the problem wholesale to the user. On the KVM side, I think we should start by auditing all the ID registers to see what should be masked and what should be under user control. Then, we need to consider what MIDR a guest that can migrate between hosts of different MIDRs should have. And that'll require addressing the errata problem in one way or another. [*] https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg02587.html Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/4] kvm: arm64: emulate ID registers @ 2020-08-13 14:19 ` Andrew Jones 0 siblings, 0 replies; 37+ messages in thread From: Andrew Jones @ 2020-08-13 14:19 UTC (permalink / raw) To: Peng Liang; +Cc: kvmarm, zhang.zhanghailiang, kvm, maz, will On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote: > In AArch64, guest will read the same values of the ID regsiters with > host. Both of them read the values from arm64_ftr_regs. This patch > series add support to emulate and configure ID registers so that we can > control the value of ID registers that guest read. > > Peng Liang (4): > arm64: add a helper function to traverse arm64_ftr_regs > kvm: arm64: emulate the ID registers > kvm: arm64: make ID registers configurable > kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension > > arch/arm64/include/asm/cpufeature.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kernel/cpufeature.c | 13 ++++++++ > arch/arm64/kvm/arm.c | 21 ++++++++++++ > arch/arm64/kvm/sys_regs.c | 50 ++++++++++++++++++++++------- > include/uapi/linux/kvm.h | 12 +++++++ > 6 files changed, 89 insertions(+), 11 deletions(-) > > -- > 2.18.4 > Hi Peng, After having looked at the QEMU series I don't believe this is the right approach to CPU models. As stated in my reply [*] this approach appears to just be shifting the problem wholesale to the user. On the KVM side, I think we should start by auditing all the ID registers to see what should be masked and what should be under user control. Then, we need to consider what MIDR a guest that can migrate between hosts of different MIDRs should have. And that'll require addressing the errata problem in one way or another. [*] https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg02587.html Thanks, drew ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2020-08-14 12:51 UTC | newest] Thread overview: 37+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-13 6:05 [RFC 0/4] kvm: arm64: emulate ID registers Peng Liang 2020-08-13 6:05 ` Peng Liang 2020-08-13 6:05 ` [RFC 1/4] arm64: add a helper function to traverse arm64_ftr_regs Peng Liang 2020-08-13 6:05 ` Peng Liang 2020-08-13 6:05 ` [RFC 2/4] kvm: arm64: emulate the ID registers Peng Liang 2020-08-13 6:05 ` Peng Liang 2020-08-13 9:05 ` Andrew Jones 2020-08-13 9:05 ` Andrew Jones 2020-08-13 10:02 ` Andrew Jones 2020-08-13 10:02 ` Andrew Jones 2020-08-14 11:49 ` Peng Liang 2020-08-14 11:49 ` Peng Liang 2020-08-14 12:51 ` Andrew Jones 2020-08-14 12:51 ` Andrew Jones 2020-08-14 11:49 ` Peng Liang 2020-08-14 11:49 ` Peng Liang 2020-08-13 23:10 ` kernel test robot 2020-08-14 12:20 ` Marc Zyngier 2020-08-14 12:20 ` Marc Zyngier 2020-08-13 6:05 ` [RFC 3/4] kvm: arm64: make ID registers configurable Peng Liang 2020-08-13 6:05 ` Peng Liang 2020-08-13 9:09 ` Andrew Jones 2020-08-13 9:09 ` Andrew Jones 2020-08-14 11:49 ` Peng Liang 2020-08-14 11:49 ` Peng Liang 2020-08-13 9:52 ` Marc Zyngier 2020-08-13 9:52 ` Marc Zyngier 2020-08-13 6:05 ` [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension Peng Liang 2020-08-13 6:05 ` Peng Liang 2020-08-13 9:10 ` Andrew Jones 2020-08-13 9:10 ` Andrew Jones 2020-08-14 11:49 ` Peng Liang 2020-08-14 11:49 ` Peng Liang 2020-08-13 9:14 ` [RFC 0/4] kvm: arm64: emulate ID registers Andrew Jones 2020-08-13 9:14 ` Andrew Jones 2020-08-13 14:19 ` Andrew Jones 2020-08-13 14:19 ` Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.