Received: by 2002:a05:6a10:5bc5:0:0:0:0 with SMTP id os5csp482498pxb; Mon, 8 Nov 2021 17:06:09 -0800 (PST) X-Google-Smtp-Source: ABdhPJz+AMWaFFniRPx5cWhMD5tAFNNlOR7IOkz/juih3sv+ZSYtg5PUMKQhqVMsF8l76SJHGcN8 X-Received: by 2002:a17:906:c9cb:: with SMTP id hk11mr4356527ejb.204.1636419969362; Mon, 08 Nov 2021 17:06:09 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1636419969; cv=none; d=google.com; s=arc-20160816; b=febsKLAG58Kj5qkzCzCDKWxLG/wzrLEMuOQ1Yi+WtqOj8UTujMwHz9/xbGheEKLaqo oZA1v44D4edKijqqsyUiom0+Yf+U+FLF1r6Bp708BteZrk9OWOC2IVosPjFkEO1kBJhO 9Kkes5Hsn+B36aKTO4ygdH/kaYWB3PokeIU5TkWCojuNiSXGSzIYHHC87PI+pbjikq5w iik7EUrdtPPkMUgwH248TuMR6VXbmarISrFqD3qBulujpqmrNlCGdeBSCk8/uKb2bcA9 HY3x+2NL/CX++o6LlNZX0kWQNubDpAJk4PgwX/X6+l9RTi89F6yTila3i6lJGzfczkXH lYGA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=hFSV1mBws9eLeLO5kPMmlLtWrmSlD924TaH/pakFmoU=; b=PM1IGrJoJfhzUSbcy8FgTV39K1bYnE7quX8h/55pQlf5KOeHG+P007pjw6YiFoEhJ8 j3SZgwov3ZyzhMl9g75K3LLRd8MzNoEcD98jnmw+nJZGfbUlOc/vJZhcGiYY4nHTu2Kd zq65PCvnMRAfxTy7Ki8MoSDaO3OTSnhDeAaSwOXVadDslyZkWkqwoc2VPTDricYRk9QD 2CKiYV/jnRx+fajCpbszhDa8sfCVkEQzKXe4d1XIN71LNYQJZ5TuNGgr1ghlrblRL5Qp kQ9UIYIBuRvvDNmeUVbeUz27hsENld9D0Ykd918VDfcGAXEU84HJhWjnQmg7hPs3gZez llog== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="O7/fYiny"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id u4si14261977ejy.603.2021.11.08.17.05.42; Mon, 08 Nov 2021 17:06:09 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="O7/fYiny"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240434AbhKHVgX (ORCPT + 99 others); Mon, 8 Nov 2021 16:36:23 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53142 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240367AbhKHVgW (ORCPT ); Mon, 8 Nov 2021 16:36:22 -0500 Received: from mail-il1-x12a.google.com (mail-il1-x12a.google.com [IPv6:2607:f8b0:4864:20::12a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14077C061714 for ; Mon, 8 Nov 2021 13:33:38 -0800 (PST) Received: by mail-il1-x12a.google.com with SMTP id j28so18500064ila.1 for ; Mon, 08 Nov 2021 13:33:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=hFSV1mBws9eLeLO5kPMmlLtWrmSlD924TaH/pakFmoU=; b=O7/fYinykmp/KyEQVR3JRt816V+8zD9gdBIlkyp3lTFLY/wKMfxdp7IVHhrIv/GSz9 bosru1a/4zIf0Dp3NEv9gwxuV1MP3HEVomdXK7nf0ciuQXqQSwaiMJ/fqzudASPJQRb0 AuyZzjiupsk0i+PgdPFaz8nTBRKhGEoAp29RScs9pyG0d8RWLvWTSv8XkaVhjGgHeMAl hj0mPf/gARZOMHKMXPzGUtAiaX6nrQbeFliJbfaa0DFCx3/LKw2HBPWxgmB7VIPkpQ0g j80tXIUKKWHBKfTzcww2g+aGHCatQqgcDjjhcznu44WID6chwqvLvwCah5B4E4hj77iO 2LQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=hFSV1mBws9eLeLO5kPMmlLtWrmSlD924TaH/pakFmoU=; b=ml8JSj7Q/yNY7aNV2TxqgcjC6RFo6I3T74E6yTUg8CxKpyPw1wqmg3bMSN5FJO+2mb orNehYRjdomXB8wThPrD1Z6Oc3UsiGCE8cyyD9birNo09cAM3IaPkH0pQG2hceGTAwYp PRnXf9xRoN4TzQaQGCtohCwc97RX4CftkhiWxv69wnx4LcBqM1DJFB5Zqo2iyRorgOqH Vrb1q9Smlyvmc4tAc+ipgZsfbQHSKUep7BJaQcrJrOfn1X5OHNCwRLBwW8RbPbCqLpkj jXKcQ8CAOjS0Z/ygtHE17lcd2SWGVWQn6vStRDN7XFeD7Vy3mw0QqwZDflhrm9wzD0fD mi3A== X-Gm-Message-State: AOAM533OwPab4JerbMDKBE4otxAqV3rI2elAKz/I6qk8pG0Y4MV10Iy1 dFnvOmUq0VhVysMiWLPwAn3jOA== X-Received: by 2002:a05:6e02:2166:: with SMTP id s6mr1565008ilv.170.1636407217245; Mon, 08 Nov 2021 13:33:37 -0800 (PST) Received: from google.com (194.225.68.34.bc.googleusercontent.com. [34.68.225.194]) by smtp.gmail.com with ESMTPSA id x11sm945804iop.55.2021.11.08.13.33.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Nov 2021 13:33:36 -0800 (PST) Date: Mon, 8 Nov 2021 21:33:33 +0000 From: Oliver Upton To: Raghavendra Rao Ananta Cc: Marc Zyngier , Andrew Jones , James Morse , Alexandru Elisei , Suzuki K Poulose , Catalin Marinas , Will Deacon , Peter Shier , Ricardo Koller , Reiji Watanabe , Jing Zhang , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [RFC PATCH 1/8] KVM: arm64: Factor out firmware register handling from psci.c Message-ID: References: <20211102002203.1046069-1-rananta@google.com> <20211102002203.1046069-2-rananta@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 04, 2021 at 10:16:21AM -0700, Raghavendra Rao Ananta wrote: > Hi Oliver, > > On Wed, Nov 3, 2021 at 2:43 PM Oliver Upton wrote: > > > > Hi Raghu, > > > > On Tue, Nov 02, 2021 at 12:21:56AM +0000, Raghavendra Rao Ananta wrote: > > > Common hypercall firmware register handing is currently employed > > > by psci.c. Since the upcoming patches add more of these registers, > > > it's better to move the generic handling to hypercall.c for a > > > cleaner presentation. > > > > > > While we are at it, collect all the firmware registers under > > > fw_reg_ids[] to help implement kvm_arm_get_fw_num_regs() and > > > kvm_arm_copy_fw_reg_indices() in a generic way. > > > > > > No functional change intended. > > > > > > Signed-off-by: Raghavendra Rao Ananta > > > --- > > > arch/arm64/kvm/guest.c | 2 +- > > > arch/arm64/kvm/hypercalls.c | 151 +++++++++++++++++++++++++++++++ > > > arch/arm64/kvm/psci.c | 167 +++-------------------------------- > > > include/kvm/arm_hypercalls.h | 7 ++ > > > include/kvm/arm_psci.h | 8 +- > > > 5 files changed, 172 insertions(+), 163 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > index 5ce26bedf23c..625f97f7b304 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -18,7 +18,7 @@ > > > #include > > > #include > > > #include > > > -#include > > > +#include > > > #include > > > #include > > > #include > > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > > > index 30da78f72b3b..d030939c5929 100644 > > > --- a/arch/arm64/kvm/hypercalls.c > > > +++ b/arch/arm64/kvm/hypercalls.c > > > @@ -146,3 +146,154 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) > > > smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]); > > > return 1; > > > } > > > + > > > +static const u64 fw_reg_ids[] = { > > > + KVM_REG_ARM_PSCI_VERSION, > > > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, > > > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, > > > +}; > > > + > > > +int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu) > > > +{ > > > + return ARRAY_SIZE(fw_reg_ids); > > > +} > > > + > > > +int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) { > > > + if (put_user(fw_reg_ids[i], uindices)) > > > + return -EFAULT; > > > + } > > > + > > > + return 0; > > > +} > > > > It would appear that this patch is separating out the hypercall services > > to each handle their own FW regs. At the same time, this is > > consolidating the register enumeration into a single place. > > > > It would be nice to keep the scoping consistent with your accessors > > below, or simply just handle all regs in hypercalls.c. Abstracting > > per-service might result in a lot of boilerplate, though. > > > It's neither here nor there, unfortunately, because of how the fw > registers exists. We have a dedicated fw register for psci and a file > of its own (psci.c). Some of the other services, such as TRNG, have > their own file, but because of the bitmap design, they won't have > their own fw register. And the ARCH_WORKAROUND have their dedicated > registers, but no file of their own. So, at best I was aiming to push > all the things relevant to a service in its own file (psci for > example), just to have a better file-context, while leaving others > (and generic handling stuff) in hypercall.c. > > Just to maintain consistency, I can create a dedicated file for the > ARCH_WORKAROUND registers, if you feel that's better. > Perhaps the easiest thing to do would be to keep all firmware ID registers in one place, much like we do for the ARM feature ID regs in sys_regs.c. > > > +#define KVM_REG_FEATURE_LEVEL_WIDTH 4 > > > +#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1) > > > + > > > +/* > > > + * Convert the workaround level into an easy-to-compare number, where higher > > > + * values mean better protection. > > > + */ > > > +static int get_kernel_wa_level(u64 regid) > > > +{ > > > + switch (regid) { > > > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1: > > > + switch (arm64_get_spectre_v2_state()) { > > > + case SPECTRE_VULNERABLE: > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL; > > > + case SPECTRE_MITIGATED: > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL; > > > + case SPECTRE_UNAFFECTED: > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED; > > > + } > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL; > > > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2: > > > + switch (arm64_get_spectre_v4_state()) { > > > + case SPECTRE_MITIGATED: > > > + /* > > > + * As for the hypercall discovery, we pretend we > > > + * don't have any FW mitigation if SSBS is there at > > > + * all times. > > > + */ > > > + if (cpus_have_final_cap(ARM64_SSBS)) > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL; > > > + fallthrough; > > > + case SPECTRE_UNAFFECTED: > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED; > > > + case SPECTRE_VULNERABLE: > > > + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL; > > > + } > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > +{ > > > + void __user *uaddr = (void __user *)(long)reg->addr; > > > + u64 val; > > > + > > > + switch (reg->id) { > > > + case KVM_REG_ARM_PSCI_VERSION: > > > + val = kvm_psci_version(vcpu, vcpu->kvm); > > > > Should this become kvm_arm_get_fw_reg() to consistently genericize the > > PSCI FW register accessors? > > > Sorry, I didn't follow. Did you mean, "kvm_arm_get_psci_fw_reg()"? Right :) Of course, this could become irrelevant depending on how you address scoping of the FW regs. -- Thanks, Oliver