| Message ID | 20220311044811.1980336-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, Mar 10, 2022 at 08:47:48PM -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 | 12 ++++++ > arch/arm64/kvm/arm.c | 1 + > arch/arm64/kvm/sys_regs.c | 65 ++++++++++++++++++++++++------- > 3 files changed, 63 insertions(+), 15 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 2869259e10c0..c041e5afe3d2 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -101,6 +101,13 @@ 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. Doesn't the Feature ID register scheme only apply to CRm={1-7}, op2={0-7}? I believe CRm=0, op2={1-4,7} are in fact UNDEFINED, not RAZ like the other ranges. Furthermore, the registers that are defined in that range do not go through the read_id_reg() plumbing. > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > + > struct kvm_arch { > struct kvm_s2_mmu mmu; > > @@ -137,6 +144,9 @@ struct kvm_arch { > /* Memory Tagging Extension enabled for the guest */ > bool mte_enabled; > bool ran_once; > + > + /* ID registers for the guest. */ > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; This is a decently large array. Should we embed it in kvm_arch or allocate at init? [...] > + > +/* > + * 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) nit, more relevant if you take the above suggestion: maybe call it kvm_init_id_regs()? > +{ > + int i; > + u32 id; > + const struct sys_reg_desc *rd; > + u64 val; > + > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { You could avoid walking the entire system register table, since we already know the start and end values for the Feature ID register range. maybe: #define FEATURE_ID_RANGE_START SYS_ID_PFR0_EL1 #define FEATURE_ID_RANGE_END sys_reg(3, 0, 0, 7, 7) u32 sys_reg; for (sys_reg = FEATURE_ID_RANGE_START; sys_reg <= FEATURE_ID_RANGE_END; sys_reg++) But, it depends on if this check is necessary: > + rd = &sys_reg_descs[i]; > + if (rd->access != access_id_reg) > + /* Not ID register, or hidden/reserved ID register */ > + continue; Which itself is dependent on whether KVM is going to sparsely or verbosely define its feature filtering tables per the other thread. So really only bother with this if that is the direction you're going. -- Thanks, Oliver
Hi Oliver, On 3/23/22 12:22 PM, Oliver Upton wrote: > Hi Reiji, > > On Thu, Mar 10, 2022 at 08:47:48PM -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 | 12 ++++++ >> arch/arm64/kvm/arm.c | 1 + >> arch/arm64/kvm/sys_regs.c | 65 ++++++++++++++++++++++++------- >> 3 files changed, 63 insertions(+), 15 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 2869259e10c0..c041e5afe3d2 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -101,6 +101,13 @@ 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. > > Doesn't the Feature ID register scheme only apply to CRm={1-7}, > op2={0-7}? I believe CRm=0, op2={1-4,7} are in fact UNDEFINED, not RAZ > like the other ranges. Furthermore, the registers that are defined in > that range do not go through the read_id_reg() plumbing. Will fix this. > >> + */ >> +#define KVM_ARM_ID_REG_MAX_NUM 64 >> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) >> + >> struct kvm_arch { >> struct kvm_s2_mmu mmu; >> >> @@ -137,6 +144,9 @@ struct kvm_arch { >> /* Memory Tagging Extension enabled for the guest */ >> bool mte_enabled; >> bool ran_once; >> + >> + /* ID registers for the guest. */ >> + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > This is a decently large array. Should we embed it in kvm_arch or > allocate at init? What is the reason why you think you might want to allocate it at init ? > [...] > >> + >> +/* >> + * 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) > > nit, more relevant if you take the above suggestion: maybe call it > kvm_init_id_regs()? > >> +{ >> + int i; >> + u32 id; >> + const struct sys_reg_desc *rd; >> + u64 val; >> + >> + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) { > > You could avoid walking the entire system register table, since we > already know the start and end values for the Feature ID register range.> > maybe: > > #define FEATURE_ID_RANGE_START SYS_ID_PFR0_EL1 > #define FEATURE_ID_RANGE_END sys_reg(3, 0, 0, 7, 7) > > u32 sys_reg; > > for (sys_reg = FEATURE_ID_RANGE_START; sys_reg <= FEATURE_ID_RANGE_END; sys_reg++) > > But, it depends on if this check is necessary: > >> + rd = &sys_reg_descs[i]; >> + if (rd->access != access_id_reg) >> + /* Not ID register, or hidden/reserved ID register */ >> + continue; > > Which itself is dependent on whether KVM is going to sparsely or > verbosely define its feature filtering tables per the other thread. So > really only bother with this if that is the direction you're going. Even just going through for ID register ranges, we should do the check to skip hidden/reserved ID registers (not to call read_sanitised_ftr_reg). Yes, it's certainly possible to avoid walking the entire system register, and I will fix it. The reason why I didn't care it so much was just because the code (walking the entire system register) will be removed by the following patches:) Thanks, Reiji
Hi Reiji, On Thu, Mar 24, 2022 at 09:23:10AM -0700, Reiji Watanabe wrote: [...] > > > + */ > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > + > > > struct kvm_arch { > > > struct kvm_s2_mmu mmu; > > > @@ -137,6 +144,9 @@ struct kvm_arch { > > > /* Memory Tagging Extension enabled for the guest */ > > > bool mte_enabled; > > > bool ran_once; > > > + > > > + /* ID registers for the guest. */ > > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > > > This is a decently large array. Should we embed it in kvm_arch or > > allocate at init? > > > What is the reason why you think you might want to allocate it at init ? Well, its a 512 byte array of mostly cold data. We're probably convinced that the guest is going to access these registers at most once per vCPU at boot. For the vCPU context at least, we only allocate space for registers we actually care about (enum vcpu_sysreg). My impression of the feature register ranges is that there are a lot of registers which are RAZ, so I don't believe we need to make room for uninteresting values. Additionally, struct kvm is visible to EL2 if running nVHE. I don't believe hyp will ever need to look at these register values. [...] > > Which itself is dependent on whether KVM is going to sparsely or > > verbosely define its feature filtering tables per the other thread. So > > really only bother with this if that is the direction you're going. > > Even just going through for ID register ranges, we should do the check > to skip hidden/reserved ID registers (not to call read_sanitised_ftr_reg). > > Yes, it's certainly possible to avoid walking the entire system register, > and I will fix it. The reason why I didn't care it so much was just > because the code (walking the entire system register) will be removed by > the following patches:) Let me go through the series again and see how this flows. If there is a way to avoid rewriting code introduced earlier in the series I would suggest going that route. -- Thanks, Oliver
Hi Oliver, > > > > + */ > > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > > + > > > > struct kvm_arch { > > > > struct kvm_s2_mmu mmu; > > > > @@ -137,6 +144,9 @@ struct kvm_arch { > > > > /* Memory Tagging Extension enabled for the guest */ > > > > bool mte_enabled; > > > > bool ran_once; > > > > + > > > > + /* ID registers for the guest. */ > > > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > > > > > This is a decently large array. Should we embed it in kvm_arch or > > > allocate at init? > > > > > > What is the reason why you think you might want to allocate it at init ? > > Well, its a 512 byte array of mostly cold data. We're probably > convinced that the guest is going to access these registers at most once > per vCPU at boot. > > For the vCPU context at least, we only allocate space for registers we > actually care about (enum vcpu_sysreg). My impression of the feature > register ranges is that there are a lot of registers which are RAZ, so I > don't believe we need to make room for uninteresting values. > > Additionally, struct kvm is visible to EL2 if running nVHE. I > don't believe hyp will ever need to look at these register values. As saving/restoring breakpoint/watchpoint registers for guests might need a special handling when AA64DFR0_EL1.BRPs get changed, next version of the series will use the data in the array at EL2 nVHE. They are cold data, and almost half of the entries will be RAZ at the moment though:) Thanks, Reiji
On Fri, Mar 25, 2022 at 07:35:39PM -0700, Reiji Watanabe wrote: > Hi Oliver, > > > > > > + */ > > > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > > > + > > > > > struct kvm_arch { > > > > > struct kvm_s2_mmu mmu; > > > > > @@ -137,6 +144,9 @@ struct kvm_arch { > > > > > /* Memory Tagging Extension enabled for the guest */ > > > > > bool mte_enabled; > > > > > bool ran_once; > > > > > + > > > > > + /* ID registers for the guest. */ > > > > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > > > > > > > This is a decently large array. Should we embed it in kvm_arch or > > > > allocate at init? > > > > > > > > > What is the reason why you think you might want to allocate it at init ? > > > > Well, its a 512 byte array of mostly cold data. We're probably > > convinced that the guest is going to access these registers at most once > > per vCPU at boot. > > > > For the vCPU context at least, we only allocate space for registers we > > actually care about (enum vcpu_sysreg). My impression of the feature > > register ranges is that there are a lot of registers which are RAZ, so I > > don't believe we need to make room for uninteresting values. > > > > Additionally, struct kvm is visible to EL2 if running nVHE. I > > don't believe hyp will ever need to look at these register values. > > As saving/restoring breakpoint/watchpoint registers for guests > might need a special handling when AA64DFR0_EL1.BRPs get changed, > next version of the series will use the data in the array at > EL2 nVHE. They are cold data, and almost half of the entries > will be RAZ at the moment though:) Shouldn't we always be doing a full context switch based on what registers are present in hardware? We probably don't want to leave host watchpoints/breakpoints visible to the guest. Additionally, if we are narrowing the guest's view of the debug hardware, are we going to need to start trapping debug register accesses? Unexposed breakpoint registers are supposed to UNDEF if we obey the Arm ARM to the letter. Even if we decide to not care about unexposed regs, I believe certain combinations of ID_AA64DF0_EL1.{BRPs, CTX_CMPs} might require that we emulate. -- Thanks, Oliver
Hi Oliver, On Sun, Mar 27, 2022 at 3:57 PM Oliver Upton <oupton@google.com> wrote: > > On Fri, Mar 25, 2022 at 07:35:39PM -0700, Reiji Watanabe wrote: > > Hi Oliver, > > > > > > > > + */ > > > > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > > > > + > > > > > > struct kvm_arch { > > > > > > struct kvm_s2_mmu mmu; > > > > > > @@ -137,6 +144,9 @@ struct kvm_arch { > > > > > > /* Memory Tagging Extension enabled for the guest */ > > > > > > bool mte_enabled; > > > > > > bool ran_once; > > > > > > + > > > > > > + /* ID registers for the guest. */ > > > > > > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; > > > > > > > > > > This is a decently large array. Should we embed it in kvm_arch or > > > > > allocate at init? > > > > > > > > > > > > What is the reason why you think you might want to allocate it at init ? > > > > > > Well, its a 512 byte array of mostly cold data. We're probably > > > convinced that the guest is going to access these registers at most once > > > per vCPU at boot. > > > > > > For the vCPU context at least, we only allocate space for registers we > > > actually care about (enum vcpu_sysreg). My impression of the feature > > > register ranges is that there are a lot of registers which are RAZ, so I > > > don't believe we need to make room for uninteresting values. > > > > > > Additionally, struct kvm is visible to EL2 if running nVHE. I > > > don't believe hyp will ever need to look at these register values. > > > > As saving/restoring breakpoint/watchpoint registers for guests > > might need a special handling when AA64DFR0_EL1.BRPs get changed, > > next version of the series will use the data in the array at > > EL2 nVHE. They are cold data, and almost half of the entries > > will be RAZ at the moment though:) > > Shouldn't we always be doing a full context switch based on what > registers are present in hardware? We probably don't want to leave host > watchpoints/breakpoints visible to the guest. Correct, any host breakpoints/watchpoints won't be exposed to the guest. (When restoring breakpoints/watchpoints for the guest, physical breakpoints that are not mapped to any virtual ones will be cleared) > Additionally, if we are narrowing the guest's view of the debug > hardware, are we going to need to start trapping debug register > accesses? Unexposed breakpoint registers are supposed to UNDEF if we > obey the Arm ARM to the letter. Even if we decide to not care about > unexposed regs, I believe certain combinations of ID_AA64DF0_EL1.{BRPs, > CTX_CMPs} might require that we emulate. Exactly, we will need to start trapping debug registers when the special handling is needed, and yes, we will need a special handling when the guest accesses to invalid virtual breakpoints/watchpoints or use the invalid virtual breakpoints as linked breakpoints. Thanks, Reiji
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 2869259e10c0..c041e5afe3d2 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -101,6 +101,13 @@ 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)) + struct kvm_arch { struct kvm_s2_mmu mmu; @@ -137,6 +144,9 @@ struct kvm_arch { /* Memory Tagging Extension enabled for the guest */ bool mte_enabled; bool ran_once; + + /* ID registers for the guest. */ + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM]; }; struct kvm_vcpu_fault_info { @@ -736,6 +746,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 4783dbf66df2..91110d996ed6 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 4dc2fba316ff..d2b3ad32ab5a 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_with_encoding(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_with_encoding(vcpu, SYS_ID_AA64MMFR1_EL1); u32 sr = reg_to_encoding(r); if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { @@ -1059,17 +1061,16 @@ 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 bool is_id_reg(u32 id) { - u32 id = reg_to_encoding(r); - u64 val; - - if (raz) - return 0; + return (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); +} - val = read_sanitised_ftr_reg(id); +static u64 read_id_reg_with_encoding(const struct kvm_vcpu *vcpu, u32 id) +{ + u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; switch (id) { case SYS_ID_AA64PFR0_EL1: @@ -1119,6 +1120,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_with_encoding(vcpu, id); +} + static unsigned int id_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { @@ -1223,9 +1232,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, @@ -1837,8 +1845,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_with_encoding(vcpu, SYS_ID_AA64DFR0_EL1); + u64 pfr = read_id_reg_with_encoding(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 +2858,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 | 12 ++++++ arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/sys_regs.c | 65 ++++++++++++++++++++++++------- 3 files changed, 63 insertions(+), 15 deletions(-)