Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sat, 13 Apr 2024 11:19:35 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH 3/4] KVM: arm64: add emulation for CTR_EL0 register |
| |
On Fri, 05 Apr 2024 13:01:07 +0100, Sebastian Ott <sebott@redhat.com> wrote: > > CTR_EL0 is currently handled as an invariant register, thus > guests will be presented with the host value of that register. > Add emulation for CTR_EL0 based on a per VM value. > > When CTR_EL0 is changed the reset function for CLIDR_EL1 is > called to make sure we present the guest with consistent > register values.
Isn't that a change in the userspace ABI? You are now creating an explicit ordering between the write to CTR_EL0 and the rest of the cache hierarchy registers. It has the obvious capacity to lead to the wrong result in a silent way...
> > Signed-off-by: Sebastian Ott <sebott@redhat.com> > --- > arch/arm64/kvm/sys_regs.c | 72 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 64 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4d29b1a0842d..b0ba292259f9 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1874,6 +1874,55 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > return true; > } > > +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > +{ > + vcpu->kvm->arch.ctr_el0 = 0; > + return kvm_get_ctr_el0(vcpu->kvm);
I'd expect the cached value to be reset instead of being set to 0. What are you achieving by this?
> +} > + > +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 *val) > +{ > + *val = kvm_get_ctr_el0(vcpu->kvm); > + return 0; > +} > + > +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding); > + > +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 val) > +{ > + u64 host_val = read_sanitised_ftr_reg(SYS_CTR_EL0); > + u64 old_val = kvm_get_ctr_el0(vcpu->kvm); > + const struct sys_reg_desc *clidr_el1; > + int ret; > + > + if (val == old_val) > + return 0; > + > + if (kvm_vm_has_ran_once(vcpu->kvm)) > + return -EBUSY; > + > + mutex_lock(&vcpu->kvm->arch.config_lock); > + ret = arm64_check_features(vcpu, rd, val); > + if (ret) { > + mutex_unlock(&vcpu->kvm->arch.config_lock); > + return ret; > + } > + if (val != host_val) > + vcpu->kvm->arch.ctr_el0 = val; > + else > + vcpu->kvm->arch.ctr_el0 = 0; > + > + mutex_unlock(&vcpu->kvm->arch.config_lock); > + > + clidr_el1 = get_sys_reg_desc(SYS_CLIDR_EL1); > + if (clidr_el1) > + clidr_el1->reset(vcpu, clidr_el1); > + > + return 0;
No check against what can be changed, and in what direction? You seem to be allowing a guest to migrate from a host with IDC==1 to one where IDC==0 (same for DIC). How can that work? Same for the cache lines, which can be larger on the target... How will the guest survive that?
> +} > + > static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > const struct sys_reg_desc *r) > { > @@ -2460,7 +2509,11 @@ static const struct sys_reg_desc sys_reg_descs[] = { > { SYS_DESC(SYS_CCSIDR2_EL1), undef_access }, > { SYS_DESC(SYS_SMIDR_EL1), undef_access }, > { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 }, > - { SYS_DESC(SYS_CTR_EL0), access_ctr }, > + { SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr, > + .get_user = get_ctr, .set_user = set_ctr, .val = (CTR_EL0_DIC_MASK | > + CTR_EL0_IDC_MASK | > + CTR_EL0_DminLine_MASK | > + CTR_EL0_IminLine_MASK)}, > { SYS_DESC(SYS_SVCR), undef_access }, > > { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr, > @@ -3623,6 +3676,13 @@ static bool index_to_params(u64 id, struct sys_reg_params *params) > } > } > > +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding) > +{ > + struct sys_reg_params params = encoding_to_params(encoding); > + > + return find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > +} > + > const struct sys_reg_desc *get_reg_by_id(u64 id, > const struct sys_reg_desc table[], > unsigned int num) > @@ -3676,18 +3736,11 @@ FUNCTION_INVARIANT(midr_el1) > FUNCTION_INVARIANT(revidr_el1) > FUNCTION_INVARIANT(aidr_el1) > > -static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) > -{ > - ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0); > - return ((struct sys_reg_desc *)r)->val; > -} > - > /* ->val is filled in by kvm_sys_reg_table_init() */ > static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = { > { SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 }, > { SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 }, > { SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 }, > - { SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 }, > }; > > static int get_invariant_sys_reg(u64 id, u64 __user *uaddr) > @@ -4049,6 +4102,9 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > vcpu->arch.hcrx_el2 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2); > } > > + if (vcpu->kvm->arch.ctr_el0) > + vcpu->arch.hcr_el2 |= HCR_TID2;
Why trap CTR_EL0 if the values are the same as the host? I really dislike the use of the value 0 as a such an indication. Why isn't this grouped with the traps in vcpu_reset_hcr()?
Thanks,
M.
-- Without deviation from the norm, progress is not possible.
| ![\](/images/icornerr.gif) |