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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29753C433F5 for ; Mon, 8 Nov 2021 21:35:11 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id E2A086112F for ; Mon, 8 Nov 2021 21:35:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E2A086112F Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org 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=yBTEXQv6blTGDSTNe15lmuSq6h9nEr1UXED1SJ/FJFE=; b=echg6QypH8W5DM gym/oLZzi0jWHjXeZB9UIzAWW5TukI0CowOUShWXwUqeQx8Vq5RJe/sPtp42g6zNMcodve2eRClGy zrdPR6Z1Uo+2WJb3l2htmuUfohlXPl9jXBZRXpKB7RD+Jud6ZLdotvcbu/uA6KMB2oX+syF43oL7x 2sXjA1BDmBnHntNHMRHqB0uVAXDtr6LMcO+mOarzVDVfN7b1s1mjI7iYtfkVHWYH+FDm3+1TP1RVV dLNLG1up5+MbuBPQLxwjpnEaYvVHPNGPl0uWUKB78RBtw/cQB+D/cLzKzXXzAMT1AtTISIRtJmbxQ IWvy6e4Epy9uqyu5+V4A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkCH3-00HVI6-0v; Mon, 08 Nov 2021 21:33:53 +0000 Received: from mail-il1-x12f.google.com ([2607:f8b0:4864:20::12f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkCGx-00HVGI-K3 for linux-arm-kernel@lists.infradead.org; Mon, 08 Nov 2021 21:33:49 +0000 Received: by mail-il1-x12f.google.com with SMTP id i9so18472883ilu.8 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=XrXWtlF3pR0rl2LGtlGsdiVX6xrKduSwA1upjmy9UZu70E2zRc+eObFZ9TvcAiQjGe dgMJzwj72Q8UErq9ocZIFDBrFVbXq/1TeaFWOTjsLHSA87Z1RGhmwISWN8EZlAekllSO mlSmIe5nlp20HoQAt4efOEpM+EIqUc8TEkOWrTSm8xOSE/va9U6sUcJ/OAY5I1j5fMnb Fmf/GlWTipIx/hAvfUqinMux5SFFHCfRteM26y2ujp4Lbuzt2H8lgtxYN1HrBub4j8ld p25uIuHEiULmCeNssgZv/jtZjUzoE+L30uuim4HhexFUsWmUIrSi5tEPIGuuXo7TtRb8 +YUw== X-Gm-Message-State: AOAM531/QUyjwaKT1wByLniLPjxploqbYw8B5JUQrxVjj7xXqu+Vayod mIsRofWKX83hahkQa/nFXpXnUQ== X-Google-Smtp-Source: ABdhPJzjPhy8/nW/9RIzLXtQxLZfiK10pduutrQX2tnPGXqQ62lKvQDM2Oe1tiRS22zzVFnb21gvuw== 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-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211108_133347_698864_99BEE742 X-CRM114-Status: GOOD ( 41.41 ) 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, 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel