| Message ID | 20220106042708.2869332-3-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On Thu, Jan 6, 2022 at 4:28 AM Reiji Watanabe <reijiw@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). > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 16 ++++++++ > arch/arm64/kvm/arm.c | 1 + > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- > 3 files changed, 62 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 2a5f7f38006f..c789a0137f58 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -102,6 +102,17 @@ struct kvm_s2_mmu { > struct kvm_arch_memory_slot { > }; > > +/* > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > + * where 0<=crm<8, 0<=op2<8. > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > +#define is_id_reg(id) \ > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ > + sys_reg_CRm(id) < 8) > + This is consistent with the Arm ARM "Table D12-2 System instruction encodings for non-Debug System register accesses". Minor nit, would it be better to have IDREG_IDX and is_id_reg in arch/arm64/kvm/sys_regs.h, since other similar and related ones are there? > struct kvm_arch { > struct kvm_s2_mmu mmu; > > @@ -137,6 +148,9 @@ struct kvm_arch { > > /* Memory Tagging Extension enabled for the guest */ > bool mte_enabled; > + > + /* ID registers for the guest. */ > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > }; > > struct kvm_vcpu_fault_info { > @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); > > set_default_spectre(kvm); > + set_default_id_regs(kvm); > > return ret; > out_free_stage2_pgd: > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index e3ec1a44f94d..80dc62f98ef0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -33,6 +33,8 @@ > > #include "trace.h" > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > + > /* > * All of this file is extremely similar to the ARM coproc.c, but the > * types are different. My gut feeling is that it should be pretty > @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > u32 sr = reg_to_encoding(r); > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > return true; > } > > -/* 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, bool raz) > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (raz) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, bool raz) > +{ > + u32 id = reg_to_encoding(r); > + > + return raz ? 0 : __read_id_reg(vcpu, id); > +} > + > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *r) > { > @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. > + * For now, these registers are immutable for userspace, so for set_id_reg() > + * we don't allow the effective value to be changed. > */ > static int __get_id_reg(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1237,7 +1238,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) Minor nit: why remove the const in this patch? This is required for a future patch but not for this one. Thanks, /fuad > { > @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > if (p->is_write) { > return ignore_write(vcpu, p); > } else { > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) > /* Clear all higher bits. */ > cache_levels &= (1 << (i*3))-1; > } > + > +/* > + * Set the guest's ID registers that are defined in sys_reg_descs[] > + * with ID_SANITISED() to the host's sanitized value. > + */ > +void set_default_id_regs(struct kvm *kvm) > +{ > + int i; > + u32 id; > + const struct sys_reg_desc *rd; > + u64 val; > + > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > + rd = &sys_reg_descs[i]; > + if (rd->access != access_id_reg) > + /* Not ID register, or hidden/reserved ID register */ > + continue; > + > + id = reg_to_encoding(rd); > + if (WARN_ON_ONCE(!is_id_reg(id))) > + /* Shouldn't happen */ > + continue; > + > + val = read_sanitised_ftr_reg(id); > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > + } > +} > -- > 2.34.1.448.ga2b2bfdf31-goog > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Wed, Jan 05, 2022 at 08:26:44PM -0800, Reiji Watanabe 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). > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 16 ++++++++ > arch/arm64/kvm/arm.c | 1 + > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- > 3 files changed, 62 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 2a5f7f38006f..c789a0137f58 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -102,6 +102,17 @@ struct kvm_s2_mmu { > struct kvm_arch_memory_slot { > }; > > +/* > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > + * where 0<=crm<8, 0<=op2<8. Is this observation based on this table? Table D12-2 System instruction encodings for non-Debug System register accesses in that case, it seems that the ID registers list might grow after crm=7, and as CRm has 4 bits, why not 16*8=128? > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > +#define is_id_reg(id) \ > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ > + sys_reg_CRm(id) < 8) > + > struct kvm_arch { > struct kvm_s2_mmu mmu; > > @@ -137,6 +148,9 @@ struct kvm_arch { > > /* Memory Tagging Extension enabled for the guest */ > bool mte_enabled; > + > + /* ID registers for the guest. */ > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > }; > > struct kvm_vcpu_fault_info { > @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); > > set_default_spectre(kvm); > + set_default_id_regs(kvm); > > return ret; > out_free_stage2_pgd: > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index e3ec1a44f94d..80dc62f98ef0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -33,6 +33,8 @@ > > #include "trace.h" > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > + > /* > * All of this file is extremely similar to the ARM coproc.c, but the > * types are different. My gut feeling is that it should be pretty > @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > u32 sr = reg_to_encoding(r); > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > return true; > } > > -/* 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, bool raz) > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (raz) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, bool raz) > +{ > + u32 id = reg_to_encoding(r); > + > + return raz ? 0 : __read_id_reg(vcpu, id); > +} > + > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *r) > { > @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. > + * For now, these registers are immutable for userspace, so for set_id_reg() > + * we don't allow the effective value to be changed. > */ > static int __get_id_reg(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1237,7 +1238,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) > { > @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > if (p->is_write) { > return ignore_write(vcpu, p); > } else { > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) > /* Clear all higher bits. */ > cache_levels &= (1 << (i*3))-1; > } > + > +/* > + * Set the guest's ID registers that are defined in sys_reg_descs[] > + * with ID_SANITISED() to the host's sanitized value. > + */ > +void set_default_id_regs(struct kvm *kvm) > +{ > + int i; > + u32 id; > + const struct sys_reg_desc *rd; > + u64 val; > + > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > + rd = &sys_reg_descs[i]; > + if (rd->access != access_id_reg) > + /* Not ID register, or hidden/reserved ID register */ > + continue; > + > + id = reg_to_encoding(rd); > + if (WARN_ON_ONCE(!is_id_reg(id))) > + /* Shouldn't happen */ > + continue; > + > + val = read_sanitised_ftr_reg(id); I'm a bit confused. Shouldn't the default+sanitized values already use arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > + } > +} > -- > 2.34.1.448.ga2b2bfdf31-goog >
Hi Ricardo, On Tue, Jan 25, 2022 at 9:22 PM Ricardo Koller <ricarkol@google.com> wrote: > > On Wed, Jan 05, 2022 at 08:26:44PM -0800, Reiji Watanabe 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). > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 16 ++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- > > 3 files changed, 62 insertions(+), 17 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 2a5f7f38006f..c789a0137f58 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -102,6 +102,17 @@ struct kvm_s2_mmu { > > struct kvm_arch_memory_slot { > > }; > > > > +/* > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > + * where 0<=crm<8, 0<=op2<8. > > Is this observation based on this table? > > Table D12-2 System instruction encodings for non-Debug System register accesses > in that case, it seems that the ID registers list might grow after > crm=7, and as CRm has 4 bits, why not 16*8=128? This is basically for registers that are already reserved as RAZ in the table (sys_reg_descs[] has entries for the reserved ones as well). Registers with crm > 7 are not reserved yet, and that will be expanded later as needed later. > > > + */ > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > +#define is_id_reg(id) \ > > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ > > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ > > + sys_reg_CRm(id) < 8) > > + > > struct kvm_arch { > > struct kvm_s2_mmu mmu; > > > > @@ -137,6 +148,9 @@ struct kvm_arch { > > > > /* Memory Tagging Extension enabled for the guest */ > > bool mte_enabled; > > + > > + /* ID registers for the guest. */ > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > }; > > > > struct kvm_vcpu_fault_info { > > @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); > > > > set_default_spectre(kvm); > > + set_default_id_regs(kvm); > > > > return ret; > > out_free_stage2_pgd: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index e3ec1a44f94d..80dc62f98ef0 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -33,6 +33,8 @@ > > > > #include "trace.h" > > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > > + > > /* > > * All of this file is extremely similar to the ARM coproc.c, but the > > * types are different. My gut feeling is that it should be pretty > > @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > > u32 sr = reg_to_encoding(r); > > > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > return true; > > } > > > > -/* 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, bool raz) > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (raz) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > + struct sys_reg_desc const *r, bool raz) > > +{ > > + u32 id = reg_to_encoding(r); > > + > > + return raz ? 0 : __read_id_reg(vcpu, id); > > +} > > + > > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *r) > > { > > @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. > > + * For now, these registers are immutable for userspace, so for set_id_reg() > > + * we don't allow the effective value to be changed. > > */ > > static int __get_id_reg(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, void __user *uaddr, > > @@ -1237,7 +1238,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) > > { > > @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > > if (p->is_write) { > > return ignore_write(vcpu, p); > > } else { > > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) > > /* Clear all higher bits. */ > > cache_levels &= (1 << (i*3))-1; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void set_default_id_regs(struct kvm *kvm) > > +{ > > + int i; > > + u32 id; > > + const struct sys_reg_desc *rd; > > + u64 val; > > + > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > + rd = &sys_reg_descs[i]; > > + if (rd->access != access_id_reg) > > + /* Not ID register, or hidden/reserved ID register */ > > + continue; > > + > > + id = reg_to_encoding(rd); > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > + /* Shouldn't happen */ > > + continue; > > + > > + val = read_sanitised_ftr_reg(id); > > I'm a bit confused. Shouldn't the default+sanitized values already use > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? I'm not sure if I understand your question. arm64_ftr_bits_kvm is used for feature support checkings when userspace tries to modify a value of ID registers. With this patch, KVM just saves the sanitized values in the kvm's buffer, but userspace is still not allowed to modify values of ID registers yet. I hope it answers your question. Thanks, Reiji > > > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > > + } > > +} > > -- > > 2.34.1.448.ga2b2bfdf31-goog > >
On Thu, Jan 27, 2022 at 10:24 PM Reiji Watanabe <reijiw@google.com> wrote: > > Hi Ricardo, > > On Tue, Jan 25, 2022 at 9:22 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > On Wed, Jan 05, 2022 at 08:26:44PM -0800, Reiji Watanabe 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). > > > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > --- > > > arch/arm64/include/asm/kvm_host.h | 16 ++++++++ > > > arch/arm64/kvm/arm.c | 1 + > > > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- > > > 3 files changed, 62 insertions(+), 17 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > > index 2a5f7f38006f..c789a0137f58 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -102,6 +102,17 @@ struct kvm_s2_mmu { > > > struct kvm_arch_memory_slot { > > > }; > > > > > > +/* > > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > > + * where 0<=crm<8, 0<=op2<8. > > > > Is this observation based on this table? > > > > Table D12-2 System instruction encodings for non-Debug System register accesses > > in that case, it seems that the ID registers list might grow after > > crm=7, and as CRm has 4 bits, why not 16*8=128? > > This is basically for registers that are already reserved as RAZ in the > table (sys_reg_descs[] has entries for the reserved ones as well). > Registers with crm > 7 are not reserved yet, and that will be expanded > later as needed later. > > > > > > > + */ > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > +#define is_id_reg(id) \ > > > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ > > > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ > > > + sys_reg_CRm(id) < 8) > > > + > > > struct kvm_arch { > > > struct kvm_s2_mmu mmu; > > > > > > @@ -137,6 +148,9 @@ struct kvm_arch { > > > > > > /* Memory Tagging Extension enabled for the guest */ > > > bool mte_enabled; > > > + > > > + /* ID registers for the guest. */ > > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > > }; > > > > > > struct kvm_vcpu_fault_info { > > > @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 > > > --- a/arch/arm64/kvm/arm.c > > > +++ b/arch/arm64/kvm/arm.c > > > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); > > > > > > set_default_spectre(kvm); > > > + set_default_id_regs(kvm); > > > > > > return ret; > > > out_free_stage2_pgd: > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index e3ec1a44f94d..80dc62f98ef0 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -33,6 +33,8 @@ > > > > > > #include "trace.h" > > > > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > > > + > > > /* > > > * All of this file is extremely similar to the ARM coproc.c, but the > > > * types are different. My gut feeling is that it should be pretty > > > @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > > > u32 sr = reg_to_encoding(r); > > > > > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > > > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > > return true; > > > } > > > > > > -/* 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, bool raz) > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > > { > > > - u32 id = reg_to_encoding(r); > > > - u64 val; > > > - > > > - if (raz) > > > - return 0; > > > - > > > - val = read_sanitised_ftr_reg(id); > > > + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > > > > > switch (id) { > > > case SYS_ID_AA64PFR0_EL1: > > > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > > return val; > > > } > > > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > > + struct sys_reg_desc const *r, bool raz) > > > +{ > > > + u32 id = reg_to_encoding(r); > > > + > > > + return raz ? 0 : __read_id_reg(vcpu, id); > > > +} > > > + > > > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > > const struct sys_reg_desc *r) > > > { > > > @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. > > > + * For now, these registers are immutable for userspace, so for set_id_reg() > > > + * we don't allow the effective value to be changed. > > > */ > > > static int __get_id_reg(const struct kvm_vcpu *vcpu, > > > const struct sys_reg_desc *rd, void __user *uaddr, > > > @@ -1237,7 +1238,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) > > > { > > > @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > > > if (p->is_write) { > > > return ignore_write(vcpu, p); > > > } else { > > > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > > > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > > > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > > > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > > > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > > > > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > > > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) > > > /* Clear all higher bits. */ > > > cache_levels &= (1 << (i*3))-1; > > > } > > > + > > > +/* > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > + * with ID_SANITISED() to the host's sanitized value. > > > + */ > > > +void set_default_id_regs(struct kvm *kvm) > > > +{ > > > + int i; > > > + u32 id; > > > + const struct sys_reg_desc *rd; > > > + u64 val; > > > + > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > + rd = &sys_reg_descs[i]; > > > + if (rd->access != access_id_reg) > > > + /* Not ID register, or hidden/reserved ID register */ > > > + continue; > > > + > > > + id = reg_to_encoding(rd); > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > + /* Shouldn't happen */ > > > + continue; > > > + > > > + val = read_sanitised_ftr_reg(id); > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > I'm not sure if I understand your question. > arm64_ftr_bits_kvm is used for feature support checkings when > userspace tries to modify a value of ID registers. > With this patch, KVM just saves the sanitized values in the kvm's > buffer, but userspace is still not allowed to modify values of ID > registers yet. > I hope it answers your question. Based on the previous commit I was assuming that some registers, like id_aa64dfr0, would default to the overwritten values as the sanitized values. More specifically: if userspace doesn't modify any ID reg, shouldn't the defaults have the KVM overwritten values (arm64_ftr_bits_kvm)? > > Thanks, > Reiji > > > > > > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > > > + } > > > +} > > > -- > > > 2.34.1.448.ga2b2bfdf31-goog > > >
Hi Ricardo, > > > > + > > > > +/* > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > + */ > > > > +void set_default_id_regs(struct kvm *kvm) > > > > +{ > > > > + int i; > > > > + u32 id; > > > > + const struct sys_reg_desc *rd; > > > > + u64 val; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > + rd = &sys_reg_descs[i]; > > > > + if (rd->access != access_id_reg) > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > + continue; > > > > + > > > > + id = reg_to_encoding(rd); > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > + /* Shouldn't happen */ > > > > + continue; > > > > + > > > > + val = read_sanitised_ftr_reg(id); > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > I'm not sure if I understand your question. > > arm64_ftr_bits_kvm is used for feature support checkings when > > userspace tries to modify a value of ID registers. > > With this patch, KVM just saves the sanitized values in the kvm's > > buffer, but userspace is still not allowed to modify values of ID > > registers yet. > > I hope it answers your question. > > Based on the previous commit I was assuming that some registers, like > id_aa64dfr0, > would default to the overwritten values as the sanitized values. More > specifically: if > userspace doesn't modify any ID reg, shouldn't the defaults have the > KVM overwritten > values (arm64_ftr_bits_kvm)? arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, and arm64_ftr_bits_kvm doesn't have the sanitized values. Thanks, Reiji
On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote: > Hi Ricardo, > > > > > > + > > > > > +/* > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > > + */ > > > > > +void set_default_id_regs(struct kvm *kvm) > > > > > +{ > > > > > + int i; > > > > > + u32 id; > > > > > + const struct sys_reg_desc *rd; > > > > > + u64 val; > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > > + rd = &sys_reg_descs[i]; > > > > > + if (rd->access != access_id_reg) > > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > > + continue; > > > > > + > > > > > + id = reg_to_encoding(rd); > > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > > + /* Shouldn't happen */ > > > > > + continue; > > > > > + > > > > > + val = read_sanitised_ftr_reg(id); > > > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > > > I'm not sure if I understand your question. > > > arm64_ftr_bits_kvm is used for feature support checkings when > > > userspace tries to modify a value of ID registers. > > > With this patch, KVM just saves the sanitized values in the kvm's > > > buffer, but userspace is still not allowed to modify values of ID > > > registers yet. > > > I hope it answers your question. > > > > Based on the previous commit I was assuming that some registers, like > > id_aa64dfr0, > > would default to the overwritten values as the sanitized values. More > > specifically: if > > userspace doesn't modify any ID reg, shouldn't the defaults have the > > KVM overwritten > > values (arm64_ftr_bits_kvm)? > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, > and arm64_ftr_bits_kvm doesn't have the sanitized values. > > Thanks, Hey Reiji, Sorry, I wasn't very clear. This is what I meant. If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the series: static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5), it means that userspace would not be able to set DEBUGVER to anything but 0x5. But I'm not sure what it should mean for the default KVM value of DEBUGVER, specifically the value calculated in set_default_id_regs(). As it is, KVM is still setting the guest-visible value to 0x6, and my "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I booted a VM and the DEBUGVER value from inside is still 0x6. I was expecting it to not boot, or to show a warning. I think this has some implications for migrations. It would not be possible to migrate the example VM on the patched kernel from above: you can boot a VM with DEBUGVER=0x5 but you can't migrate it. Thanks, Ricardo
Hi Ricardo, On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@google.com> wrote: > > On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote: > > Hi Ricardo, > > > > > > > > + > > > > > > +/* > > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > > > + */ > > > > > > +void set_default_id_regs(struct kvm *kvm) > > > > > > +{ > > > > > > + int i; > > > > > > + u32 id; > > > > > > + const struct sys_reg_desc *rd; > > > > > > + u64 val; > > > > > > + > > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > > > + rd = &sys_reg_descs[i]; > > > > > > + if (rd->access != access_id_reg) > > > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > > > + continue; > > > > > > + > > > > > > + id = reg_to_encoding(rd); > > > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > > > + /* Shouldn't happen */ > > > > > > + continue; > > > > > > + > > > > > > + val = read_sanitised_ftr_reg(id); > > > > > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > > > > > I'm not sure if I understand your question. > > > > arm64_ftr_bits_kvm is used for feature support checkings when > > > > userspace tries to modify a value of ID registers. > > > > With this patch, KVM just saves the sanitized values in the kvm's > > > > buffer, but userspace is still not allowed to modify values of ID > > > > registers yet. > > > > I hope it answers your question. > > > > > > Based on the previous commit I was assuming that some registers, like > > > id_aa64dfr0, > > > would default to the overwritten values as the sanitized values. More > > > specifically: if > > > userspace doesn't modify any ID reg, shouldn't the defaults have the > > > KVM overwritten > > > values (arm64_ftr_bits_kvm)? > > > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, > > and arm64_ftr_bits_kvm doesn't have the sanitized values. > > > > Thanks, > > Hey Reiji, > > Sorry, I wasn't very clear. This is what I meant. > > If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the > series: > > static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5), > > it means that userspace would not be able to set DEBUGVER to anything > but 0x5. But I'm not sure what it should mean for the default KVM value > of DEBUGVER, specifically the value calculated in set_default_id_regs(). > As it is, KVM is still setting the guest-visible value to 0x6, and my > "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I > booted a VM and the DEBUGVER value from inside is still 0x6. I was > expecting it to not boot, or to show a warning. Thank you for the explanation! FTR_EXACT (in the existing code) means that the safe_val should be used if values of the field are not identical between CPUs (see how update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage, it means that if the field value for a vCPU is different from the one for the host's sanitized value, only the safe_val can be used safely for the guest (purely in terms of CPU feature). If KVM wants to restrict some features due to some reasons (e.g. a feature for guests is not supported by the KVM yet), it should be done by KVM (not by cpufeature.c), and 'validate' function in "struct id_reg_info", which is introduced in patch-3, will be used for such cases (the following patches actually use). Thanks, Reiji > > I think this has some implications for migrations. It would not be > possible to migrate the example VM on the patched kernel from above: you > can boot a VM with DEBUGVER=0x5 but you can't migrate it. > > Thanks, > Ricardo
Hey Reiji, On Mon, Jan 31, 2022 at 10:00:40PM -0800, Reiji Watanabe wrote: > Hi Ricardo, > > On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote: > > > Hi Ricardo, > > > > > > > > > > + > > > > > > > +/* > > > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > > > > + */ > > > > > > > +void set_default_id_regs(struct kvm *kvm) > > > > > > > +{ > > > > > > > + int i; > > > > > > > + u32 id; > > > > > > > + const struct sys_reg_desc *rd; > > > > > > > + u64 val; > > > > > > > + > > > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > > > > + rd = &sys_reg_descs[i]; > > > > > > > + if (rd->access != access_id_reg) > > > > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > > > > + continue; > > > > > > > + > > > > > > > + id = reg_to_encoding(rd); > > > > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > > > > + /* Shouldn't happen */ > > > > > > > + continue; > > > > > > > + > > > > > > > + val = read_sanitised_ftr_reg(id); > > > > > > > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > > > > > > > I'm not sure if I understand your question. > > > > > arm64_ftr_bits_kvm is used for feature support checkings when > > > > > userspace tries to modify a value of ID registers. > > > > > With this patch, KVM just saves the sanitized values in the kvm's > > > > > buffer, but userspace is still not allowed to modify values of ID > > > > > registers yet. > > > > > I hope it answers your question. > > > > > > > > Based on the previous commit I was assuming that some registers, like > > > > id_aa64dfr0, > > > > would default to the overwritten values as the sanitized values. More > > > > specifically: if > > > > userspace doesn't modify any ID reg, shouldn't the defaults have the > > > > KVM overwritten > > > > values (arm64_ftr_bits_kvm)? > > > > > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, > > > and arm64_ftr_bits_kvm doesn't have the sanitized values. > > > > > > Thanks, > > > > Hey Reiji, > > > > Sorry, I wasn't very clear. This is what I meant. > > > > If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the > > series: > > > > static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), > > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), > > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5), > > > > it means that userspace would not be able to set DEBUGVER to anything > > but 0x5. But I'm not sure what it should mean for the default KVM value > > of DEBUGVER, specifically the value calculated in set_default_id_regs(). > > As it is, KVM is still setting the guest-visible value to 0x6, and my > > "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I > > booted a VM and the DEBUGVER value from inside is still 0x6. I was > > expecting it to not boot, or to show a warning. > > Thank you for the explanation! > > FTR_EXACT (in the existing code) means that the safe_val should be > used if values of the field are not identical between CPUs (see how > update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage, > it means that if the field value for a vCPU is different from the one > for the host's sanitized value, only the safe_val can be used safely > for the guest (purely in terms of CPU feature). Let me double check my understanding using the DEBUGVER example, please. The safe_value would be DEBUGVER=5, and it contradicts the initial VM value calculated on the KVM side. Q1: Can a contradiction like this occur in practice? Q2: If the user saves and restores this id-reg on the same kernel, the AA64DFR0 userspace write would fail (ftr_val != arm64_ftr_safe_value), right? > > If KVM wants to restrict some features due to some reasons (e.g. > a feature for guests is not supported by the KVM yet), it should > be done by KVM (not by cpufeature.c), and 'validate' function in > "struct id_reg_info", which is introduced in patch-3, will be used > for such cases (the following patches actually use). > Got it, thanks. > Thanks, > Reiji > Thanks, Ricardo > > > > I think this has some implications for migrations. It would not be > > possible to migrate the example VM on the patched kernel from above: you > > can boot a VM with DEBUGVER=0x5 but you can't migrate it. > > > > Thanks, > > Ricardo
Hi Ricardo, On Tue, Feb 1, 2022 at 10:39 AM Ricardo Koller <ricarkol@google.com> wrote: > > Hey Reiji, > > On Mon, Jan 31, 2022 at 10:00:40PM -0800, Reiji Watanabe wrote: > > Hi Ricardo, > > > > On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > > > On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote: > > > > Hi Ricardo, > > > > > > > > > > > > + > > > > > > > > +/* > > > > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > > > > > + */ > > > > > > > > +void set_default_id_regs(struct kvm *kvm) > > > > > > > > +{ > > > > > > > > + int i; > > > > > > > > + u32 id; > > > > > > > > + const struct sys_reg_desc *rd; > > > > > > > > + u64 val; > > > > > > > > + > > > > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > > > > > + rd = &sys_reg_descs[i]; > > > > > > > > + if (rd->access != access_id_reg) > > > > > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > > > > > + continue; > > > > > > > > + > > > > > > > > + id = reg_to_encoding(rd); > > > > > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > > > > > + /* Shouldn't happen */ > > > > > > > > + continue; > > > > > > > > + > > > > > > > > + val = read_sanitised_ftr_reg(id); > > > > > > > > > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > > > > > > > > > I'm not sure if I understand your question. > > > > > > arm64_ftr_bits_kvm is used for feature support checkings when > > > > > > userspace tries to modify a value of ID registers. > > > > > > With this patch, KVM just saves the sanitized values in the kvm's > > > > > > buffer, but userspace is still not allowed to modify values of ID > > > > > > registers yet. > > > > > > I hope it answers your question. > > > > > > > > > > Based on the previous commit I was assuming that some registers, like > > > > > id_aa64dfr0, > > > > > would default to the overwritten values as the sanitized values. More > > > > > specifically: if > > > > > userspace doesn't modify any ID reg, shouldn't the defaults have the > > > > > KVM overwritten > > > > > values (arm64_ftr_bits_kvm)? > > > > > > > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, > > > > and arm64_ftr_bits_kvm doesn't have the sanitized values. > > > > > > > > Thanks, > > > > > > Hey Reiji, > > > > > > Sorry, I wasn't very clear. This is what I meant. > > > > > > If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the > > > series: > > > > > > static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), > > > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), > > > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5), > > > > > > it means that userspace would not be able to set DEBUGVER to anything > > > but 0x5. But I'm not sure what it should mean for the default KVM value > > > of DEBUGVER, specifically the value calculated in set_default_id_regs(). > > > As it is, KVM is still setting the guest-visible value to 0x6, and my > > > "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I > > > booted a VM and the DEBUGVER value from inside is still 0x6. I was > > > expecting it to not boot, or to show a warning. > > > > Thank you for the explanation! > > > > FTR_EXACT (in the existing code) means that the safe_val should be > > used if values of the field are not identical between CPUs (see how > > update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage, > > it means that if the field value for a vCPU is different from the one > > for the host's sanitized value, only the safe_val can be used safely > > for the guest (purely in terms of CPU feature). > > Let me double check my understanding using the DEBUGVER example, please. > The safe_value would be DEBUGVER=5, and it contradicts the initial VM > value calculated on the KVM side. Q1: Can a contradiction like this > occur in practice? Q2: If the user saves and restores this id-reg on the > same kernel, the AA64DFR0 userspace write would fail (ftr_val != > arm64_ftr_safe_value), right? Thank you for the comment! For Q1, yes, we might possibly create a bug that makes a contradiction between KVM and cpufeature.c. For Q2, even with such a contradiction, userspace will still be able to save and restore the id reg on the same kernel on the same system in most cases because @limit that KVM will specify for arm64_check_features() will mostly be the same as the initial value for the guest (except for fields corresponding to opt-in CPU features, which are configured with KVM_ARM_VCPU_INIT or etc) and arm64_check_features does an equality check per field. Having said that, as you suggested, it might be better to run arm64_check_features for the initial value against the host value so we can catch such a bug. I'll look into doing that in v5. Thanks, Reiji
On Wed, Feb 02, 2022 at 10:31:26PM -0800, Reiji Watanabe wrote: > Hi Ricardo, > > On Tue, Feb 1, 2022 at 10:39 AM Ricardo Koller <ricarkol@google.com> wrote: > > > > Hey Reiji, > > > > On Mon, Jan 31, 2022 at 10:00:40PM -0800, Reiji Watanabe wrote: > > > Hi Ricardo, > > > > > > On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > > > > > On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote: > > > > > Hi Ricardo, > > > > > > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > > > > > > > > + * with ID_SANITISED() to the host's sanitized value. > > > > > > > > > + */ > > > > > > > > > +void set_default_id_regs(struct kvm *kvm) > > > > > > > > > +{ > > > > > > > > > + int i; > > > > > > > > > + u32 id; > > > > > > > > > + const struct sys_reg_desc *rd; > > > > > > > > > + u64 val; > > > > > > > > > + > > > > > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > > > > > > > > + rd = &sys_reg_descs[i]; > > > > > > > > > + if (rd->access != access_id_reg) > > > > > > > > > + /* Not ID register, or hidden/reserved ID register */ > > > > > > > > > + continue; > > > > > > > > > + > > > > > > > > > + id = reg_to_encoding(rd); > > > > > > > > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > > > > > > > > + /* Shouldn't happen */ > > > > > > > > > + continue; > > > > > > > > > + > > > > > > > > > + val = read_sanitised_ftr_reg(id); > > > > > > > > > > > > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use > > > > > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)? > > > > > > > > > > > > > > I'm not sure if I understand your question. > > > > > > > arm64_ftr_bits_kvm is used for feature support checkings when > > > > > > > userspace tries to modify a value of ID registers. > > > > > > > With this patch, KVM just saves the sanitized values in the kvm's > > > > > > > buffer, but userspace is still not allowed to modify values of ID > > > > > > > registers yet. > > > > > > > I hope it answers your question. > > > > > > > > > > > > Based on the previous commit I was assuming that some registers, like > > > > > > id_aa64dfr0, > > > > > > would default to the overwritten values as the sanitized values. More > > > > > > specifically: if > > > > > > userspace doesn't modify any ID reg, shouldn't the defaults have the > > > > > > KVM overwritten > > > > > > values (arm64_ftr_bits_kvm)? > > > > > > > > > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits, > > > > > and arm64_ftr_bits_kvm doesn't have the sanitized values. > > > > > > > > > > Thanks, > > > > > > > > Hey Reiji, > > > > > > > > Sorry, I wasn't very clear. This is what I meant. > > > > > > > > If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the > > > > series: > > > > > > > > static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { > > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), > > > > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), > > > > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5), > > > > > > > > it means that userspace would not be able to set DEBUGVER to anything > > > > but 0x5. But I'm not sure what it should mean for the default KVM value > > > > of DEBUGVER, specifically the value calculated in set_default_id_regs(). > > > > As it is, KVM is still setting the guest-visible value to 0x6, and my > > > > "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I > > > > booted a VM and the DEBUGVER value from inside is still 0x6. I was > > > > expecting it to not boot, or to show a warning. > > > > > > Thank you for the explanation! > > > > > > FTR_EXACT (in the existing code) means that the safe_val should be > > > used if values of the field are not identical between CPUs (see how > > > update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage, > > > it means that if the field value for a vCPU is different from the one > > > for the host's sanitized value, only the safe_val can be used safely > > > for the guest (purely in terms of CPU feature). > > > > Let me double check my understanding using the DEBUGVER example, please. > > The safe_value would be DEBUGVER=5, and it contradicts the initial VM > > value calculated on the KVM side. Q1: Can a contradiction like this > > occur in practice? Q2: If the user saves and restores this id-reg on the > > same kernel, the AA64DFR0 userspace write would fail (ftr_val != > > arm64_ftr_safe_value), right? > > Thank you for the comment! > > For Q1, yes, we might possibly create a bug that makes a contradiction > between KVM and cpufeature.c. > For Q2, even with such a contradiction, userspace will still be able to > save and restore the id reg on the same kernel on the same system in most > cases because @limit that KVM will specify for arm64_check_features() > will mostly be the same as the initial value for the guest (except for > fields corresponding to opt-in CPU features, which are configured with > KVM_ARM_VCPU_INIT or etc) and arm64_check_features does an equality check > per field. Having said that, as you suggested, it might be better to run > arm64_check_features for the initial value against the host value so we > can catch such a bug. I'll look into doing that in v5. > Thanks Reiji. Looking forward to v5. > Thanks, > Reiji
Hi Fuad, Sorry for the late reply. I've noticed that I didn't reply to the comments in this mail. On Mon, Jan 24, 2022 at 8:22 AM Fuad Tabba <tabba@google.com> wrote: > > Hi Reiji, > > On Thu, Jan 6, 2022 at 4:28 AM Reiji Watanabe <reijiw@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). > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 16 ++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- > > 3 files changed, 62 insertions(+), 17 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 2a5f7f38006f..c789a0137f58 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -102,6 +102,17 @@ struct kvm_s2_mmu { > > struct kvm_arch_memory_slot { > > }; > > > > +/* > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > + * where 0<=crm<8, 0<=op2<8. > > + */ > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > +#define is_id_reg(id) \ > > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ > > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ > > + sys_reg_CRm(id) < 8) > > + > > This is consistent with the Arm ARM "Table D12-2 System instruction > encodings for non-Debug System register accesses". > > Minor nit, would it be better to have IDREG_IDX and is_id_reg in > arch/arm64/kvm/sys_regs.h, since other similar and related ones are > there? I will move is_id_reg in sys_regs.c as it is used only in the file. I will keep IDREG_IDX in arch/arm64/kvm/sys_regs.h as IDREG_IDX is used to get an index of kvm_arch.id_regs[], which is defined in kvm_host.h. > > > struct kvm_arch { > > struct kvm_s2_mmu mmu; > > > > @@ -137,6 +148,9 @@ struct kvm_arch { > > > > /* Memory Tagging Extension enabled for the guest */ > > bool mte_enabled; > > + > > + /* ID registers for the guest. */ > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > }; > > > > struct kvm_vcpu_fault_info { > > @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); > > > > set_default_spectre(kvm); > > + set_default_id_regs(kvm); > > > > return ret; > > out_free_stage2_pgd: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index e3ec1a44f94d..80dc62f98ef0 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -33,6 +33,8 @@ > > > > #include "trace.h" > > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > > + > > /* > > * All of this file is extremely similar to the ARM coproc.c, but the > > * types are different. My gut feeling is that it should be pretty > > @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > > u32 sr = reg_to_encoding(r); > > > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > return true; > > } > > > > -/* 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, bool raz) > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (raz) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > + struct sys_reg_desc const *r, bool raz) > > +{ > > + u32 id = reg_to_encoding(r); > > + > > + return raz ? 0 : __read_id_reg(vcpu, id); > > +} > > + > > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *r) > > { > > @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. > > + * For now, these registers are immutable for userspace, so for set_id_reg() > > + * we don't allow the effective value to be changed. > > */ > > static int __get_id_reg(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, void __user *uaddr, > > @@ -1237,7 +1238,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) > > Minor nit: why remove the const in this patch? This is required for a > future patch but not for this one. Thank you for catching this. I will fix this. Regards, Reiji > > > > { > > @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > > if (p->is_write) { > > return ignore_write(vcpu, p); > > } else { > > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) > > /* Clear all higher bits. */ > > cache_levels &= (1 << (i*3))-1; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in sys_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void set_default_id_regs(struct kvm *kvm) > > +{ > > + int i; > > + u32 id; > > + const struct sys_reg_desc *rd; > > + u64 val; > > + > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > + rd = &sys_reg_descs[i]; > > + if (rd->access != access_id_reg) > > + /* Not ID register, or hidden/reserved ID register */ > > + continue; > > + > > + id = reg_to_encoding(rd); > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > + /* Shouldn't happen */ > > + continue; > > + > > + val = read_sanitised_ftr_reg(id); > > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > > + } > > +} > > -- > > 2.34.1.448.ga2b2bfdf31-goog > > > > _______________________________________________ > > kvmarm mailing list > > kvmarm@lists.cs.columbia.edu > > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 2a5f7f38006f..c789a0137f58 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -102,6 +102,17 @@ struct kvm_s2_mmu { struct kvm_arch_memory_slot { }; +/* + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), + * where 0<=crm<8, 0<=op2<8. + */ +#define KVM_ARM_ID_REG_MAX_NUM 64 +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) +#define is_id_reg(id) \ + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \ + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \ + sys_reg_CRm(id) < 8) + struct kvm_arch { struct kvm_s2_mmu mmu; @@ -137,6 +148,9 @@ struct kvm_arch { /* Memory Tagging Extension enabled for the guest */ bool mte_enabled; + + /* ID registers for the guest. */ + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; }; struct kvm_vcpu_fault_info { @@ -734,6 +748,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 set_default_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 e4727dc771bf..5f497a0af254 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) kvm->arch.max_vcpus = kvm_arm_default_max_vcpus(); set_default_spectre(kvm); + set_default_id_regs(kvm); return ret; out_free_stage2_pgd: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index e3ec1a44f94d..80dc62f98ef0 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -33,6 +33,8 @@ #include "trace.h" +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); + /* * All of this file is extremely similar to the ARM coproc.c, but the * types are different. My gut feeling is that it should be pretty @@ -273,7 +275,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 = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); u32 sr = reg_to_encoding(r); if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, return true; } -/* 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, bool raz) +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) { - u32 id = reg_to_encoding(r); - u64 val; - - if (raz) - return 0; - - val = read_sanitised_ftr_reg(id); + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; switch (id) { case SYS_ID_AA64PFR0_EL1: @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, return val; } +static u64 read_id_reg(const struct kvm_vcpu *vcpu, + struct sys_reg_desc const *r, bool raz) +{ + u32 id = reg_to_encoding(r); + + return raz ? 0 : __read_id_reg(vcpu, id); +} + static unsigned int id_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { @@ -1223,9 +1225,8 @@ static int set_id_aa64pfr0_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. + * For now, these registers are immutable for userspace, so for set_id_reg() + * we don't allow the effective value to be changed. */ static int __get_id_reg(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, void __user *uaddr, @@ -1237,7 +1238,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) { @@ -1837,8 +1838,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, if (p->is_write) { return ignore_write(vcpu, p); } else { - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void) /* Clear all higher bits. */ cache_levels &= (1 << (i*3))-1; } + +/* + * Set the guest's ID registers that are defined in sys_reg_descs[] + * with ID_SANITISED() to the host's sanitized value. + */ +void set_default_id_regs(struct kvm *kvm) +{ + int i; + u32 id; + const struct sys_reg_desc *rd; + u64 val; + + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { + rd = &sys_reg_descs[i]; + if (rd->access != access_id_reg) + /* Not ID register, or hidden/reserved ID register */ + continue; + + id = reg_to_encoding(rd); + if (WARN_ON_ONCE(!is_id_reg(id))) + /* Shouldn't happen */ + continue; + + val = read_sanitised_ftr_reg(id); + kvm->arch.id_regs[IDREG_IDX(id)] = val; + } +}
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). Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/kvm_host.h | 16 ++++++++ arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++--------- 3 files changed, 62 insertions(+), 17 deletions(-)