Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

From: Cornelia Huck <cohuck@redhat.com>
To: Oliver Upton <oliver.upton@linux.dev>,
	Jing Zhang <jingzhangos@google.com>
Cc: KVM <kvm@vger.kernel.org>, KVMARM <kvmarm@lists.linux.dev>,
	ARMLinux <linux-arm-kernel@lists.infradead.org>,
	Marc Zyngier <maz@kernel.org>, Oliver Upton <oupton@google.com>,
	Will Deacon <will@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>,
	Raghavendra Rao Ananta <rananta@google.com>,
	Suraj Jitindar Singh <surajjs@amazon.com>
Subject: Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1
Date: Tue, 04 Jul 2023 17:06:30 +0200	[thread overview]
Message-ID: <874jmjiumh.fsf@redhat.com> (raw)
In-Reply-To: <ZJm+Kj0C5YySp055@linux.dev>

On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
>> Since number of context-aware breakpoints must be no more than number
>> of supported breakpoints according to Arm ARM, return an error if
>> userspace tries to set CTX_CMPS field to such value.
>> 
>> Signed-off-by: Jing Zhang <jingzhangos@google.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 50d4e25f42d3..a6299c796d03 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>  			       const struct sys_reg_desc *rd,
>>  			       u64 val)
>>  {
>> -	u8 pmuver, host_pmuver;
>> +	u8 pmuver, host_pmuver, brps, ctx_cmps;
>>  	bool valid_pmu;
>>  
>> +	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
>> +	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
>> +	if (ctx_cmps > brps)
>> +		return -EINVAL;
>> +
>
> I'm not fully convinced on the need to do this sort of cross-field
> validation... I think it is probably more trouble than it is worth. If
> userspace writes something illogical to the register, oh well. All we
> should care about is that the advertised feature set is a subset of
> what's supported by the host.
>
> The series doesn't even do complete sanity checking, and instead works
> on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
> special handling depending on how pedantic you're feeling. AArch32
> support at a higher exception level implies AArch32 support at all lower
> exception levels.
>
> But that isn't a suggestion to implement it, more of a suggestion to
> just avoid the problem as a whole.

Generally speaking, how much effort do we want to invest to prevent
userspace from doing dumb things? "Make sure we advertise a subset of
features of what the host supports" and "disallow writing values that
are not allowed by the architecture in the first place" seem reasonable,
but if userspace wants to create weird frankencpus[1], should it be
allowed to break the guest and get to keep the pieces?

I'd be more in favour to rely on userspace to configure something that
is actually usable; it needs to sanitize any user-provided configuration
anyway.

[1] I think userspace will end up creating frankencpus in any case, but
at least it should be the kind that doesn't look out of place in the
subway if you dress it in proper clothing.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel