Received-SPF: pass (google.com: domain of linux-kernel+bounces-143740-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sat, 13 Apr 2024 15:50:42 +0200 (CEST)
From: Sebastian Ott <sebott@redhat.com>
To: Marc Zyngier <maz@kernel.org>
cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, 
    linux-kernel@vger.kernel.org, Oliver Upton <oliver.upton@linux.dev>, 
    James Morse <james.morse@arm.com>, 
    Suzuki K Poulose <suzuki.poulose@arm.com>, 
    Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH 3/4] KVM: arm64: add emulation for CTR_EL0 register
In-Reply-To: <86edb9sgy0.wl-maz@kernel.org>
Message-ID: <7cc16dc9-6eef-59f9-d019-8b5dea6a4254@redhat.com>
References: <20240405120108.11844-1-sebott@redhat.com> <20240405120108.11844-4-sebott@redhat.com> <86edb9sgy0.wl-maz@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed

On Sat, 13 Apr 2024, Marc Zyngier wrote:

> 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...

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@redhat.com/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@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?

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