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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AFBA7C433E0 for ; Tue, 22 Dec 2020 16:06:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6D2F322955 for ; Tue, 22 Dec 2020 16:06:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727762AbgLVQGU convert rfc822-to-8bit (ORCPT ); Tue, 22 Dec 2020 11:06:20 -0500 Received: from mail.kernel.org ([198.145.29.99]:55538 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727321AbgLVQGT (ORCPT ); Tue, 22 Dec 2020 11:06:19 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 7E7FB22955; Tue, 22 Dec 2020 16:05:38 +0000 (UTC) Received: from 91-161-240-24.subs.proxad.net ([91.161.240.24] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1krkAK-0037KV-7U; Tue, 22 Dec 2020 16:05:36 +0000 Date: Tue, 22 Dec 2020 16:05:35 +0000 Message-ID: <87r1nhrflc.wl-maz@kernel.org> From: Marc Zyngier To: Mark Rutland Cc: David Brazdil , kvmarm@lists.cs.columbia.edu, James Morse , Julien Thierry , Suzuki K Poulose , Catalin Marinas , Will Deacon , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208172628.GB18222@C02TD0UTHF1T.local> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> <20201208172628.GB18222@C02TD0UTHF1T.local> 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 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT X-SA-Exim-Connect-IP: 91.161.240.24 X-SA-Exim-Rcpt-To: mark.rutland@arm.com, dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 08 Dec 2020 17:26:28 +0000, Mark Rutland wrote: > > On Tue, Dec 08, 2020 at 03:56:39PM +0000, Marc Zyngier wrote: > > On 2020-12-08 14:24, David Brazdil wrote: > > > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > > > configured in the DT. However, the struct does not convey the > > > information whether these were set from DT or contain the default value > > > zero. This could be a problem for PSCI proxy in KVM protected mode. > > > > > > Extend config passed to KVM with a bit mask with individual bits set > > > depending on whether the corresponding function pointer in psci_ops is > > > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > > > > > Previously config was split into multiple global variables. Put > > > everything into a single struct for convenience. > > > > > > Reported-by: Mark Rutland > > > Signed-off-by: David Brazdil > > > --- > > > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > > > arch/arm64/kvm/arm.c | 14 +++++--- > > > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > > > 3 files changed, 70 insertions(+), 17 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h > > > b/arch/arm64/include/asm/kvm_host.h > > > index 11beda85ee7e..828d50d40dc2 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -240,6 +241,25 @@ struct kvm_host_data { > > > struct kvm_pmu_events pmu_events; > > > }; > > > > > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > > > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > > > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > > > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > > > + > > > +struct kvm_host_psci_config { > > > + /* PSCI version used by host. */ > > > + u32 version; > > > + > > > + /* Function IDs used by host if version is v0.1. */ > > > + struct psci_0_1_function_ids function_ids_0_1; > > > + > > > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ > > > + unsigned int enabled_functions_0_1; > > > > Nit: the conventional type for bitmaps is 'unsigned long'. > > Also, "enabled" seems odd. Isn't it actually "available"? > > Sure, that or "implemented" works here. > > Since there are only 4 functions here, it might make sense to use > independent bools rather than a bitmap, which might make this a bit > simpler... > > > > get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.version = psci_ops.get_version(); > > > + > > > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > > > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.enabled_functions_0_1 = > > > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > > > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > > > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > > > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > > ... since e.g. this could be roughly: > > kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend; > kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off; > kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on; > kvm_host_psci_config.migrate_implemented = psci_ops.migrate; > > > > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > > > +{ > > > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > > > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); > > > +} > > ...and similarly: > > return kvm_host_psci_config.cpu_suspend_implemented && > func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend) > > > Otherwise looks OK. Don't bother respinning the series for my > > comments, I can tidy things up as I apply it if there are no other > > issues. > > FWIW, I'm happy with whatever choose to do here, so don't feel like you > have to follow my suggestions above. FWIW, I've queued the following patch on top of the series, using the above suggestions and some creative use of macros, allowing some cleanup. M. >From 767c973f2e4a9264a4f159c9fad5ca8acdb9915e Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Tue, 22 Dec 2020 12:46:41 +0000 Subject: [PATCH] KVM: arm64: Declutter host PSCI 0.1 handling Although there is nothing wrong with the current host PSCI relay implementation, we can clean it up and remove some of the helpers that do not improve the overall readability of the legacy PSCI 0.1 handling. Opportunity is taken to turn the bitmap into a set of booleans, and creative use of preprocessor macros make init and check more concise/readable. Suggested-by: Mark Rutland Signed-off-by: Marc Zyngier --- arch/arm64/include/asm/kvm_host.h | 11 ++-- arch/arm64/kvm/arm.c | 12 +++-- arch/arm64/kvm/hyp/nvhe/psci-relay.c | 77 +++++++--------------------- 3 files changed, 30 insertions(+), 70 deletions(-) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index bce2452b305c..8fcfab0c2567 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -241,11 +241,6 @@ struct kvm_host_data { struct kvm_pmu_events pmu_events; }; -#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) -#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) -#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) -#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) - struct kvm_host_psci_config { /* PSCI version used by host. */ u32 version; @@ -253,8 +248,10 @@ struct kvm_host_psci_config { /* Function IDs used by host if version is v0.1. */ struct psci_0_1_function_ids function_ids_0_1; - /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ - unsigned int enabled_functions_0_1; + bool psci_0_1_cpu_suspend_implemented; + bool psci_0_1_cpu_on_implemented; + bool psci_0_1_cpu_off_implemented; + bool psci_0_1_migrate_implemented; }; extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 836ca763b91d..e207e4541f55 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1603,6 +1603,9 @@ static void init_cpu_logical_map(void) hyp_cpu_logical_map[cpu] = cpu_logical_map(cpu); } +#define init_psci_0_1_impl_state(config, what) \ + config.psci_0_1_ ## what ## _implemented = psci_ops.what + static bool init_psci_relay(void) { /* @@ -1618,11 +1621,10 @@ static bool init_psci_relay(void) if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); - kvm_host_psci_config.enabled_functions_0_1 = - (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | - (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | - (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | - (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_suspend); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_on); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_off); + init_psci_0_1_impl_state(kvm_host_psci_config, migrate); } return true; } diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c index 1f7237e45148..e3947846ffcb 100644 --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c @@ -43,48 +43,16 @@ struct psci_boot_args { static DEFINE_PER_CPU(struct psci_boot_args, cpu_on_args) = PSCI_BOOT_ARGS_INIT; static DEFINE_PER_CPU(struct psci_boot_args, suspend_args) = PSCI_BOOT_ARGS_INIT; -static u64 get_psci_func_id(struct kvm_cpu_context *host_ctxt) -{ - DECLARE_REG(u64, func_id, host_ctxt, 0); - - return func_id; -} - -static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) -{ - return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; -} - -static inline bool is_psci_0_1_cpu_suspend(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); -} - -static inline bool is_psci_0_1_cpu_on(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); -} - -static inline bool is_psci_0_1_cpu_off(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); -} - -static inline bool is_psci_0_1_migrate(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && - (func_id == kvm_host_psci_config.function_ids_0_1.migrate); -} +#define is_psci_0_1(what, func_id) \ + (kvm_host_psci_config.psci_0_1_ ## what ## _implemented && \ + (func_id) == kvm_host_psci_config.function_ids_0_1.what) static bool is_psci_0_1_call(u64 func_id) { - return is_psci_0_1_cpu_suspend(func_id) || - is_psci_0_1_cpu_on(func_id) || - is_psci_0_1_cpu_off(func_id) || - is_psci_0_1_migrate(func_id); + return (is_psci_0_1(cpu_suspend, func_id) || + is_psci_0_1(cpu_on, func_id) || + is_psci_0_1(cpu_off, func_id) || + is_psci_0_1(migrate, func_id)); } static bool is_psci_0_2_call(u64 func_id) @@ -94,16 +62,6 @@ static bool is_psci_0_2_call(u64 func_id) (PSCI_0_2_FN64(0) <= func_id && func_id <= PSCI_0_2_FN64(31)); } -static bool is_psci_call(u64 func_id) -{ - switch (kvm_host_psci_config.version) { - case PSCI_VERSION(0, 1): - return is_psci_0_1_call(func_id); - default: - return is_psci_0_2_call(func_id); - } -} - static unsigned long psci_call(unsigned long fn, unsigned long arg0, unsigned long arg1, unsigned long arg2) { @@ -273,14 +231,14 @@ asmlinkage void __noreturn kvm_host_psci_cpu_entry(bool is_cpu_on) static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) { - if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) + if (is_psci_0_1(cpu_off, func_id) || is_psci_0_1(migrate, func_id)) return psci_forward(host_ctxt); - else if (is_psci_0_1_cpu_on(func_id)) + if (is_psci_0_1(cpu_on, func_id)) return psci_cpu_on(func_id, host_ctxt); - else if (is_psci_0_1_cpu_suspend(func_id)) + if (is_psci_0_1(cpu_suspend, func_id)) return psci_cpu_suspend(func_id, host_ctxt); - else - return PSCI_RET_NOT_SUPPORTED; + + return PSCI_RET_NOT_SUPPORTED; } static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) @@ -322,20 +280,23 @@ static unsigned long psci_1_0_handler(u64 func_id, struct kvm_cpu_context *host_ bool kvm_host_psci_handler(struct kvm_cpu_context *host_ctxt) { - u64 func_id = get_psci_func_id(host_ctxt); + DECLARE_REG(u64, func_id, host_ctxt, 0); unsigned long ret; - if (!is_psci_call(func_id)) - return false; - switch (kvm_host_psci_config.version) { case PSCI_VERSION(0, 1): + if (!is_psci_0_1_call(func_id)) + return false; ret = psci_0_1_handler(func_id, host_ctxt); break; case PSCI_VERSION(0, 2): + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_0_2_handler(func_id, host_ctxt); break; default: + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_1_0_handler(func_id, host_ctxt); break; } -- 2.29.2 -- Without deviation from the norm, progress is not possible. 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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DA763C433E0 for ; Tue, 22 Dec 2020 16:05:44 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 5C70923124 for ; Tue, 22 Dec 2020 16:05:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5C70923124 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id A56D34B2A6; Tue, 22 Dec 2020 11:05:43 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jD2L1LtGyj+L; Tue, 22 Dec 2020 11:05:42 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 160084B2B3; Tue, 22 Dec 2020 11:05:42 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 6A8BB4B2A6 for ; Tue, 22 Dec 2020 11:05:41 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6anjimK1E+IM for ; Tue, 22 Dec 2020 11:05:40 -0500 (EST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id E01054A4A0 for ; Tue, 22 Dec 2020 11:05:39 -0500 (EST) Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 7E7FB22955; Tue, 22 Dec 2020 16:05:38 +0000 (UTC) Received: from 91-161-240-24.subs.proxad.net ([91.161.240.24] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1krkAK-0037KV-7U; Tue, 22 Dec 2020 16:05:36 +0000 Date: Tue, 22 Dec 2020 16:05:35 +0000 Message-ID: <87r1nhrflc.wl-maz@kernel.org> From: Marc Zyngier To: Mark Rutland Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208172628.GB18222@C02TD0UTHF1T.local> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> <20201208172628.GB18222@C02TD0UTHF1T.local> 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 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 91.161.240.24 X-SA-Exim-Rcpt-To: mark.rutland@arm.com, dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: kernel-team@android.com, Catalin Marinas , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon , kvmarm@lists.cs.columbia.edu X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Tue, 08 Dec 2020 17:26:28 +0000, Mark Rutland wrote: > > On Tue, Dec 08, 2020 at 03:56:39PM +0000, Marc Zyngier wrote: > > On 2020-12-08 14:24, David Brazdil wrote: > > > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > > > configured in the DT. However, the struct does not convey the > > > information whether these were set from DT or contain the default value > > > zero. This could be a problem for PSCI proxy in KVM protected mode. > > > > > > Extend config passed to KVM with a bit mask with individual bits set > > > depending on whether the corresponding function pointer in psci_ops is > > > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > > > > > Previously config was split into multiple global variables. Put > > > everything into a single struct for convenience. > > > > > > Reported-by: Mark Rutland > > > Signed-off-by: David Brazdil > > > --- > > > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > > > arch/arm64/kvm/arm.c | 14 +++++--- > > > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > > > 3 files changed, 70 insertions(+), 17 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h > > > b/arch/arm64/include/asm/kvm_host.h > > > index 11beda85ee7e..828d50d40dc2 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -240,6 +241,25 @@ struct kvm_host_data { > > > struct kvm_pmu_events pmu_events; > > > }; > > > > > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > > > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > > > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > > > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > > > + > > > +struct kvm_host_psci_config { > > > + /* PSCI version used by host. */ > > > + u32 version; > > > + > > > + /* Function IDs used by host if version is v0.1. */ > > > + struct psci_0_1_function_ids function_ids_0_1; > > > + > > > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ > > > + unsigned int enabled_functions_0_1; > > > > Nit: the conventional type for bitmaps is 'unsigned long'. > > Also, "enabled" seems odd. Isn't it actually "available"? > > Sure, that or "implemented" works here. > > Since there are only 4 functions here, it might make sense to use > independent bools rather than a bitmap, which might make this a bit > simpler... > > > > get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.version = psci_ops.get_version(); > > > + > > > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > > > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.enabled_functions_0_1 = > > > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > > > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > > > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > > > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > > ... since e.g. this could be roughly: > > kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend; > kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off; > kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on; > kvm_host_psci_config.migrate_implemented = psci_ops.migrate; > > > > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > > > +{ > > > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > > > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); > > > +} > > ...and similarly: > > return kvm_host_psci_config.cpu_suspend_implemented && > func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend) > > > Otherwise looks OK. Don't bother respinning the series for my > > comments, I can tidy things up as I apply it if there are no other > > issues. > > FWIW, I'm happy with whatever choose to do here, so don't feel like you > have to follow my suggestions above. FWIW, I've queued the following patch on top of the series, using the above suggestions and some creative use of macros, allowing some cleanup. M. >From 767c973f2e4a9264a4f159c9fad5ca8acdb9915e Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Tue, 22 Dec 2020 12:46:41 +0000 Subject: [PATCH] KVM: arm64: Declutter host PSCI 0.1 handling Although there is nothing wrong with the current host PSCI relay implementation, we can clean it up and remove some of the helpers that do not improve the overall readability of the legacy PSCI 0.1 handling. Opportunity is taken to turn the bitmap into a set of booleans, and creative use of preprocessor macros make init and check more concise/readable. Suggested-by: Mark Rutland Signed-off-by: Marc Zyngier --- arch/arm64/include/asm/kvm_host.h | 11 ++-- arch/arm64/kvm/arm.c | 12 +++-- arch/arm64/kvm/hyp/nvhe/psci-relay.c | 77 +++++++--------------------- 3 files changed, 30 insertions(+), 70 deletions(-) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index bce2452b305c..8fcfab0c2567 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -241,11 +241,6 @@ struct kvm_host_data { struct kvm_pmu_events pmu_events; }; -#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) -#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) -#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) -#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) - struct kvm_host_psci_config { /* PSCI version used by host. */ u32 version; @@ -253,8 +248,10 @@ struct kvm_host_psci_config { /* Function IDs used by host if version is v0.1. */ struct psci_0_1_function_ids function_ids_0_1; - /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ - unsigned int enabled_functions_0_1; + bool psci_0_1_cpu_suspend_implemented; + bool psci_0_1_cpu_on_implemented; + bool psci_0_1_cpu_off_implemented; + bool psci_0_1_migrate_implemented; }; extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 836ca763b91d..e207e4541f55 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1603,6 +1603,9 @@ static void init_cpu_logical_map(void) hyp_cpu_logical_map[cpu] = cpu_logical_map(cpu); } +#define init_psci_0_1_impl_state(config, what) \ + config.psci_0_1_ ## what ## _implemented = psci_ops.what + static bool init_psci_relay(void) { /* @@ -1618,11 +1621,10 @@ static bool init_psci_relay(void) if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); - kvm_host_psci_config.enabled_functions_0_1 = - (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | - (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | - (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | - (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_suspend); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_on); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_off); + init_psci_0_1_impl_state(kvm_host_psci_config, migrate); } return true; } diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c index 1f7237e45148..e3947846ffcb 100644 --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c @@ -43,48 +43,16 @@ struct psci_boot_args { static DEFINE_PER_CPU(struct psci_boot_args, cpu_on_args) = PSCI_BOOT_ARGS_INIT; static DEFINE_PER_CPU(struct psci_boot_args, suspend_args) = PSCI_BOOT_ARGS_INIT; -static u64 get_psci_func_id(struct kvm_cpu_context *host_ctxt) -{ - DECLARE_REG(u64, func_id, host_ctxt, 0); - - return func_id; -} - -static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) -{ - return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; -} - -static inline bool is_psci_0_1_cpu_suspend(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); -} - -static inline bool is_psci_0_1_cpu_on(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); -} - -static inline bool is_psci_0_1_cpu_off(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); -} - -static inline bool is_psci_0_1_migrate(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && - (func_id == kvm_host_psci_config.function_ids_0_1.migrate); -} +#define is_psci_0_1(what, func_id) \ + (kvm_host_psci_config.psci_0_1_ ## what ## _implemented && \ + (func_id) == kvm_host_psci_config.function_ids_0_1.what) static bool is_psci_0_1_call(u64 func_id) { - return is_psci_0_1_cpu_suspend(func_id) || - is_psci_0_1_cpu_on(func_id) || - is_psci_0_1_cpu_off(func_id) || - is_psci_0_1_migrate(func_id); + return (is_psci_0_1(cpu_suspend, func_id) || + is_psci_0_1(cpu_on, func_id) || + is_psci_0_1(cpu_off, func_id) || + is_psci_0_1(migrate, func_id)); } static bool is_psci_0_2_call(u64 func_id) @@ -94,16 +62,6 @@ static bool is_psci_0_2_call(u64 func_id) (PSCI_0_2_FN64(0) <= func_id && func_id <= PSCI_0_2_FN64(31)); } -static bool is_psci_call(u64 func_id) -{ - switch (kvm_host_psci_config.version) { - case PSCI_VERSION(0, 1): - return is_psci_0_1_call(func_id); - default: - return is_psci_0_2_call(func_id); - } -} - static unsigned long psci_call(unsigned long fn, unsigned long arg0, unsigned long arg1, unsigned long arg2) { @@ -273,14 +231,14 @@ asmlinkage void __noreturn kvm_host_psci_cpu_entry(bool is_cpu_on) static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) { - if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) + if (is_psci_0_1(cpu_off, func_id) || is_psci_0_1(migrate, func_id)) return psci_forward(host_ctxt); - else if (is_psci_0_1_cpu_on(func_id)) + if (is_psci_0_1(cpu_on, func_id)) return psci_cpu_on(func_id, host_ctxt); - else if (is_psci_0_1_cpu_suspend(func_id)) + if (is_psci_0_1(cpu_suspend, func_id)) return psci_cpu_suspend(func_id, host_ctxt); - else - return PSCI_RET_NOT_SUPPORTED; + + return PSCI_RET_NOT_SUPPORTED; } static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) @@ -322,20 +280,23 @@ static unsigned long psci_1_0_handler(u64 func_id, struct kvm_cpu_context *host_ bool kvm_host_psci_handler(struct kvm_cpu_context *host_ctxt) { - u64 func_id = get_psci_func_id(host_ctxt); + DECLARE_REG(u64, func_id, host_ctxt, 0); unsigned long ret; - if (!is_psci_call(func_id)) - return false; - switch (kvm_host_psci_config.version) { case PSCI_VERSION(0, 1): + if (!is_psci_0_1_call(func_id)) + return false; ret = psci_0_1_handler(func_id, host_ctxt); break; case PSCI_VERSION(0, 2): + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_0_2_handler(func_id, host_ctxt); break; default: + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_1_0_handler(func_id, host_ctxt); break; } -- 2.29.2 -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4D499C433E0 for ; Tue, 22 Dec 2020 16:06:58 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 EC550229C5 for ; Tue, 22 Dec 2020 16:06:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EC550229C5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject: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=DnzzukgIWTQWEwMb2E1SceRRYDN33t2E8/KCNaQaBFs=; b=eCACcFWu0+XncITD1mFgbSpWA aBeUszNdubGXLCPQoZHr7FL+giy7yjWzURYFk8569ldKjnJwyf6IgaUfdvY8a80IIsZrxJMrm56pH mEGpwv0IKHnEeYkPl6BzlETsUupeaU2acO5TqfSmOb1Sd/fSV7uX7CDI3Gc8GNhCwcw3xavFCgaJp u7Xw0RSKMVaNdjgNmhh/D74Rqq4yf11pySR4Q5A/1P1PMpcoSYRiaBCoZh1Am7KJ5jaRaZmAME/Km tGC3WQ5R8lwteDE3BQrf8/yHqGfEGhD3jye8/4BkNad9Bo8ETPwLkR2wWWBT7jD/YvM1r0qIHnQWi qACVbsR7w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1krkAP-0008Vb-WF; Tue, 22 Dec 2020 16:05:42 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1krkAN-0008V7-F6 for linux-arm-kernel@lists.infradead.org; Tue, 22 Dec 2020 16:05:40 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 7E7FB22955; Tue, 22 Dec 2020 16:05:38 +0000 (UTC) Received: from 91-161-240-24.subs.proxad.net ([91.161.240.24] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1krkAK-0037KV-7U; Tue, 22 Dec 2020 16:05:36 +0000 Date: Tue, 22 Dec 2020 16:05:35 +0000 Message-ID: <87r1nhrflc.wl-maz@kernel.org> From: Marc Zyngier To: Mark Rutland Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208172628.GB18222@C02TD0UTHF1T.local> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> <20201208172628.GB18222@C02TD0UTHF1T.local> 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 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 91.161.240.24 X-SA-Exim-Rcpt-To: mark.rutland@arm.com, dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com 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-20201222_110539_634486_9C84CE5E X-CRM114-Status: GOOD ( 44.15 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kernel-team@android.com, Suzuki K Poulose , Catalin Marinas , linux-kernel@vger.kernel.org, James Morse , linux-arm-kernel@lists.infradead.org, David Brazdil , Will Deacon , kvmarm@lists.cs.columbia.edu, Julien Thierry 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 Tue, 08 Dec 2020 17:26:28 +0000, Mark Rutland wrote: > > On Tue, Dec 08, 2020 at 03:56:39PM +0000, Marc Zyngier wrote: > > On 2020-12-08 14:24, David Brazdil wrote: > > > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > > > configured in the DT. However, the struct does not convey the > > > information whether these were set from DT or contain the default value > > > zero. This could be a problem for PSCI proxy in KVM protected mode. > > > > > > Extend config passed to KVM with a bit mask with individual bits set > > > depending on whether the corresponding function pointer in psci_ops is > > > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > > > > > Previously config was split into multiple global variables. Put > > > everything into a single struct for convenience. > > > > > > Reported-by: Mark Rutland > > > Signed-off-by: David Brazdil > > > --- > > > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > > > arch/arm64/kvm/arm.c | 14 +++++--- > > > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > > > 3 files changed, 70 insertions(+), 17 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h > > > b/arch/arm64/include/asm/kvm_host.h > > > index 11beda85ee7e..828d50d40dc2 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -240,6 +241,25 @@ struct kvm_host_data { > > > struct kvm_pmu_events pmu_events; > > > }; > > > > > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > > > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > > > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > > > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > > > + > > > +struct kvm_host_psci_config { > > > + /* PSCI version used by host. */ > > > + u32 version; > > > + > > > + /* Function IDs used by host if version is v0.1. */ > > > + struct psci_0_1_function_ids function_ids_0_1; > > > + > > > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ > > > + unsigned int enabled_functions_0_1; > > > > Nit: the conventional type for bitmaps is 'unsigned long'. > > Also, "enabled" seems odd. Isn't it actually "available"? > > Sure, that or "implemented" works here. > > Since there are only 4 functions here, it might make sense to use > independent bools rather than a bitmap, which might make this a bit > simpler... > > > > get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.version = psci_ops.get_version(); > > > + > > > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > > > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > > > + kvm_host_psci_config.enabled_functions_0_1 = > > > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > > > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > > > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > > > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > > ... since e.g. this could be roughly: > > kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend; > kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off; > kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on; > kvm_host_psci_config.migrate_implemented = psci_ops.migrate; > > > > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > > > +{ > > > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > > > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); > > > +} > > ...and similarly: > > return kvm_host_psci_config.cpu_suspend_implemented && > func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend) > > > Otherwise looks OK. Don't bother respinning the series for my > > comments, I can tidy things up as I apply it if there are no other > > issues. > > FWIW, I'm happy with whatever choose to do here, so don't feel like you > have to follow my suggestions above. FWIW, I've queued the following patch on top of the series, using the above suggestions and some creative use of macros, allowing some cleanup. M. >From 767c973f2e4a9264a4f159c9fad5ca8acdb9915e Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Tue, 22 Dec 2020 12:46:41 +0000 Subject: [PATCH] KVM: arm64: Declutter host PSCI 0.1 handling Although there is nothing wrong with the current host PSCI relay implementation, we can clean it up and remove some of the helpers that do not improve the overall readability of the legacy PSCI 0.1 handling. Opportunity is taken to turn the bitmap into a set of booleans, and creative use of preprocessor macros make init and check more concise/readable. Suggested-by: Mark Rutland Signed-off-by: Marc Zyngier --- arch/arm64/include/asm/kvm_host.h | 11 ++-- arch/arm64/kvm/arm.c | 12 +++-- arch/arm64/kvm/hyp/nvhe/psci-relay.c | 77 +++++++--------------------- 3 files changed, 30 insertions(+), 70 deletions(-) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index bce2452b305c..8fcfab0c2567 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -241,11 +241,6 @@ struct kvm_host_data { struct kvm_pmu_events pmu_events; }; -#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) -#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) -#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) -#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) - struct kvm_host_psci_config { /* PSCI version used by host. */ u32 version; @@ -253,8 +248,10 @@ struct kvm_host_psci_config { /* Function IDs used by host if version is v0.1. */ struct psci_0_1_function_ids function_ids_0_1; - /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ - unsigned int enabled_functions_0_1; + bool psci_0_1_cpu_suspend_implemented; + bool psci_0_1_cpu_on_implemented; + bool psci_0_1_cpu_off_implemented; + bool psci_0_1_migrate_implemented; }; extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 836ca763b91d..e207e4541f55 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1603,6 +1603,9 @@ static void init_cpu_logical_map(void) hyp_cpu_logical_map[cpu] = cpu_logical_map(cpu); } +#define init_psci_0_1_impl_state(config, what) \ + config.psci_0_1_ ## what ## _implemented = psci_ops.what + static bool init_psci_relay(void) { /* @@ -1618,11 +1621,10 @@ static bool init_psci_relay(void) if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); - kvm_host_psci_config.enabled_functions_0_1 = - (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | - (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | - (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | - (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_suspend); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_on); + init_psci_0_1_impl_state(kvm_host_psci_config, cpu_off); + init_psci_0_1_impl_state(kvm_host_psci_config, migrate); } return true; } diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c index 1f7237e45148..e3947846ffcb 100644 --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c @@ -43,48 +43,16 @@ struct psci_boot_args { static DEFINE_PER_CPU(struct psci_boot_args, cpu_on_args) = PSCI_BOOT_ARGS_INIT; static DEFINE_PER_CPU(struct psci_boot_args, suspend_args) = PSCI_BOOT_ARGS_INIT; -static u64 get_psci_func_id(struct kvm_cpu_context *host_ctxt) -{ - DECLARE_REG(u64, func_id, host_ctxt, 0); - - return func_id; -} - -static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) -{ - return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; -} - -static inline bool is_psci_0_1_cpu_suspend(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); -} - -static inline bool is_psci_0_1_cpu_on(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); -} - -static inline bool is_psci_0_1_cpu_off(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && - (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); -} - -static inline bool is_psci_0_1_migrate(u64 func_id) -{ - return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && - (func_id == kvm_host_psci_config.function_ids_0_1.migrate); -} +#define is_psci_0_1(what, func_id) \ + (kvm_host_psci_config.psci_0_1_ ## what ## _implemented && \ + (func_id) == kvm_host_psci_config.function_ids_0_1.what) static bool is_psci_0_1_call(u64 func_id) { - return is_psci_0_1_cpu_suspend(func_id) || - is_psci_0_1_cpu_on(func_id) || - is_psci_0_1_cpu_off(func_id) || - is_psci_0_1_migrate(func_id); + return (is_psci_0_1(cpu_suspend, func_id) || + is_psci_0_1(cpu_on, func_id) || + is_psci_0_1(cpu_off, func_id) || + is_psci_0_1(migrate, func_id)); } static bool is_psci_0_2_call(u64 func_id) @@ -94,16 +62,6 @@ static bool is_psci_0_2_call(u64 func_id) (PSCI_0_2_FN64(0) <= func_id && func_id <= PSCI_0_2_FN64(31)); } -static bool is_psci_call(u64 func_id) -{ - switch (kvm_host_psci_config.version) { - case PSCI_VERSION(0, 1): - return is_psci_0_1_call(func_id); - default: - return is_psci_0_2_call(func_id); - } -} - static unsigned long psci_call(unsigned long fn, unsigned long arg0, unsigned long arg1, unsigned long arg2) { @@ -273,14 +231,14 @@ asmlinkage void __noreturn kvm_host_psci_cpu_entry(bool is_cpu_on) static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) { - if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) + if (is_psci_0_1(cpu_off, func_id) || is_psci_0_1(migrate, func_id)) return psci_forward(host_ctxt); - else if (is_psci_0_1_cpu_on(func_id)) + if (is_psci_0_1(cpu_on, func_id)) return psci_cpu_on(func_id, host_ctxt); - else if (is_psci_0_1_cpu_suspend(func_id)) + if (is_psci_0_1(cpu_suspend, func_id)) return psci_cpu_suspend(func_id, host_ctxt); - else - return PSCI_RET_NOT_SUPPORTED; + + return PSCI_RET_NOT_SUPPORTED; } static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) @@ -322,20 +280,23 @@ static unsigned long psci_1_0_handler(u64 func_id, struct kvm_cpu_context *host_ bool kvm_host_psci_handler(struct kvm_cpu_context *host_ctxt) { - u64 func_id = get_psci_func_id(host_ctxt); + DECLARE_REG(u64, func_id, host_ctxt, 0); unsigned long ret; - if (!is_psci_call(func_id)) - return false; - switch (kvm_host_psci_config.version) { case PSCI_VERSION(0, 1): + if (!is_psci_0_1_call(func_id)) + return false; ret = psci_0_1_handler(func_id, host_ctxt); break; case PSCI_VERSION(0, 2): + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_0_2_handler(func_id, host_ctxt); break; default: + if (!is_psci_0_2_call(func_id)) + return false; ret = psci_1_0_handler(func_id, host_ctxt); break; } -- 2.29.2 -- 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