Message-ID: <d55a24d2-bad9-40c7-8a2e-4a7bebe9c682@arm.com>
Date: Mon, 19 Aug 2024 15:10:14 +0100
From: Steven Price <steven.price@....com>
To: Suzuki K Poulose <suzuki.poulose@....com>, kvm@...r.kernel.org,
 kvmarm@...ts.linux.dev
Cc: Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
 Will Deacon <will@...nel.org>, James Morse <james.morse@....com>,
 Oliver Upton <oliver.upton@...ux.dev>, Zenghui Yu <yuzenghui@...wei.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Joey Gouly <joey.gouly@....com>, Alexandru Elisei
 <alexandru.elisei@....com>, Christoffer Dall <christoffer.dall@....com>,
 Fuad Tabba <tabba@...gle.com>, linux-coco@...ts.linux.dev,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>,
 Gavin Shan <gshan@...hat.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 Alper Gun <alpergun@...gle.com>
Subject: Re: [PATCH v5 05/19] arm64: Detect if in a realm and set RIPAS RAM

On 19/08/2024 15:04, Suzuki K Poulose wrote:
> Hi Steven
> 
> On 19/08/2024 14:19, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@....com>
>>
>> Detect that the VM is a realm guest by the presence of the RSI
>> interface.
>>
>> 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@....com>
>> Co-developed-by: Steven Price <steven.price@....com>
>> Signed-off-by: Steven Price <steven.price@....com>
>> ---
>> 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 | 65 ++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/Makefile   |  3 +-
>>   arch/arm64/kernel/rsi.c      | 78 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/setup.c    |  8 ++++
>>   4 files changed, 153 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm64/include/asm/rsi.h
>>   create mode 100644 arch/arm64/kernel/rsi.c
>>
>> diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
>> new file mode 100644
>> index 000000000000..2bc013badbc3
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/rsi.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 ARM Ltd.
>> + */
>> +
>> +#ifndef __ASM_RSI_H_
>> +#define __ASM_RSI_H_
>> +
>> +#include <linux/jump_label.h>
>> +#include <asm/rsi_cmds.h>
>> +
>> +DECLARE_STATIC_KEY_FALSE(rsi_present);
>> +
>> +void __init arm64_rsi_init(void);
>> +void __init arm64_rsi_setup_memory(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;
>> +}
>> +
>> +/*
>> + * Convert the specified range to RAM. Do not use this if you rely on
>> the
>> + * contents of a page that may already be in RAM state.
>> + */
>> +static inline int rsi_set_memory_range_protected(phys_addr_t start,
>> +                         phys_addr_t end)
>> +{
>> +    return rsi_set_memory_range(start, end, RSI_RIPAS_RAM,
>> +                    RSI_CHANGE_DESTROYED);
>> +}
>> +
>> +/*
>> + * Convert the specified range to RAM. Do not convert any pages that
>> may have
>> + * been DESTROYED, without our permission.
>> + */
>> +static inline int rsi_set_memory_range_protected_safe(phys_addr_t start,
>> +                              phys_addr_t end)
>> +{
>> +    return rsi_set_memory_range(start, end, RSI_RIPAS_RAM,
>> +                    RSI_NO_CHANGE_DESTROYED);
>> +}
>> +
>> +static inline int rsi_set_memory_range_shared(phys_addr_t start,
>> +                          phys_addr_t end)
>> +{
>> +    return rsi_set_memory_range(start, end, RSI_RIPAS_EMPTY,
>> +                    RSI_NO_CHANGE_DESTROYED);
> 
> I think this should be RSI_CHANGE_DESTROYED, as we are transitioning a
> page to "shared" (i.e, IPA state to EMPTY) and we do not expect the data
> to be retained over the transition. Thus we do not care if the IPA was
> in RIPAS_DESTROYED.

Fair point - although something has gone wrong if the VMM has destroyed
the memory we're calling this on. But it's not going to cause problems
using RSI_CHANGE_DESTROYED and might be (slightly) more efficient.

Thanks,

Steve

> Rest looks good to me.
> 
> 
> Suzuki

Powered by blists - more mailing lists