From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>,
Steven Price <steven.price@arm.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev,
Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
James Morse <james.morse@arm.com>,
Oliver Upton <oliver.upton@linux.dev>,
Zenghui Yu <yuzenghui@huawei.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Joey Gouly <joey.gouly@arm.com>,
Alexandru Elisei <alexandru.elisei@arm.com>,
Christoffer Dall <christoffer.dall@arm.com>,
Fuad Tabba <tabba@google.com>,
linux-coco@lists.linux.dev,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
Subject: Re: [PATCH v2 02/14] arm64: Detect if in a realm and set RIPAS RAM
Date: Tue, 14 May 2024 11:18:13 +0100 [thread overview]
Message-ID: <a5dbe87b-dcb0-4467-9002-775fbdfb239d@arm.com> (raw)
In-Reply-To: <Zj5a9Kt6r7U9WN5E@arm.com>
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 <linux/jump_label.h>
>> +#include <asm/rsi_cmds.h>
>> +
>> +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 <linux/jump_label.h>
>> +#include <linux/memblock.h>
>> +#include <asm/rsi.h>
>> +
>> +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 <asm/cpu_ops.h>
>> #include <asm/kasan.h>
>> #include <asm/numa.h>
>> +#include <asm/rsi.h>
>> #include <asm/scs.h>
>> #include <asm/sections.h>
>> #include <asm/setup.h>
>> @@ -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 <asm/kvm_host.h>
>> #include <asm/memory.h>
>> #include <asm/numa.h>
>> +#include <asm/rsi.h>
>> #include <asm/sections.h>
>> #include <asm/setup.h>
>> #include <linux/sizes.h>
>> @@ -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
>
next prev parent reply other threads:[~2024-05-14 10:18 UTC|newest]
Thread overview: 132+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 8:40 [v2] Support for Arm CCA VMs on Linux Steven Price
2024-04-11 18:54 ` Itaru Kitayama
2024-04-15 8:14 ` Steven Price
2024-04-12 8:41 ` [PATCH v2 00/14] arm64: Support for running as a guest in Arm CCA Steven Price
2024-04-12 8:42 ` [PATCH v2 01/14] arm64: rsi: Add RSI definitions Steven Price
2024-04-12 8:42 ` [PATCH v2 02/14] arm64: Detect if in a realm and set RIPAS RAM Steven Price
2024-05-10 17:35 ` Catalin Marinas
2024-05-14 10:18 ` Suzuki K Poulose [this message]
2024-05-16 14:32 ` Catalin Marinas
2024-05-15 15:03 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 03/14] arm64: realm: Query IPA size from the RMM Steven Price
2024-05-13 14:03 ` Catalin Marinas
2024-05-16 15:13 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 04/14] arm64: Mark all I/O as non-secure shared Steven Price
2024-04-12 8:42 ` [PATCH v2 05/14] fixmap: Allow architecture overriding set_fixmap_io Steven Price
2024-04-12 8:42 ` [PATCH v2 06/14] arm64: Override set_fixmap_io Steven Price
2024-05-13 16:14 ` Catalin Marinas
2024-05-14 10:21 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 07/14] arm64: Make the PHYS_MASK_SHIFT dynamic Steven Price
2024-05-13 16:38 ` Catalin Marinas
2024-05-16 15:34 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 08/14] arm64: Enforce bounce buffers for realm DMA Steven Price
2024-05-13 16:56 ` Catalin Marinas
2024-04-12 8:42 ` [PATCH v2 09/14] arm64: Enable memory encrypt for Realms Steven Price
2024-04-15 3:13 ` kernel test robot
2024-04-25 13:42 ` Suzuki K Poulose
2024-04-25 15:52 ` Steven Price
2024-04-25 16:29 ` Suzuki K Poulose
2024-04-25 18:16 ` Emanuele Rocca
2024-05-14 18:00 ` Catalin Marinas
2024-05-15 10:47 ` Suzuki K Poulose
2024-05-16 7:48 ` Catalin Marinas
2024-05-16 9:06 ` Suzuki K Poulose
2024-05-20 16:53 ` Catalin Marinas
2024-05-20 20:32 ` Michael Kelley
2024-05-21 10:14 ` Catalin Marinas
2024-05-21 15:58 ` Michael Kelley
2024-04-12 8:42 ` [PATCH v2 10/14] arm64: Force device mappings to be non-secure shared Steven Price
2024-05-15 9:01 ` Catalin Marinas
2024-05-15 11:00 ` Suzuki K Poulose
2024-05-17 9:34 ` Catalin Marinas
2024-04-12 8:42 ` [PATCH v2 11/14] efi: arm64: Map Device with Prot Shared Steven Price
2024-04-12 8:42 ` [PATCH v2 12/14] arm64: realm: Support nonsecure ITS emulation shared Steven Price
2024-05-15 11:01 ` Catalin Marinas
2024-05-22 15:52 ` Steven Price
2024-05-22 17:05 ` Catalin Marinas
2024-05-23 9:57 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 13/14] arm64: rsi: Interfaces to query attestation token Steven Price
2024-05-15 11:10 ` Catalin Marinas
2024-05-22 15:52 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 14/14] virt: arm-cca-guest: TSM_REPORT support for realms Steven Price
2024-04-24 13:06 ` Thomas Fossati
2024-04-24 13:27 ` Suzuki K Poulose
2024-04-24 13:19 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 00/43] arm64: Support for Arm CCA in KVM Steven Price
2024-04-12 8:42 ` [PATCH v2 01/43] KVM: Prepare for handling only shared mappings in mmu_notifier events Steven Price
2024-04-25 9:48 ` Fuad Tabba
2024-04-25 15:58 ` Steven Price
2024-04-25 22:56 ` Sean Christopherson
2024-04-12 8:42 ` [PATCH v2 02/43] kvm: arm64: pgtable: Track the number of pages in the entry level Steven Price
2024-04-12 8:42 ` [PATCH v2 03/43] kvm: arm64: Include kvm_emulate.h in kvm/arm_psci.h Steven Price
2024-04-12 8:42 ` [PATCH v2 04/43] arm64: RME: Handle Granule Protection Faults (GPFs) Steven Price
2024-04-16 11:17 ` Suzuki K Poulose
2024-04-18 13:17 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 05/43] arm64: RME: Add SMC definitions for calling the RMM Steven Price
2024-04-16 12:38 ` Suzuki K Poulose
2024-04-18 13:17 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 06/43] arm64: RME: Add wrappers for RMI calls Steven Price
2024-04-16 13:14 ` Suzuki K Poulose
2024-04-19 11:18 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 07/43] arm64: RME: Check for RME support at KVM init Steven Price
2024-04-16 13:30 ` Suzuki K Poulose
2024-04-22 15:39 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 08/43] arm64: RME: Define the user ABI Steven Price
2024-04-12 8:42 ` [PATCH v2 09/43] arm64: RME: ioctls to create and configure realms Steven Price
2024-04-17 9:51 ` Suzuki K Poulose
2024-04-22 16:33 ` Steven Price
2024-04-18 16:04 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 10/43] kvm: arm64: Expose debug HW register numbers for Realm Steven Price
2024-04-12 8:42 ` [PATCH v2 11/43] arm64: kvm: Allow passing machine type in KVM creation Steven Price
2024-04-17 10:20 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 12/43] arm64: RME: Keep a spare page delegated to the RMM Steven Price
2024-04-17 10:19 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 13/43] arm64: RME: RTT handling Steven Price
2024-04-17 13:37 ` Suzuki K Poulose
2024-04-24 10:59 ` Steven Price
2024-04-12 8:42 ` [PATCH v2 14/43] arm64: RME: Allocate/free RECs to match vCPUs Steven Price
2024-04-18 9:23 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 15/43] arm64: RME: Support for the VGIC in realms Steven Price
2024-04-12 8:42 ` [PATCH v2 16/43] KVM: arm64: Support timers in realm RECs Steven Price
2024-04-18 9:30 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 17/43] arm64: RME: Allow VMM to set RIPAS Steven Price
2024-04-19 9:34 ` Suzuki K Poulose
2024-04-19 10:20 ` Suzuki K Poulose
2024-05-01 15:47 ` Steven Price
2024-05-02 10:16 ` Suzuki K Poulose
2024-04-25 9:53 ` Fuad Tabba
2024-05-01 14:27 ` Jean-Philippe Brucker
2024-05-01 14:56 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 18/43] arm64: RME: Handle realm enter/exit Steven Price
2024-04-19 13:00 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 19/43] KVM: arm64: Handle realm MMIO emulation Steven Price
2024-04-12 8:42 ` [PATCH v2 20/43] arm64: RME: Allow populating initial contents Steven Price
2024-04-19 13:17 ` Suzuki K Poulose
2024-04-12 8:42 ` [PATCH v2 21/43] arm64: RME: Runtime faulting of memory Steven Price
2024-04-25 10:43 ` Fuad Tabba
2024-04-12 8:42 ` [PATCH v2 22/43] KVM: arm64: Handle realm VCPU load Steven Price
2024-04-12 8:42 ` [PATCH v2 23/43] KVM: arm64: Validate register access for a Realm VM Steven Price
2024-04-12 8:42 ` [PATCH v2 24/43] KVM: arm64: Handle Realm PSCI requests Steven Price
2024-04-12 8:42 ` [PATCH v2 25/43] KVM: arm64: WARN on injected undef exceptions Steven Price
2024-04-12 8:42 ` [PATCH v2 26/43] arm64: Don't expose stolen time for realm guests Steven Price
2024-04-12 8:42 ` [PATCH v2 27/43] arm64: rme: allow userspace to inject aborts Steven Price
2024-04-12 8:42 ` [PATCH v2 28/43] arm64: rme: support RSI_HOST_CALL Steven Price
2024-04-12 8:42 ` [PATCH v2 29/43] arm64: rme: Allow checking SVE on VM instance Steven Price
2024-04-12 8:42 ` [PATCH v2 30/43] arm64: RME: Always use 4k pages for realms Steven Price
2024-04-12 8:42 ` [PATCH v2 31/43] arm64: rme: Prevent Device mappings for Realms Steven Price
2024-04-12 8:42 ` [PATCH v2 32/43] arm_pmu: Provide a mechanism for disabling the physical IRQ Steven Price
2024-04-12 8:42 ` [PATCH v2 33/43] arm64: rme: Enable PMU support with a realm guest Steven Price
2024-04-13 23:44 ` kernel test robot
2024-04-18 16:06 ` Suzuki K Poulose
2024-04-12 8:43 ` [PATCH v2 34/43] kvm: rme: Hide KVM_CAP_READONLY_MEM for realm guests Steven Price
2024-04-12 8:43 ` [PATCH v2 35/43] arm64: RME: Propagate number of breakpoints and watchpoints to userspace Steven Price
2024-04-12 8:43 ` [PATCH v2 36/43] arm64: RME: Set breakpoint parameters through SET_ONE_REG Steven Price
2024-04-12 8:43 ` [PATCH v2 37/43] arm64: RME: Initialize PMCR.N with number counter supported by RMM Steven Price
2024-04-12 8:43 ` [PATCH v2 38/43] arm64: RME: Propagate max SVE vector length from RMM Steven Price
2024-04-12 8:43 ` [PATCH v2 39/43] arm64: RME: Configure max SVE vector length for a Realm Steven Price
2024-04-12 8:43 ` [PATCH v2 40/43] arm64: RME: Provide register list for unfinalized RME RECs Steven Price
2024-04-12 8:43 ` [PATCH v2 41/43] arm64: RME: Provide accurate register list Steven Price
2024-04-12 8:43 ` [PATCH v2 42/43] arm64: kvm: Expose support for private memory Steven Price
2024-04-25 14:44 ` Fuad Tabba
2024-04-12 8:43 ` [PATCH v2 43/43] KVM: arm64: Allow activating realms Steven Price
2024-04-12 16:52 ` [v2] Support for Arm CCA VMs on Linux Jean-Philippe Brucker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5dbe87b-dcb0-4467-9002-775fbdfb239d@arm.com \
--to=suzuki.poulose@arm.com \
--cc=alexandru.elisei@arm.com \
--cc=catalin.marinas@arm.com \
--cc=christoffer.dall@arm.com \
--cc=gankulkarni@os.amperecomputing.com \
--cc=james.morse@arm.com \
--cc=joey.gouly@arm.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.linux.dev \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=steven.price@arm.com \
--cc=tabba@google.com \
--cc=will@kernel.org \
--cc=yuzenghui@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).