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 BF95FC25B10 for ; Fri, 10 May 2024 17:36:13 +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=/vyPz3KX8nQHNO46mY6J1NejpyGCBNoh/PWvvms3nyg=; b=AdUYoGXn3j4W/M rlnIwS9fRLSi5hXmOr2kGFgtSESyR4GT2S9H7fMJiIw0QgDWAxSoL/pcKccOkMRCno6Dc4Pl2Ygk/ yefCFW1f34xS0oxsyqgOUAi8R4bV9ahsM4Ikq5kL6bfnKE/5tIPgB0AqtVk2xJ0am8vRZPzS4GGbH qHlWGkq0kCEVv/MyhU2h7TcEXbcueruM6RdvTfOEB/VdrzZXYpze3wIWa/QVeq9uJeqMwFHmGinWi b1MT0xpVR26S5i40Sj1iXXYSzPZyxiRRYPsrSwU0g/T9OnAHi7Wl8+dhhjDQaTpIxByFYgYaG3SHV xx5qAmy9rF75juy7NZeQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s5UA4-000000061YN-269i; Fri, 10 May 2024 17:36:00 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s5UA1-000000061Xn-1nGT for linux-arm-kernel@lists.infradead.org; Fri, 10 May 2024 17:35:58 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 37157CE1EA8; Fri, 10 May 2024 17:35:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3BEA9C113CC; Fri, 10 May 2024 17:35:51 +0000 (UTC) Date: Fri, 10 May 2024 18:35:48 +0100 From: Catalin Marinas To: Steven Price Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev, Suzuki K Poulose , Marc Zyngier , Will Deacon , James Morse , Oliver Upton , Zenghui Yu , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Joey Gouly , Alexandru Elisei , Christoffer Dall , Fuad Tabba , linux-coco@lists.linux.dev, Ganapatrao Kulkarni Subject: Re: [PATCH v2 02/14] arm64: Detect if in a realm and set RIPAS RAM Message-ID: References: <20240412084213.1733764-1-steven.price@arm.com> <20240412084213.1733764-3-steven.price@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240412084213.1733764-3-steven.price@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240510_103557_854985_5D0F33E0 X-CRM114-Status: GOOD ( 35.91 ) 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 Fri, Apr 12, 2024 at 09:42:01AM +0100, Steven Price wrote: > diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h > new file mode 100644 > index 000000000000..3b56aac5dc43 > --- /dev/null > +++ b/arch/arm64/include/asm/rsi.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2023 ARM Ltd. You may want to update the year ;). > + */ > + > +#ifndef __ASM_RSI_H_ > +#define __ASM_RSI_H_ > + > +#include > +#include > + > +extern struct static_key_false rsi_present; Nitpick: we tend to use DECLARE_STATIC_KEY_FALSE(), it pairs with DEFINE_STATIC_KEY_FALSE(). > +void arm64_setup_memory(void); > + > +void __init arm64_rsi_init(void); > +static inline bool is_realm_world(void) > +{ > + return static_branch_unlikely(&rsi_present); > +} > + > +static inline void set_memory_range(phys_addr_t start, phys_addr_t end, > + enum ripas state) > +{ > + unsigned long ret; > + phys_addr_t top; > + > + while (start != end) { > + ret = rsi_set_addr_range_state(start, end, state, &top); > + BUG_ON(ret); > + BUG_ON(top < start); > + BUG_ON(top > end); Are these always fatal? BUG_ON() is frowned upon in general. The alternative would be returning an error code from the function and maybe printing a warning here (it seems that some people don't like WARN_ON either but it's better than BUG_ON; could use a pr_err() instead). Also if something's wrong with the RSI interface to mess up the return values, it will be hard to debug just from those BUG_ON(). If there's no chance of continuing beyond the point, add a comment on why we have a BUG_ON(). > diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c > new file mode 100644 > index 000000000000..1076649ac082 > --- /dev/null > +++ b/arch/arm64/kernel/rsi.c > @@ -0,0 +1,58 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 ARM Ltd. > + */ > + > +#include > +#include > +#include > + > +DEFINE_STATIC_KEY_FALSE_RO(rsi_present); > +EXPORT_SYMBOL(rsi_present); Does this need to be made available to loadable modules? > + > +static bool rsi_version_matches(void) > +{ > + unsigned long ver; > + unsigned long ret = rsi_get_version(RSI_ABI_VERSION, &ver, NULL); I wonder whether rsi_get_version() is the right name (I know it was introduced in the previous patch but the usage is here, hence my comment). From the RMM spec, this looks more like an rsi_request_version() to me. TBH, the RMM spec around versioning doesn't fully make sense to me ;). I assume people working on it had some good reasons around the lower revision reporting in case of an error. > + > + if (ret == SMCCC_RET_NOT_SUPPORTED) > + return false; > + > + if (ver != RSI_ABI_VERSION) { > + pr_err("RME: RSI version %lu.%lu not supported\n", > + RSI_ABI_VERSION_GET_MAJOR(ver), > + RSI_ABI_VERSION_GET_MINOR(ver)); > + return false; > + } The above check matches what the spec says but wouldn't it be clearer to just check for ret == RSI_SUCCESS? It saves one having to read the spec to figure out what lower revision actually means in the spec (not the actual lowest supported but the highest while still lower than the requested one _or_ equal to the higher revision if the lower is higher than the requested one - if any of this makes sense to people ;), I'm sure I missed some other combinations). > + > + pr_info("RME: Using RSI version %lu.%lu\n", > + RSI_ABI_VERSION_GET_MAJOR(ver), > + RSI_ABI_VERSION_GET_MINOR(ver)); > + > + return true; > +} > + > +void arm64_setup_memory(void) I would give this function a better name, something to resemble the RSI setup. Similarly for others like set_memory_range_protected/shared(). Some of the functions have 'rsi' in the name like arm64_rsi_init() but others don't and at a first look they'd seem like some generic memory setup on arm64, not RSI-specific. > +{ > + u64 i; > + phys_addr_t start, end; > + > + if (!static_branch_unlikely(&rsi_present)) > + return; We have an accessor for rsi_present - is_realm_world(). Why not use that? > + > + /* > + * Iterate over the available memory ranges > + * and convert the state to protected memory. > + */ > + for_each_mem_range(i, &start, &end) { > + set_memory_range_protected(start, end); > + } > +} > + > +void __init arm64_rsi_init(void) > +{ > + if (!rsi_version_matches()) > + return; > + > + static_branch_enable(&rsi_present); > +} > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 65a052bf741f..a4bd97e74704 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -43,6 +43,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -293,6 +294,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) > * cpufeature code and early parameters. > */ > jump_label_init(); > + /* Init RSI after jump_labels are active */ > + arm64_rsi_init(); > parse_early_param(); Does it need to be this early? It's fine for now but I wonder whether we may have some early parameter at some point that could influence what we do in the arm64_rsi_init(). I'd move it after or maybe even as part of the arm64_setup_memory(), though I haven't read the following patches if they update this function. > > dynamic_scs_init(); > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 03efd86dce0a..786fd6ce5f17 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -40,6 +40,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -313,6 +314,7 @@ void __init arm64_memblock_init(void) > early_init_fdt_scan_reserved_mem(); > > high_memory = __va(memblock_end_of_DRAM() - 1) + 1; > + arm64_setup_memory(); > } This feels like a random placement. This function is about memblock initialisation. You might as well put it in paging_init(), it could make more sense there. But I'd rather keep it in setup_arch() immediately after arm64_memblock_init(). -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel