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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C832ECDB474 for ; Mon, 23 Oct 2023 10:41:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=/aXiqaEca84krxr+amus4ZUk6UU5JGiivBT0NSQcuUQ=; b=VMYSZPbg2YEE6x f7ykmzFY0hVTJPw4WvUe07In726MG+jwfu8rT29rb0K/SavIgsXa45pwFuzGLdke5QixIHIrFDPbg lByzT+i4eNcog9crN8PP/YNgYmdebLgsWixkgM8ioplrawnF0olAyFHOUkJu+U2YetBmmD2ZScG8Q bWp9T4hqfnsU5hrR01a8/s/vHM3G+tTRFyi1+XWXRxPoZ8td2sHM0Z5QrxjUZLkg423ub201Vd7dF WPQSPaqIcPhFgnzRgrViipNycdHi5UGQD6esMqpLbS9rBg2WqBYUbr0eS1dO4s787xe4jhcKE6USo 0v2+IIRiIFiSu77lG8Tg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qusMk-0072kI-2r; Mon, 23 Oct 2023 10:40:58 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qusMh-0072jk-2R for linux-arm-kernel@lists.infradead.org; Mon, 23 Oct 2023 10:40:57 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id E364660BD8; Mon, 23 Oct 2023 10:40:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 932A5C433C7; Mon, 23 Oct 2023 10:40:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1698057654; bh=dLDu4Z9DGcX4lMCPJLO64Bez7E60sRC9AOdA72H5GcM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=d8sUNXPvI9rfDXLpyPBktUT2kiWykiZvBA+zpi6kkBEnoC88t1g2ReKTn1s5ZgAj0 otouI1rKZ5pPPzOsLXh3KvAvHpT8raygEIWt4cVVbp/Ic6JQSM9XF2lwUSQEV29Yd6 gR/1wrcn2SOldfRAvCK/MFfGrL1PDfM6SF4eHQiQPArZoBrtD3KbvkCTCFANdtq7Tl qZMyJ633giEZYLRMb1gr+ERjIId+B2TuCQdwYzqc5VJuDxtJWKpbQdjEW/O4iBOy41 h4WPV80MizmBFFn+BLuCteMnlMYl2VkliVVrqTjSmiUK6dH6ZKzWGL3e8YPciPN1vk bH/1wC3MUDRWg== 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 1qusMe-006oJw-7Q; Mon, 23 Oct 2023 11:40:52 +0100 Date: Mon, 23 Oct 2023 11:40:50 +0100 Message-ID: <8634y162q5.wl-maz@kernel.org> From: Marc Zyngier To: Raghavendra Rao Ananta Cc: Oliver Upton , Alexandru Elisei , James Morse , Suzuki K Poulose , Paolo Bonzini , Zenghui Yu , Shaoqin Huang , Jing Zhang , Reiji Watanabe , Colton Lewis , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH v8 02/13] KVM: arm64: PMU: Set the default PMU for the guest before vCPU reset In-Reply-To: <20231020214053.2144305-3-rananta@google.com> References: <20231020214053.2144305-1-rananta@google.com> <20231020214053.2144305-3-rananta@google.com> 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/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: rananta@google.com, oliver.upton@linux.dev, alexandru.elisei@arm.com, james.morse@arm.com, suzuki.poulose@arm.com, pbonzini@redhat.com, yuzenghui@huawei.com, shahuang@redhat.com, jingzhangos@google.com, reijiw@google.com, coltonlewis@google.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231023_034055_879847_BE3BD7B1 X-CRM114-Status: GOOD ( 37.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 20 Oct 2023 22:40:42 +0100, Raghavendra Rao Ananta wrote: > > From: Reiji Watanabe > > The following patches will use the number of counters information > from the arm_pmu and use this to set the PMCR.N for the guest > during vCPU reset. However, since the guest is not associated > with any arm_pmu until userspace configures the vPMU device > attributes, and a reset can happen before this event, assign a > default PMU to the guest just before doing the reset. > > Signed-off-by: Reiji Watanabe > Signed-off-by: Raghavendra Rao Ananta > --- > arch/arm64/kvm/arm.c | 19 +++++++++++++++++++ > arch/arm64/kvm/pmu-emul.c | 16 ++++------------ > include/kvm/arm_pmu.h | 6 ++++++ > 3 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index c6cad400490f9..08c2f76983b9d 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -1319,6 +1319,21 @@ static bool kvm_vcpu_init_changed(struct kvm_vcpu *vcpu, > KVM_VCPU_MAX_FEATURES); > } > > +static int kvm_setup_vcpu(struct kvm_vcpu *vcpu) > +{ > + struct kvm *kvm = vcpu->kvm; > + > + /* > + * When the vCPU has a PMU, but no PMU is set for the guest > + * yet, set the default one. > + */ > + if (kvm_vcpu_has_pmu(vcpu) && !kvm->arch.arm_pmu && > + kvm_arm_set_default_pmu(kvm)) > + return -EINVAL; nit: I'm not keen on re-interpreting the error code. If kvm_arm_set_default_pmu() returns an error, we should return *that* particular error, and not any other. Something like: static int kvm_setup_vcpu(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; int err = 0; /* * When the vCPU has a PMU, but no PMU is set for the guest * yet, set the default one. */ if (kvm_vcpu_has_pmu(vcpu) && !kvm->arch.arm_pmu) err = kvm_arm_set_default_pmu(kvm); return err; } > + > + return 0; > +} > + > static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu, > const struct kvm_vcpu_init *init) > { > @@ -1334,6 +1349,10 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu, > > bitmap_copy(kvm->arch.vcpu_features, &features, KVM_VCPU_MAX_FEATURES); > > + ret = kvm_setup_vcpu(vcpu); > + if (ret) > + goto out_unlock; > + Hmmm. Contrary to what the commit message says, the default PMU is not picked at reset time, but at the point where the target is set (the very first vcpu init). Which is pretty different from reset, which happens more than once. I also can't say I'm over the moon with yet another function that does a very tiny bit of initialisation outside of the rest of the code that performs the vcpu init. Following things is an absolute maze... > /* Now we know what it is, we can reset it. */ > kvm_reset_vcpu(vcpu); > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index eb5dcb12dafe9..66c244021ff08 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -717,10 +717,9 @@ static struct arm_pmu *kvm_pmu_probe_armpmu(void) > * It is still necessary to get a valid cpu, though, to probe for the > * default PMU instance as userspace is not required to specify a PMU > * type. In order to uphold the preexisting behavior KVM selects the > - * PMU instance for the core where the first call to the > - * KVM_ARM_VCPU_PMU_V3_CTRL attribute group occurs. A dependent use case > - * would be a user with disdain of all things big.LITTLE that affines > - * the VMM to a particular cluster of cores. > + * PMU instance for the core just before the vcpu reset. A dependent use > + * case would be a user with disdain of all things big.LITTLE that > + * affines the VMM to a particular cluster of cores. Same problem, see above. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel