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 D398EC433EF for ; Mon, 6 Dec 2021 09:53:56 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+f//dYjs7kimv8p+ucKgXjLhjEbMBPIujbMFmkdMCxk=; b=ptbpbz8QjlwEU2 Slmg/siZA8QKc2GSIaI9D+z2Q9SozrNaaM/zD7f7crTFBUlS1UoCnWfur4KreQeaQq6KkUHnW2B8z mqSwvFiYcZnPQXcAv+dlkFk1HBMEh/VM1j3cfesjl8J/XrkynrQ0LaE1TCGGAzccencbGNAi10xNe 44P3b+syfglYz9ZrOEv4gI58mSDc6H1DI2t0leKfn8Qy5paT7F3NKpZsBSoNpx7pUrfuMuCKT5mB0 wu00ibN0zF3gqTppe/8oeTum2dPblSEWAB4jr+vwb2Z/L+RUk0GpWtCdUIxwmrZSm9S4PZPRBSn7j SK7VgpWc1fG9r8DswSlQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1muAfN-003D9m-GO; Mon, 06 Dec 2021 09:52:13 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1muAfK-003D9E-4X for linux-arm-kernel@lists.infradead.org; Mon, 06 Dec 2021 09:52:11 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DDEBA11FB; Mon, 6 Dec 2021 01:52:07 -0800 (PST) Received: from monolith.localdoman (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 51E6B3F73D; Mon, 6 Dec 2021 01:52:06 -0800 (PST) Date: Mon, 6 Dec 2021 09:52:01 +0000 From: Alexandru Elisei To: Reiji Watanabe Cc: Eric Auger , kvm@vger.kernel.org, Marc Zyngier , Peter Shier , Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v3 09/29] KVM: arm64: Hide IMPLEMENTATION DEFINED PMU support for the guest Message-ID: References: <20211117064359.2362060-1-reijiw@google.com> <20211117064359.2362060-10-reijiw@google.com> <5bd01c9c-6ac8-4034-6f49-be636a3b287c@redhat.com> <2ed3072b-f83d-1b17-0949-ca38267ba94e@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211206_015210_321456_72B278C8 X-CRM114-Status: GOOD ( 40.62 ) 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 Hi, On Sat, Dec 04, 2021 at 09:39:59AM -0800, Reiji Watanabe wrote: > Hi Eric, > > On Sat, Dec 4, 2021 at 6:14 AM Eric Auger wrote: > > > > Hi Reiji, > > > > On 12/4/21 2:04 AM, Reiji Watanabe wrote: > > > Hi Eric, > > > > > > On Thu, Dec 2, 2021 at 2:57 AM Eric Auger wrote: > > >> > > >> Hi Reiji, > > >> > > >> On 11/30/21 6:32 AM, Reiji Watanabe wrote: > > >>> Hi Eric, > > >>> > > >>> On Thu, Nov 25, 2021 at 12:30 PM Eric Auger wrote: > > >>>> > > >>>> Hi Reiji, > > >>>> > > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > >>>>> When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > >>>>> means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > >>>>> expose the value for the guest as it is. Since KVM doesn't support > > >>>>> IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > >>>>> exopse 0x0 (PMU is not implemented) instead. > > >>>> s/exopse/expose > > >>>>> > > >>>>> Change cpuid_feature_cap_perfmon_field() to update the field value > > >>>>> to 0x0 when it is 0xf. > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>>> is it wrong to expose the guest with a Perfmon value of 0xF? Then the > > >>>> guest should not use it as a PMUv3? > > >>> > > >>> For the value 0xf in ID_AA64DFR0_EL1.PMUVER and ID_DFR0_EL1.PERFMON, > > >>> Arm ARM says: > > >>> "IMPLEMENTATION DEFINED form of performance monitors supported, > > >>> PMUv3 not supported." > > >>> > > >>> Since the PMU that KVM supports for guests is PMUv3, 0xf shouldn't > > >>> be exposed to guests (And this patch series doesn't allow userspace > > >>> to set the fields to 0xf for guests). > > >> What I don't get is why this isn't detected before (in kvm_reset_vcpu). > > >> if the VCPU was initialized with KVM_ARM_VCPU_PMU_V3 can we honor this > > >> init request if the host pmu is implementation defined? > > > > > > KVM_ARM_VCPU_INIT with KVM_ARM_VCPU_PMU_V3 will fail in > > > kvm_reset_vcpu() if the host PMU is implementation defined. > > > > OK. This was not obvsious to me. > > > > if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) { > > ret = -EINVAL; > > goto out; > > } > > > > kvm_perf_init > > + if (perf_num_counters() > 0) > > + static_branch_enable(&kvm_arm_pmu_available); > > > > But I believe you ;-), sorry for the noise > > Thank you for the review ! > > I didn't find the code above in v5.16-rc3, which is the base code of > this series. So, I'm not sure where the code came from (any kvmarm > repository branch ??). > > What I see in v5.16-rc3 is: > ---- > int kvm_perf_init(void) > { > return perf_register_guest_info_callbacks(&kvm_guest_cbs); > } > > void kvm_host_pmu_init(struct arm_pmu *pmu) > { > if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF && > !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled()) > static_branch_enable(&kvm_arm_pmu_available); > } > ---- > > And I don't find any other code that enables kvm_arm_pmu_available. The code was recently changed (in v5.15 I think), I think Eric is looking at an older version. > > Looking at the KVM's PMUV3 support code for guests in v5.16-rc3, > if KVM allows userspace to configure KVM_ARM_VCPU_PMU_V3 even with > ID_AA64DFR0_PMUVER_IMP_DEF on the host (, which I don't think it does), > I think we should fix that to not allow that. I recently started looking into that too. If there's only one PMU, then the guest won't see the value IMP DEF for PMUVer (userspace cannot set the PMU feature because !kvm_arm_support_pmu_v3()). On heterogeneous systems with multiple PMUs, it gets complicated. I don't have any such hardware, but what I think will happen is that KVM will enable the static branch if there is at least one PMU with PMUVer != IMP_DEF, even if there are other PMUs with PMUVer = IMP_DEF. But read_sanitised_ftr_reg() will always return 0 for the PMUVer field because the field is defined as FTR_EXACT with a safe value of 0 in cpufeature.c. So the guest ends up seeing PMUVer = 0. I'm not sure if this is the case because I'm not familiar with the cpu features code, but I planning to investigate further. Thanks, Alex > (I'm not sure how KVM's PMUV3 support code is implemented in the > code that you are looking at though) > > Thanks, > Reiji _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel