Re: [PATCH v6 02/11] arm64: Detect if in a realm and set RIPAS RAM

[Steven Price]

On 08/10/2024 00:31, Gavin Shan wrote:
> On 10/5/24 12:42 AM, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>>
>> Detect that the VM is a realm guest by the presence of the RSI
>> interface. This is done after PSCI has been initialised so that we can
>> check the SMCCC conduit before making any RSI calls.
>>
>> If in a realm then all memory needs to be marked as RIPAS RAM initially,
>> the loader may or may not have done this for us. To be sure iterate over
>> all RAM and mark it as such. Any failure is fatal as that implies the
>> RAM regions passed to Linux are incorrect - which would mean failing
>> later when attempting to access non-existent RAM.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Co-developed-by: Steven Price <steven.price@xxxxxxx>
>> Signed-off-by: Steven Price <steven.price@xxxxxxx>
>> ---
>> Changes since v5:
>>   * Replace BUG_ON() with a panic() call that provides a message with the
>>     memory range that couldn't be set to RIPAS_RAM.
>>   * Move the call to arm64_rsi_init() later so that it is after PSCI,
>>     this means we can use arm_smccc_1_1_get_conduit() to check if it is
>>     safe to make RSI calls.
>> Changes since v4:
>>   * Minor tidy ups.
>> Changes since v3:
>>   * Provide safe/unsafe versions for converting memory to protected,
>>     using the safer version only for the early boot.
>>   * Use the new psci_early_test_conduit() function to avoid calling an
>>     SMC if EL3 is not present (or not configured to handle an SMC).
>> Changes since v2:
>>   * Use DECLARE_STATIC_KEY_FALSE rather than "extern struct
>>     static_key_false".
>>   * Rename set_memory_range() to rsi_set_memory_range().
>>   * Downgrade some BUG()s to WARN()s and handle the condition by
>>     propagating up the stack. Comment the remaining case that ends in a
>>     BUG() to explain why.
>>   * Rely on the return from rsi_request_version() rather than checking
>>     the version the RMM claims to support.
>>   * Rename the generic sounding arm64_setup_memory() to
>>     arm64_rsi_setup_memory() and move the call site to setup_arch().
>> ---
>>   arch/arm64/include/asm/rsi.h | 66 +++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/Makefile   |  3 +-
>>   arch/arm64/kernel/rsi.c      | 75 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/setup.c    |  3 ++
>>   4 files changed, 146 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm64/include/asm/rsi.h
>>   create mode 100644 arch/arm64/kernel/rsi.c
>>
> 
> Two nitpicks below.
> 
> Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
> 
>> diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
>> new file mode 100644
>> index 000000000000..e4c01796c618
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/rsi.h
>> @@ -0,0 +1,66 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 ARM Ltd.
>> + */
>> +
>> +#ifndef __ASM_RSI_H_
>> +#define __ASM_RSI_H_
>> +
>> +#include <linux/errno.h>
>> +#include <linux/jump_label.h>
>> +#include <asm/rsi_cmds.h>
>> +
>> +DECLARE_STATIC_KEY_FALSE(rsi_present);
>> +
>> +void __init arm64_rsi_init(void);
>> +
>> +static inline bool is_realm_world(void)
>> +{
>> +    return static_branch_unlikely(&rsi_present);
>> +}
>> +
>> +static inline int rsi_set_memory_range(phys_addr_t start, phys_addr_t
>> end,
>> +                       enum ripas state, unsigned long flags)
>> +{
>> +    unsigned long ret;
>> +    phys_addr_t top;
>> +
>> +    while (start != end) {
>> +        ret = rsi_set_addr_range_state(start, end, state, flags, &top);
>> +        if (WARN_ON(ret || top < start || top > end))
>> +            return -EINVAL;
>> +        start = top;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
> 
> The WARN_ON() is redundant when the caller is arm64_rsi_setup_memory(),
> where
> system panic is invoked on any errors. So we perhaps need to drop the
> WARN_ON().

Actually this is error when I was preparing the series - the WARN_ON is
then dropped in the next patch. Thanks for pointing it out!

> [...]
> 
>> +
>> +static void __init arm64_rsi_setup_memory(void)
>> +{
>> +    u64 i;
>> +    phys_addr_t start, end;
>> +
>> +    /*
>> +     * Iterate over the available memory ranges and convert the state to
>> +     * protected memory. We should take extra care to ensure that we
>> DO NOT
>> +     * permit any "DESTROYED" pages to be converted to "RAM".
>> +     *
>> +     * panic() is used because if the attempt to switch the memory to
>> +     * protected has failed here, then future accesses to the memory are
>> +     * simply going to be reflected as a SEA (Synchronous External
>> Abort)
>> +     * which we can't handle.  Bailing out early prevents the guest
>> limping
>> +     * on and dying later.
>> +     */
>> +    for_each_mem_range(i, &start, &end) {
>> +        if (rsi_set_memory_range_protected_safe(start, end))
>> +            panic("Failed to set memory range to protected: %pa-%pa",
>> +                  &start, &end);
>> +    }
>> +}
>> +
> 
> {} is needed since the panic statement spans multiple lines.

Ack.

Thanks,
Steve