| Message ID | 20230503171618.2020461-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support writable CPU ID registers from userspace | expand |
On Wed, 03 May 2023 18:16:14 +0100, Jing Zhang <jingzhangos@google.com> wrote: > > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers, > and save ID registers' sanitized value in the array at KVM_CREATE_VM. > Use the saved ones when ID registers are read by the guest or > userspace (via KVM_GET_ONE_REG). > > No functional change intended. > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++ > arch/arm64/kvm/arm.c | 1 + > arch/arm64/kvm/id_regs.c | 46 +++++++++++++++++++++++++------ > arch/arm64/kvm/sys_regs.c | 11 +++++++- > arch/arm64/kvm/sys_regs.h | 3 +- > 5 files changed, 69 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index bcd774d74f34..a7d4d9e093e3 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -177,6 +177,21 @@ struct kvm_smccc_features { > unsigned long vendor_hyp_bmap; > }; > > +/* > + * Emulated CPU ID registers per VM > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. > + * > + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. > + * Access to id regs are guarded by kvm_arch.config_lock. > + */ > +#define KVM_ARM_ID_REG_NUM 56 You already have this as part of patch #1 in another include file, and then move it here. Surely you can do that in one go. I'd also like it to be defined in terms of encodings, and not as a raw value. > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) > +#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)] Missing brackets around 'kvm'. > +struct kvm_idregs { > + u64 regs[KVM_ARM_ID_REG_NUM]; > +}; > + > typedef unsigned int pkvm_handle_t; > > struct kvm_protected_vm { > @@ -243,6 +258,9 @@ struct kvm_arch { > /* Hypercall features firmware registers' descriptor */ > struct kvm_smccc_features smccc_feat; > > + /* Emulated CPU ID registers */ > + struct kvm_idregs idregs; > + > /* > * For an untrusted host VM, 'pkvm.handle' is used to lookup > * the associated pKVM instance in the hypervisor. > @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > struct kvm_arm_copy_mte_tags *copy_tags); > > +void kvm_arm_init_id_regs(struct kvm *kvm); > + > /* Guest/host FPSIMD coordination helpers */ > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 4b2e16e696a8..e34744c36406 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > set_default_spectre(kvm); > kvm_arm_init_hypercalls(kvm); > + kvm_arm_init_id_regs(kvm); > > /* > * Initialise the default PMUver before there is a chance to > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 96b4c43a5100..e769223bcee2 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) > } > } > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ Why getting rid of this comment instead of moving it next to the (re-implemented) function? > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (sysreg_visible_as_raz(vcpu, r)) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = IDREG(vcpu->kvm, id); > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > +{ > + if (sysreg_visible_as_raz(vcpu, r)) > + return 0; > + > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); > +} > + > /* cpufeature ID register access trap handlers */ > > static bool access_id_reg(struct kvm_vcpu *vcpu, > @@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) > > return 1; > } > + > +/* > + * Set the guest's ID registers that are defined in id_reg_descs[] > + * with ID_SANITISED() to the host's sanitized value. > + */ > +void kvm_arm_init_id_regs(struct kvm *kvm) > +{ > + u64 val; > + u32 id; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > + id = reg_to_encoding(&id_reg_descs[i]); > + > + /* > + * Some hidden ID registers which are not in arm64_ftr_regs[] > + * would cause warnings from read_sanitised_ftr_reg(). > + * Skip those ID registers to avoid the warnings. > + */ > + if (id_reg_descs[i].visibility == raz_visibility) > + /* Hidden or reserved ID register */ > + continue; Are you sure? What about other visibility attributes that are normally evaluated at runtime? This may work as a short term hack, but I'm not sure this is the correct long-term solution... M.
Hi Marc, On Wed, May 17, 2023 at 12:41 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 03 May 2023 18:16:14 +0100, > Jing Zhang <jingzhangos@google.com> wrote: > > > > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers, > > and save ID registers' sanitized value in the array at KVM_CREATE_VM. > > Use the saved ones when ID registers are read by the guest or > > userspace (via KVM_GET_ONE_REG). > > > > No functional change intended. > > > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/id_regs.c | 46 +++++++++++++++++++++++++------ > > arch/arm64/kvm/sys_regs.c | 11 +++++++- > > arch/arm64/kvm/sys_regs.h | 3 +- > > 5 files changed, 69 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index bcd774d74f34..a7d4d9e093e3 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -177,6 +177,21 @@ struct kvm_smccc_features { > > unsigned long vendor_hyp_bmap; > > }; > > > > +/* > > + * Emulated CPU ID registers per VM > > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it > > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. > > + * > > + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. > > + * Access to id regs are guarded by kvm_arch.config_lock. > > + */ > > +#define KVM_ARM_ID_REG_NUM 56 > > You already have this as part of patch #1 in another include file, and > then move it here. Surely you can do that in one go. I'd also like it > to be defined in terms of encodings, and not as a raw value. Sure, will do. > > > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) > > +#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)] > > Missing brackets around 'kvm'. Thanks, will fix. > > > +struct kvm_idregs { > > + u64 regs[KVM_ARM_ID_REG_NUM]; > > +}; > > + > > typedef unsigned int pkvm_handle_t; > > > > struct kvm_protected_vm { > > @@ -243,6 +258,9 @@ struct kvm_arch { > > /* Hypercall features firmware registers' descriptor */ > > struct kvm_smccc_features smccc_feat; > > > > + /* Emulated CPU ID registers */ > > + struct kvm_idregs idregs; > > + > > /* > > * For an untrusted host VM, 'pkvm.handle' is used to lookup > > * the associated pKVM instance in the hypervisor. > > @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > struct kvm_arm_copy_mte_tags *copy_tags); > > > > +void kvm_arm_init_id_regs(struct kvm *kvm); > > + > > /* Guest/host FPSIMD coordination helpers */ > > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 4b2e16e696a8..e34744c36406 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > > > set_default_spectre(kvm); > > kvm_arm_init_hypercalls(kvm); > > + kvm_arm_init_id_regs(kvm); > > > > /* > > * Initialise the default PMUver before there is a chance to > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > > index 96b4c43a5100..e769223bcee2 100644 > > --- a/arch/arm64/kvm/id_regs.c > > +++ b/arch/arm64/kvm/id_regs.c > > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) > > } > > } > > > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > > Why getting rid of this comment instead of moving it next to the > (re-implemented) function? > Right, will move it to the re-implemented function. > > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > > +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (sysreg_visible_as_raz(vcpu, r)) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = IDREG(vcpu->kvm, id); > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > > +{ > > + if (sysreg_visible_as_raz(vcpu, r)) > > + return 0; > > + > > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); > > +} > > + > > /* cpufeature ID register access trap handlers */ > > > > static bool access_id_reg(struct kvm_vcpu *vcpu, > > @@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) > > > > return 1; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in id_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void kvm_arm_init_id_regs(struct kvm *kvm) > > +{ > > + u64 val; > > + u32 id; > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > > + id = reg_to_encoding(&id_reg_descs[i]); > > + > > + /* > > + * Some hidden ID registers which are not in arm64_ftr_regs[] > > + * would cause warnings from read_sanitised_ftr_reg(). > > + * Skip those ID registers to avoid the warnings. > > + */ > > + if (id_reg_descs[i].visibility == raz_visibility) > > + /* Hidden or reserved ID register */ > > + continue; > > Are you sure? What about other visibility attributes that are normally > evaluated at runtime? This may work as a short term hack, but I'm not > sure this is the correct long-term solution... Yes, this is a short term hack. It would be replaced by checking the reset() function of idregs in patch #5 in this series. > > M. > > -- > Without deviation from the norm, progress is not possible. Thanks, Jing
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index bcd774d74f34..a7d4d9e093e3 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -177,6 +177,21 @@ struct kvm_smccc_features { unsigned long vendor_hyp_bmap; }; +/* + * Emulated CPU ID registers per VM + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. + * + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. + * Access to id regs are guarded by kvm_arch.config_lock. + */ +#define KVM_ARM_ID_REG_NUM 56 +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) +#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)] +struct kvm_idregs { + u64 regs[KVM_ARM_ID_REG_NUM]; +}; + typedef unsigned int pkvm_handle_t; struct kvm_protected_vm { @@ -243,6 +258,9 @@ struct kvm_arch { /* Hypercall features firmware registers' descriptor */ struct kvm_smccc_features smccc_feat; + /* Emulated CPU ID registers */ + struct kvm_idregs idregs; + /* * For an untrusted host VM, 'pkvm.handle' is used to lookup * the associated pKVM instance in the hypervisor. @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, struct kvm_arm_copy_mte_tags *copy_tags); +void kvm_arm_init_id_regs(struct kvm *kvm); + /* Guest/host FPSIMD coordination helpers */ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 4b2e16e696a8..e34744c36406 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) set_default_spectre(kvm); kvm_arm_init_hypercalls(kvm); + kvm_arm_init_id_regs(kvm); /* * Initialise the default PMUver before there is a chance to diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c index 96b4c43a5100..e769223bcee2 100644 --- a/arch/arm64/kvm/id_regs.c +++ b/arch/arm64/kvm/id_regs.c @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) } } -/* Read a sanitised cpufeature ID register by sys_reg_desc */ -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) { - u32 id = reg_to_encoding(r); - u64 val; - - if (sysreg_visible_as_raz(vcpu, r)) - return 0; - - val = read_sanitised_ftr_reg(id); + u64 val = IDREG(vcpu->kvm, id); switch (id) { case SYS_ID_AA64PFR0_EL1: @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r return val; } +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) +{ + if (sysreg_visible_as_raz(vcpu, r)) + return 0; + + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); +} + /* cpufeature ID register access trap handlers */ static bool access_id_reg(struct kvm_vcpu *vcpu, @@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) return 1; } + +/* + * Set the guest's ID registers that are defined in id_reg_descs[] + * with ID_SANITISED() to the host's sanitized value. + */ +void kvm_arm_init_id_regs(struct kvm *kvm) +{ + u64 val; + u32 id; + int i; + + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { + id = reg_to_encoding(&id_reg_descs[i]); + + /* + * Some hidden ID registers which are not in arm64_ftr_regs[] + * would cause warnings from read_sanitised_ftr_reg(). + * Skip those ID registers to avoid the warnings. + */ + if (id_reg_descs[i].visibility == raz_visibility) + /* Hidden or reserved ID register */ + continue; + + val = read_sanitised_ftr_reg(id); + IDREG(kvm, id) = val; + } +} diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 8dba10696dac..2e7b17d14b4f 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -364,7 +364,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); + u64 val = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); u32 sr = reg_to_encoding(r); if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) { @@ -2967,5 +2967,14 @@ int __init kvm_sys_reg_table_init(void) for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++) invariant_sys_regs[i].reset(NULL, &invariant_sys_regs[i]); + /* Sanity check for ID registers */ + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { + if (!is_id_reg(reg_to_encoding(&id_reg_descs[i]))) { + kvm_err("Non-ID register(index:%d, name: %s) found in idregs table!\n", + i, id_reg_descs[i].name); + return -EINVAL; + } + } + return 0; } diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index 7ce546a8be60..e88fd77309b2 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -237,6 +237,7 @@ bool write_to_read_only(struct kvm_vcpu *vcpu, struct sys_reg_params *params, const struct sys_reg_desc *r); unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r); int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params); +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id); #define AA32(_x) .aarch32_map = AA32_##_x #define Op0(_x) .Op0 = _x @@ -251,6 +252,4 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params); CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)), \ Op2(sys_reg_Op2(reg)) -#define KVM_ARM_ID_REG_NUM 56 - #endif /* __ARM64_KVM_SYS_REGS_LOCAL_H__ */