From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8186BC77B7C for ; Wed, 31 May 2023 07:31:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234603AbjEaHba (ORCPT ); Wed, 31 May 2023 03:31:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234510AbjEaHb3 (ORCPT ); Wed, 31 May 2023 03:31:29 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DD8F4123 for ; Wed, 31 May 2023 00:31:27 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7AA6263768 for ; Wed, 31 May 2023 07:31:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D82B3C433D2; Wed, 31 May 2023 07:31:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685518286; bh=HF7OKbXM2BGCGbyxMZ7v0Zp7UV6T2JLlyQvFgaWN0d8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=kzdyDYEZfiGiN0E3Bfb41AdhWH/bZ+GihR4qv7iFZZOpaM37vhemeovJc7CHYOHl3 ILIFb7e/JXHSa9Wp9x9NHyi9uBeDyN1szHlxtfEdiWiUSxF9cbVrSBLLORDWxubf42 0EBSPI53J6WplbvxbfA7m8hPvC4W1yliWNo0i9ydim58+psQgJjb8y4xtqIGI7ZAKI T73sCIiUdCZPjfefrLflMNkkzncONG4UHDYqmIGcuLLxxlqNgQZpKqPphTR0j0gWGS bbCnfZhaToYXwJkq6Nc+N4ro6F5q+RtGr/+cKHA1Dva4QUA3g8dAf1O+04iEOK3iiV LQ8ru5FM44QMw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1q4GIm-001Xqs-UU; Wed, 31 May 2023 08:31:25 +0100 Date: Wed, 31 May 2023 08:31:24 +0100 Message-ID: <86353dc5yr.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Cc: KVM , KVMARM , ARMLinux , Oliver Upton , Will Deacon , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Fuad Tabba , Reiji Watanabe , Raghavendra Rao Ananta Subject: Re: [PATCH v10 5/5] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 In-Reply-To: References: <20230522221835.957419-1-jingzhangos@google.com> <20230522221835.957419-6-jingzhangos@google.com> <87pm6kogx8.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, oupton@google.com, will@kernel.org, pbonzini@redhat.com, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, tabba@google.com, reijiw@google.com, rananta@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, 30 May 2023 22:18:04 +0100, Jing Zhang wrote: >=20 > Hi Marc, >=20 > On Sun, May 28, 2023 at 4:05=E2=80=AFAM Marc Zyngier wro= te: > > > > On Mon, 22 May 2023 23:18:35 +0100, > > Jing Zhang wrote: > > > > > > Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3], > > > ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities > > > specific to ID register. > > > > > > Signed-off-by: Jing Zhang > > > --- > > > arch/arm64/include/asm/cpufeature.h | 1 + > > > arch/arm64/kernel/cpufeature.c | 2 +- > > > arch/arm64/kvm/sys_regs.c | 365 ++++++++++++++++++--------= -- > > > 3 files changed, 243 insertions(+), 125 deletions(-) > > > > Reading the result after applying this series, I feel like a stuck > > record. This final series still contains gems like this: > > > > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, > > u64 val) > > { > > u8 csv2, csv3; > > > > /* > > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > > * it doesn't promise more than what is actually provided (the > > * guest could otherwise be covered in ectoplasmic residue). > > */ > > csv2 =3D cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_= EL1_CSV2_SHIFT); > > if (csv2 > 1 || > > (csv2 && arm64_get_spectre_v2_state() !=3D SPECTRE_UNAFFECT= ED)) > > return -EINVAL; > > > > /* Same thing for CSV3 */ > > csv3 =3D cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_= EL1_CSV3_SHIFT); > > if (csv3 > 1 || > > (csv3 && arm64_get_meltdown_state() !=3D SPECTRE_UNAFFECTED= )) > > return -EINVAL; > > > > return set_id_reg(vcpu, rd, val); > > } > > > > Why do we have this? I've asked the question at least 3 times in the > > previous versions, and I still see the same code. > > > > If we have sane limits, the call to arm64_check_features() in > > set_id_reg() will catch the illegal write. So why do we have this at > > all? The whole point of the exercise was to unify the handling. But > > you're actually making it worse. > > > > So what's the catch? > Sorry, I am only aware of one discussion of this code in v8. The > reason I still keep the check here is that the arm64_check_features() > can not catch all illegal writes as this code does. > For example, for CSV2, one concern is: > When arm64_get_spectre_v2_state() !=3D SPECTRE_UNAFFECTED, this code > only allows guest CSV2 to be set to 0, any non-zero value would lead > to -EINVAL. If we remove the check here, the guest CSV2 can be set to > any value lower or equal to host CSV2. Sorry, this doesn't make sense. Lower is always fine. If you meant 'higher', then I agree that it would be bad. But that doesn't make keeping this code the right outcome. > Of course, we can set the sane limit of CSV2 to 0 when > arm64_get_spectre_v2_state() !=3D SPECTRE_UNAFFECTED in > read_sanitised_id_aa64pfr0_el1(). Then we can remove all the checks > here and no specific set_id function for AA64PFR0_EL1 is needed. This is what I have been asking for all along: the "sanitised" view of the register *must* return the absolute limit for the fields that are flagged as writable by "mask". If we need extra code, then something is really wrong. The core feature code manages that without any special casing, and we should be able to reach the same level. Thanks, M. --=20 Without deviation from the norm, progress is not possible.