Received-SPF: pass (google.com: domain of linux-kernel+bounces-214055-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: Thu, 13 Jun 2024 22:19:56 +0000
From: Oliver Upton <oliver.upton@linux.dev>
To: Sebastian Ott <sebott@redhat.com>
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
Message-ID: <ZmtwjLbP283ra0Xq@linux.dev>
References: <20240603130507.17597-1-sebott@redhat.com>
 <20240603130507.17597-4-sebott@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240603130507.17597-4-sebott@redhat.com>

Hi Sebastian,

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.

> +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().

> +	}
> +	ret = validate_cache_topology(vcpu, val);
> +	if (ret)
> +		goto out_unlock;

My concerns about adding these sort of cross-checks remains. The sysreg
code will become exponentially more messy with each cross-register check
we add, given the complete lack of ordering on the UAPI.

So long as KVM has independently tested the validity of the cache
hierarchy and CTR_EL0 against the capabilities of hardware, we know that
userspace cannot advertise more than what's supported in hardware.

If CLIDR_EL1 doesn't line up with the value of CTR_EL0 exposed to the
guest then it is a userspace bug. There simply is no amount of
foolproofing that can be done in KVM to protect against a buggy VMM.

-- 
Thanks,
Oliver