From: Steven Price <steven.price@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev,
Suzuki K Poulose <suzuki.poulose@arm.com>,
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: Wed, 15 May 2024 16:03:25 +0100 [thread overview]
Message-ID: <6fef6e77-a173-4c66-bf6c-574c19820dfa@arm.com> (raw)
In-Reply-To: <Zj5a9Kt6r7U9WN5E@arm.com>
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 ;).
Noted! ;)
>> + */
>> +
>> +#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().
Makes sense.
>> +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().
I think you're right, these shouldn't be (immediately) fatal - if this
is a change happening at runtime it might be possible to handle it.
However, at boot time it makes no sense to try to continue (which is
what I was originally looking at when this was written) as the
environment isn't what the guest is expecting, and continuing will
either lead to a later crash, attestation failure, or potential exploit
if the guest can be somehow be tricked to use the shared mapping rather
than the protected one.
I'll update the set_memory_range...() functions to return an error code
and push the boot BUG_ON up the chain (with a comment). But this is
still in the "should never happen" situation so I'll leave a WARN_ON here.
>> 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?
It's used by drivers/virt/coco/arm-cca-guest/arm-cca-guest.c (through
is_realm_world()) which can be built as a module - see patch 14.
>> +
>> +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.
There's been a fair bit of discussion around versioning. Currently the
RMM implementation is very much a "get version" function. The issue was
what happens if in the future there is an incompatible RMM spec (i.e.
v2.0). The intention is that old OSes will provide the older version
number and give the RMM the opportunity to 'emulate' it. Equally where
the OS supports multiple versions then there's a need to negotiate a
commonly accepted version.
In terms of naming - mostly I've tried to just follow the spec, but the
naming in the spec isn't great. Calling the function rsi_version() would
be confusing so a verb is needed, but I'm not hung up on the verb. I'll
change it to rsi_request_version().
>> +
>> + 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).
Indeed - I got similar feedback on the RMI side. The spec evolved and I
forgot to update it. It should be sufficient (for now) to just look for
RSI_SUCCESS. Only when we start supporting multiple versions (on the
Linux side) do we need to look at the returned version numbers.
>> +
>> + 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.
>> +{
>> + 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?
Will change.
>> +
>> + /*
>> + * 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.
As Suzuki said - it's needed for "earlycon" to work - I'll put a comment
in explaining.
>>
>> 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().
>
Will move.
Thanks,
Steve
next prev parent reply other threads:[~2024-05-15 15:03 UTC|newest]
Thread overview: 135+ 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-06-01 20:40 ` Jason Gunthorpe
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
2024-05-16 14:32 ` Catalin Marinas
2024-05-15 15:03 ` Steven Price [this message]
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-05-31 16:29 ` Suzuki K Poulose
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-05-31 16:03 ` Steven Price
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=6fef6e77-a173-4c66-bf6c-574c19820dfa@arm.com \
--to=steven.price@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=suzuki.poulose@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).