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 21648C25B75 for ; Tue, 14 May 2024 10:18:40 +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-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=FGtZLMh1femp8Y1aKKIbRHmwKea7HuiS5QaWfRqpalU=; b=sPD8ZDkZTlXxp5 4lccgb+aj21QmUse427sCEYLP+mfAuMfBtKLGIlHF+bJ8B7UOPAZSJrhnfbX6lJlA+BjbMb++6LEx RLkE7AHp8+vi+soRJyZUIysiNnAefWMH3fWtpssgSzQ9iOZwHfilHawOy9zKm9fDJnLoyKxfgJvAa 907Zi5ggUyj+lZrmQAaY/sZWTapTNcAr5MS1C8EzOOJUamamN60yduXdBeYcFX32s00Fj0GGfDD7K VjU0lonBUUpFJOjhZ0rtScSdZ+4U2yKG/i+Z+VuTyygf8weytbLC5SR+qchG8uQA5MvbL/8tY81Lo jy+LAO6UUhIuP7Czmkdg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s6pEp-0000000FY5h-07YL; Tue, 14 May 2024 10:18:27 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s6pEl-0000000FY4V-46mz for linux-arm-kernel@lists.infradead.org; Tue, 14 May 2024 10:18:25 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E51B61007; Tue, 14 May 2024 03:18:45 -0700 (PDT) Received: from [10.57.81.220] (unknown [10.57.81.220]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 27A3D3F762; Tue, 14 May 2024 03:18:15 -0700 (PDT) Message-ID: Date: Tue, 14 May 2024 11:18:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 02/14] arm64: Detect if in a realm and set RIPAS RAM Content-Language: en-GB To: Catalin Marinas , Steven Price Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev, 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 References: <20240412084213.1733764-1-steven.price@arm.com> <20240412084213.1733764-3-steven.price@arm.com> From: Suzuki K Poulose In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240514_031824_167090_AD22DED7 X-CRM114-Status: GOOD ( 48.51 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Catalin, On 10/05/2024 18:35, Catalin Marinas wrote: > 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 ;). This was written in 2023 ;-), hasn't changed much since, hence 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(). Agree > >> +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(). The BUG_ON was put in to stop the guest from running, when it detects that it cannot transition a given IPA to a desired state. This could happen manily if the Host described some address to the Guest and has backed out from the promise. However, thinking about this a little deeper, we could relax this a bit and leave it to the "caller" to take an action. e.g. 1. If this fails for "Main memory" -> RIPAS_RAM transition, it is fatal. 2. If we are transitioning some random IPA to RIPAS_EMPTY (for setting up in-realm MMIO, which we do not support yet), it may be dealt with. We could have other cases in the future where we support trusted I/O, and a failure to transition is not end of the world, but simply refusing to use a device for e.g. That said, the current uses in the kernel are always fatal. So, the BUG_ON is justified as it stands. Happy to change either ways. > > 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? Yes, for e.g., TSM_CONFIG report for attestation framework. Patch 14 in the list. > >> + >> +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. Agree. > > 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 Ack. I guess this was never changed since the spec update. I have requested a similar change for RMI_ABI_VERSION checks. > 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. Ack. arm64_rsi_setup_memory() ? I agree, we should "rsi" fy the names. > >> +{ >> + 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. We must do this before we setup the "earlycon", so that the console is accessed using shared alias and that happens via parse_early_param() :-(. > >> >> 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(). Makes sense. This was done to make sure we process all the memory regions, as soon as they are identified. Suzuki > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel