Re: [PATCH 3/4] KVM: arm64: add emulation for CTR_EL0 register

[Sebastian Ott]

On Sat, 13 Apr 2024, Marc Zyngier wrote:

> On Fri, 05 Apr 2024 13:01:07 +0100,
> Sebastian Ott <sebott@xxxxxxxxxx> 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...

Yea, that's why I've asked in the cover letter if userspace would be
ok with that. I thought that this is what you suggested in your reply
to the RFC. 
(https://lore.kernel.org/linux-arm-kernel/20240318111636.10613-5-sebott@xxxxxxxxxx/T/#m0aea5b744774f123bd9a4a4b7be6878f6c737f63)

But I guess I've got that wrong.

Do we have other means to handle the dependencies between registers?
Allow inconsistent values and do a sanity check before the first
vcpu_run()?

> > Signed-off-by: Sebastian Ott <sebott@xxxxxxxxxx>
> > ---
> >  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?

The idea was that kvm->arch.ctr_el0 == 0 means we use the host value and
don't set up a trap.

> > +}
> > +
> > +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?

Shouldn't this all be handled by arm64_check_features() using the safe
value definitions from ftr_ctr? (I'll double check that..)

> > @@ -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?

For values same as host vcpu->kvm->arch.ctr_el0 would be zero and
reg access would not be trapped.

> I really dislike the use of the value 0 as a such an indication.

OK.

> Why isn't this grouped with the traps in vcpu_reset_hcr()?

I was under the impression that vcpu_reset_hcr() is called too early
to decide if we need to set up a trap or not (but lemme double check
that).

Thanks,
Sebastian