Received-SPF: pass (google.com: domain of linux-kernel+bounces-215149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 14 Jun 2024 17:31:37 +0200 (CEST)
From: Sebastian Ott <sebott@redhat.com>
To: Oliver Upton <oliver.upton@linux.dev>
cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, 
    linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>, 
    James Morse <james.morse@arm.com>, 
    Suzuki K Poulose <suzuki.poulose@arm.com>, 
    Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, 
    Shaoqin Huang <shahuang@redhat.com>, Eric Auger <eric.auger@redhat.com>
Subject: Re: [PATCH v4 3/6] KVM: arm64: add emulation for CTR_EL0 register
In-Reply-To: <Zmty99X4hnYwtRS4@linux.dev>
Message-ID: <0b148e21-1714-f4f7-bc77-2a12b990572d@redhat.com>
References: <20240603130507.17597-1-sebott@redhat.com> <20240603130507.17597-4-sebott@redhat.com> <ZmtwjLbP283ra0Xq@linux.dev> <Zmty99X4hnYwtRS4@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed

Hi Oliver,

On Thu, 13 Jun 2024, Oliver Upton wrote:
> On Thu, Jun 13, 2024 at 10:19:56PM +0000, Oliver Upton wrote:
>> On Mon, Jun 03, 2024 at 03:05:04PM +0200, Sebastian Ott wrote:
>>> +static int validate_cache_topology(struct kvm_vcpu *vcpu, u64 ctr_el0)
>>> +{
>>> +	const struct sys_reg_desc *clidr_el1;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	clidr_el1 = get_sys_reg_desc(SYS_CLIDR_EL1);
>>> +	if (!clidr_el1)
>>> +		return -ENOENT;
>>
>> This doesn't actually matter if we agree on dropping the cross-checking,
>> but if this lookup fails it is 100% a KVM bug. Returning ENOENT isn't
>> exactly right here, since it gives userspace the impression that the
>> sysreg index it tried to access does not exist.
>>
>> So in the future it'd be good to return EINVAL in places where the
>> kernel did something stupid, probably with a warning for good measure.

OK.

>>> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>>> +		   u64 val)
>>> +{
>>> +	u64 ctr, writable_mask = rd->val;
>>> +	int ret = 0;
>>> +
>>> +	mutex_lock(&vcpu->kvm->arch.config_lock);
>>> +	ctr  = vcpu->kvm->arch.ctr_el0;
>>> +	if (val == ctr)
>>> +		goto out_unlock;
>>> +
>>> +	ret = -EBUSY;
>>> +	if (kvm_vm_has_ran_once(vcpu->kvm))
>>> +		goto out_unlock;
>>> +
>>> +	ret = -EINVAL;
>>> +	if ((ctr & ~writable_mask) != (val & ~writable_mask))
>>> +		goto out_unlock;
>>> +
>>> +	if (((ctr & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)) ||
>>> +	    ((ctr & CTR_EL0_IDC_MASK) < (val & CTR_EL0_IDC_MASK)) ||
>>> +	    ((ctr & CTR_EL0_DminLine_MASK) < (val & CTR_EL0_DminLine_MASK)) ||
>>> +	    ((ctr & CTR_EL0_IminLine_MASK) < (val & CTR_EL0_IminLine_MASK))) {
>>> +		goto out_unlock;
>>
>> I'd prefer if we addressed the issue w/ arm64_check_features() by making
>> CTR_EL0 behave like the other registers in the ID space instead of
>> open-coding these sorts of checks.
>>
>> I believe that can be accomplished by using kvm_read_sanitised_id_reg()
>> as the ::reset() function in the descriptor and initializing
>> kvm->arch.ctr_el0 in kvm_reset_id_regs().
>
> Durr, I got rid of kvm_reset_id_regs() in commit 44cbe80b7616 ("KVM: arm64:
> Reset VM feature ID regs from kvm_reset_sys_regs()"), I should engage
> brain before responding.
>
> Adding a check for encoding == CTR_EL0 to is_vm_ftr_id_reg() seems to be
> the best way out.

Hm, but in that case we'd use reset_vm_ftr_id_reg() meaning we would have
to make IDREG() work for this reg. Either by adding special handling to
that macro or by increasing kvm->arch.id_regs[] a lot - both options don't
sound very appealing.

I'll think of smth to make arm64_check_features() work for this.

Thanks,
Sebastian