On 26/08/2024 11:03, Catalin Marinas wrote:
> On Mon, Aug 19, 2024 at 02:19:10PM +0100, Steven Price wrote:
>> +static bool rsi_version_matches(void)
>> +{
>> + unsigned long ver_lower, ver_higher;
>> + unsigned long ret = rsi_request_version(RSI_ABI_VERSION,
>> + &ver_lower,
>> + &ver_higher);
>> +
>> + if (ret == SMCCC_RET_NOT_SUPPORTED)
>> + return false;
>> +
>> + if (ret != RSI_SUCCESS) {
>> + pr_err("RME: RMM doesn't support RSI version %lu.%lu. Supported range: %lu.%lu-%lu.%lu\n",
>> + RSI_ABI_VERSION_MAJOR, RSI_ABI_VERSION_MINOR,
>> + RSI_ABI_VERSION_GET_MAJOR(ver_lower),
>> + RSI_ABI_VERSION_GET_MINOR(ver_lower),
>> + RSI_ABI_VERSION_GET_MAJOR(ver_higher),
>> + RSI_ABI_VERSION_GET_MINOR(ver_higher));
>> + return false;
>> + }
>> +
>> + pr_info("RME: Using RSI version %lu.%lu\n",
>> + RSI_ABI_VERSION_GET_MAJOR(ver_lower),
>> + RSI_ABI_VERSION_GET_MINOR(ver_lower));
>> +
>> + return true;
>> +}
>
> I don't have the spec at hand now (on a plane) but given the possibility
> of a 1.0 guest regressing on later RMM versions, I wonder whether we
> should simply bail out if it's not an exact version match. I forgot what
> the spec says about returned ranges (they were pretty confusing last
> time I checked).
Well the idea at least is that the RMM can tell us that it is providing
a 1.0 compatible interface. So it might be supporting 1.x but it's
promising that what it's providing is 1.0 compatible.
Indeed the spec allows the RMM to emulate 1.0 while supporting a higher
(incompatible) interface as well - which is where the version ranges
come in. So in the future we might negotiate versions with the RMM, or
opportunistically use newer features if the RMM provides them. But
obviously for now the guest is only 1.0.
I'd prefer not to add an exact version match because then upgrading the
RMM would break existing guests and would probably lead to pressure for
the RMM to simply lie to guests to avoid them breaking on upgrade.
>> +
>> +void __init arm64_rsi_setup_memory(void)
>> +{
>> + u64 i;
>> + phys_addr_t start, end;
>> +
>> + if (!is_realm_world())
>> + return;
>> +
>> + /*
>> + * 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".
>> + *
>> + * BUG_ON 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) {
>> + BUG_ON(rsi_set_memory_range_protected_safe(start, end));
>> + }
>
> Would it help debugging if we print the memory ranges as well rather
> than just a BUG_ON()?
>
Yes that would probably be useful - I'll fix that.
Thanks,
Steve
