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 EC694C4828E for ; Fri, 2 Feb 2024 07:36:41 +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=I67NFwLUSuAWWOMyUQFOfI13YnhxJ7NwTfFZrPfS9KM=; b=WSYeBQgnuP2WaA k/M6nsCM5UMRIn1yN1OH6fSOKa4R2hGWJ2MtnwkbeOFO3WKwwzQXNwQMCB8BGdqQTFphayUTQRIhk 3uTtuiKZZn9DQJHP5a8wWP6XVbv9j/CYqeKWF3zTo3xUFXfvMteUu4Dgi3C4yfsVDHpexHtBRio4/ vmluR+AlwM0EgXzskm81Y70Cx3gl5nAZ61YBVxIAc2KaEYfXg+7KY7SPn3CvjmeSEzpnF0JcYrMB+ oVO5fzafDwsFvutwgYPkM13iRRuKgEORebFC30cjTdHc04bCLDvrowZynoBSRnLcYZWJwifKJQHZK YKFp2fE02YR4JxLNSIwA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVo65-0000000AbSR-3m0C; Fri, 02 Feb 2024 07:36:25 +0000 Received: from out-173.mta0.migadu.com ([2001:41d0:1004:224b::ad]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVo63-0000000AbPZ-0Fav for linux-arm-kernel@lists.infradead.org; Fri, 02 Feb 2024 07:36:24 +0000 Date: Fri, 2 Feb 2024 07:36:08 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1706859375; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5EWZhYkrDt/8XKpyBqmfzBglkyRPBzsB9XpFbePCy6s=; b=Ky1+34J6vLyruougguTgN/oJcFJWeF+YLyxaTsnQ9gIwRuQsHNtyO1eFPA58P6H3pOO0tx 7EuHIa7HF/yJjY14d7KjLmsw4C0A5wKd3POh4hbUg68SxT5aSTr6mugk2tEcm/R5sY8hD8 /dqY+kyYgEts8oY6uBbwKYoFe+aM5no= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Oliver Upton To: Shaoqin Huang Cc: Marc Zyngier , kvmarm@lists.linux.dev, Eric Auger , Eric Auger , Paolo Bonzini , Shuah Khan , James Morse , Suzuki K Poulose , Zenghui Yu , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public Message-ID: References: <20240202025659.5065-1-shahuang@redhat.com> <20240202025659.5065-2-shahuang@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240202025659.5065-2-shahuang@redhat.com> X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240201_233623_518636_B448F301 X-CRM114-Status: GOOD ( 18.13 ) 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 Thu, Feb 01, 2024 at 09:56:50PM -0500, Shaoqin Huang wrote: [...] > diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h > new file mode 100644 > index 000000000000..0a56183644ee > --- /dev/null > +++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include > + > +#define GICD_BASE_GPA 0x8000000ULL > +#define GICR_BASE_GPA 0x80A0000ULL Shouldn't a standardized layout of the GIC frames go with the rest of the GIC stuff? > +/* Create a VM that has one vCPU with PMUv3 configured. */ > +struct vpmu_vm *create_vpmu_vm(void *guest_code) > +{ > + struct kvm_vcpu_init init; > + uint8_t pmuver; > + uint64_t dfr0, irq = 23; > + struct kvm_device_attr irq_attr = { > + .group = KVM_ARM_VCPU_PMU_V3_CTRL, > + .attr = KVM_ARM_VCPU_PMU_V3_IRQ, > + .addr = (uint64_t)&irq, > + }; > + struct kvm_device_attr init_attr = { > + .group = KVM_ARM_VCPU_PMU_V3_CTRL, > + .attr = KVM_ARM_VCPU_PMU_V3_INIT, > + }; > + struct vpmu_vm *vpmu_vm; > + > + vpmu_vm = calloc(1, sizeof(*vpmu_vm)); > + TEST_ASSERT(vpmu_vm != NULL, "Insufficient Memory"); !vpmu_vm would be the normal way to test if a pointer is NULL. > + memset(vpmu_vm, 0, sizeof(vpmu_vm)); What? man calloc would tell you that the returned object is already zero-initalized. > + vpmu_vm->vm = vm_create(1); > + vm_init_descriptor_tables(vpmu_vm->vm); > + > + /* Create vCPU with PMUv3 */ > + vm_ioctl(vpmu_vm->vm, KVM_ARM_PREFERRED_TARGET, &init); > + init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3); > + vpmu_vm->vcpu = aarch64_vcpu_add(vpmu_vm->vm, 0, &init, guest_code); > + vcpu_init_descriptor_tables(vpmu_vm->vcpu); I extremely dislike that the VM is semi-configured by this helper. You're still expecting the caller to actually install the exception handler. > + vpmu_vm->gic_fd = vgic_v3_setup(vpmu_vm->vm, 1, 64, > + GICD_BASE_GPA, GICR_BASE_GPA); > + __TEST_REQUIRE(vpmu_vm->gic_fd >= 0, > + "Failed to create vgic-v3, skipping"); > + > + /* Make sure that PMUv3 support is indicated in the ID register */ > + vcpu_get_reg(vpmu_vm->vcpu, > + KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &dfr0); > + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), dfr0); > + TEST_ASSERT(pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && > + pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP, > + "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver); Not your code, but this assertion is meaningless. KVM does not advertise an IMP_DEF PMU to guests. > + /* Initialize vPMU */ > + vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr); > + vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr); Not your code, but these should be converted to kvm_device_attr_set() calls. Overall I'm somewhat tepid on the idea of the library being so coarse-grained. It is usually more helpful to expose finer-grained controls, like a helper that initializes the vPMU state for a preexisting VM. That way the PMU code can more easily be composed with other helpers in different tests. -- Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel